r109144 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109143‎ | r109144 | r109145 >
Date:13:51, 17 January 2012
Author:nikerabbit
Status:ok (Comments)
Tags:i18nreview, miscextensions 
Comment:
Modify directly the style attr, .css didn't work - more tests passing now
Some whitespace and test comment cleanups
Modified paths:
  • /trunk/extensions/WebFonts/tests/qunit/ext.webfonts.tests.js (modified) (history)

Diff [purge]

Index: trunk/extensions/WebFonts/tests/qunit/ext.webfonts.tests.js
——@@ -16,7 +16,7 @@
1717 assertTrue( mw.webfonts.addFont( 'Lohit Devanagari' ) , 'Add the Lohit Devanagari font again' );
1818 assertTrue( loadedFontsSize === mw.webfonts.fonts.length , 'Already loaded fonts not loaded again.' );
1919 assertFalse( mw.webfonts.addFont( 'Some Non existing font' ), 'addFont should return false if font not found' );
20 - assertTrue( cssRulesLength + 1 === document.styleSheets.length, 'No new css rules added at this stage' );
 20+ assertTrue( cssRulesLength + 1 === document.styleSheets.length, 'No new css rules added at this stage' );
2121 } );
2222
2323 test( '-- Dynamic font loading based on lang attribute', function() {
——@@ -24,14 +24,14 @@
2525
2626 ok( $( 'body' ).append( "<p class='webfonts-testing-lang-attr'>Some Content</p>") );
2727 $testElement = $( 'p.webfonts-testing-lang-attr' )
28 - assertTrue( $testElement !== undefined, 'Test element added' ) ;
 28+ assertTrue( $testElement !== [], 'Test element added' ) ;
2929
3030 ok( mw.webfonts.loadFontsForLangAttr() );
3131 assertFalse( $testElement.hasClass( 'webfonts-lang-attr' ), 'The element has no webfonts-lang-attr class since there is no lang attribute' ) ;
3232
3333 ok( $testElement.attr( 'lang' , 'en' ) , 'Set lang attribute as english' );
3434 ok( mw.webfonts.loadFontsForLangAttr() );
35 - assertFalse( $testElement.hasClass( 'webfonts-lang-attr' ), 'The element has no webfonts-lang-attr class since there is en lang has no fonts available' ) ;
 35+ assertFalse( $testElement.hasClass( 'webfonts-lang-attr' ), 'The element has no webfonts-lang-attr class since en lang has no fonts available' ) ;
3636
3737 ok( $testElement.attr( 'lang' , 'ta' ) , 'Set lang attribute as Tamil' );
3838 var cssRulesLength = document.styleSheets.length;
——@@ -49,24 +49,24 @@
5050 test( '-- Dynamic font loading based on font-family style attribute', function() {
5151 expect( 14 )
5252
53 - ok( $( 'body' ).append( "<p class='webfonts-testing-font-family-style'>Some Content</p>") );
 53+ ok( $( 'body' ).append( "<p class='webfonts-testing-font-family-style'>Some Content</p>" ) );
5454 var $testElement = $( 'p.webfonts-testing-font-family-style' );
55 - assertTrue( $testElement !== undefined, 'Test element added' ) ;
 55+ assertTrue( $testElement !== [], 'Test element added' ) ;
5656
57 - $testElement.css( 'font-family', 'RufScript, Arial, Helvetica, sans');
 57+ $testElement.attr( 'style','font-family: RufScript, Arial, Helvetica, sans' );
5858 var cssRulesLength = document.styleSheets.length;
5959 assertTrue( $.inArray( 'RufScript', mw.webfonts.fonts ) === -1 , 'RufScript Font not loaded yet' );
6060 ok( mw.webfonts.loadFontsForFontFamilyStyle() );
6161 assertTrue( $.inArray( 'RufScript', mw.webfonts.fonts ) >= 0 , 'Font loaded' );
6262 assertTrue( cssRulesLength +1 === document.styleSheets.length, 'New css rule added to the document' );
6363
64 - $testElement.css( 'font-family', 'NonExistingFont, Arial, Helvetica, sans' );
 64+ $testElement.attr( 'style','font-family: NonExistingFont, Arial, Helvetica, sans' );
6565 cssRulesLength = document.styleSheets.length;
6666 ok( mw.webfonts.loadFontsForFontFamilyStyle() );
6767 assertTrue( $.inArray( 'NonExistingFont', mw.webfonts.fonts ) === -1 , 'Font not loaded since it is not existing, including fallback fonts' );
6868 assertTrue( cssRulesLength === document.styleSheets.length, 'No new css rule added to the document' );
6969
70 - $testElement.css( 'font-family', 'NonExistingFont, AnjaliOldLipi, Arial, Helvetica, sans');
 70+ $testElement.attr( 'style','font-family: NonExistingFont, AnjaliOldLipi, Arial, Helvetica, sans' );
7171 cssRulesLength = document.styleSheets.length;
7272 assertTrue( $.inArray( 'AnjaliOldLipi', mw.webfonts.fonts ) === -1 , 'Fallback font AnjaliOldLipi not loaded yet' );
7373 ok( mw.webfonts.loadFontsForFontFamilyStyle() );

Follow-up revisions

RevisionCommit summaryAuthorDate
r109677[WebFonts.tests] Fixes...krinkle06:21, 21 January 2012
r111374[WebFonts tests] Refactor / fix breakage...krinkle14:49, 13 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r109138More test cases for webfonts...santhosh12:14, 17 January 2012

Comments

#Comment by Krinkle (talk | contribs)   06:33, 21 January 2012

bogus $testElement !== [] removed in r109677.

#Comment by Krinkle (talk | contribs)   06:37, 21 January 2012

Modify directly the style attr, .css didn't work

-	$testElement.css( 'font-family', 'NonExistingFont, AnjaliOldLipi, Arial, Helvetica, sans');
+	$testElement.attr( 'style','font-family: NonExistingFont, AnjaliOldLipi, Arial, Helvetica, sans' );

This is a problem imho. Considering that the main WebFonts module uses this all-over: 

 $body.css( 'font-family' )
..
$( '.webfonts-lang-attr' ).css( 'font-family', '' )
..
$( 'body, input, select, textarea' ).css(
				'font-family', '"' + font + '", Helvetica, Arial, sans-serif'
			);
..
	$(el).css( 'font-family', fontFamily )

If that really doesn't work then the main module can't work either. Needs more investigation, perhaps the test failure was not caused by this ?

#Comment by Nikerabbit (talk | contribs)   09:28, 21 January 2012

I investigated it as far that the *[style] rule didn't find the newly added element which had fontFamily set with .css().

#Comment by Nikerabbit (talk | contribs)   13:34, 23 January 2012

So, I think it is only about how it affects the DOM/HTML structure or whatever, and not that the style would not be applied.

#Comment by Hashar (talk | contribs)   21:41, 13 February 2012

Looks like most of this change was modified / refactored by Timo in subsequent revision. So marking this one ok in the sense it is not blocking live deployement.

Status & tagging log