r96261 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96260‎ | r96261 | r96262 >
Date:21:50, 4 September 2011
Author:robin
Status:resolved (Comments)
Tags:todo 
Comment:
* Mark underline css as !important. It should override other css (such as the css added in r91432) if the user specifically selected that option.
* Fix css flipping: for Simple, it still depended on the content language (however, no css actually needs flipping there). For Standard/CologneBlue, I re-added flipping even though it's not needed there either. Just in case css is added that does need flipping, so it is future-proof :). Consequently, I added noflip to the existing css.
* For this flipping I added a parameter to addInlineStyle(), which I assume to be useful in other cases as well
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderUserOptionsModule.php (modified) (history)
  • /trunk/phase3/skins/CologneBlue.php (modified) (history)
  • /trunk/phase3/skins/Simple.php (modified) (history)
  • /trunk/phase3/skins/Standard.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/Simple.php
——@@ -30,12 +30,12 @@
3131 $out->addModuleStyles( 'skins.simple' );
3232
3333 /* Add some userprefs specific CSS styling */
34 - global $wgUser, $wgContLang;
 34+ global $wgUser;
3535 $rules = array();
3636 $underline = "";
3737
3838 if ( $wgUser->getOption( 'underline' ) < 2 ) {
39 - $underline = "text-decoration: " . $wgUser->getOption( 'underline' ) ? 'underline' : 'none' . ";";
 39+ $underline = "text-decoration: " . $wgUser->getOption( 'underline' ) ? 'underline' : 'none' . " !important;";
4040 }
4141
4242 /* Also inherits from resourceloader */
——@@ -45,10 +45,7 @@
4646 $rules[] = "a.stub:after { $underline; }";
4747 }
4848 $style = implode( "\n", $rules );
49 - if ( $wgContLang->getDir() === 'rtl' ) {
50 - $style = CSSJanus::transform( $style, true, false );
51 - }
52 - $out->addInlineStyle( $style );
 49+ $out->addInlineStyle( $style, /* flip css if RTL */true );
5350
5451 }
5552 }
Index: trunk/phase3/skins/CologneBlue.php
——@@ -30,24 +30,24 @@
3131 $rules = array();
3232
3333 if ( 2 == $qb ) { # Right
34 - $rules[] = "#quickbar { position: absolute; right: 4px; }";
35 - $rules[] = "#article { margin-left: 4px; margin-right: 148px; }";
 34+ $rules[] = "/* @noflip */#quickbar { position: absolute; right: 4px; }";
 35+ $rules[] = "/* @noflip */#article { margin-left: 4px; margin-right: 148px; }";
3636 } elseif ( 1 == $qb ) {
37 - $rules[] = "#quickbar { position: absolute; left: 4px; }";
38 - $rules[] = "#article { margin-left: 148px; margin-right: 4px; }";
 37+ $rules[] = "/* @noflip */#quickbar { position: absolute; left: 4px; }";
 38+ $rules[] = "/* @noflip */#article { margin-left: 148px; margin-right: 4px; }";
3939 } elseif ( 3 == $qb ) { # Floating left
40 - $rules[] = "#quickbar { position:absolute; left:4px }";
41 - $rules[] = "#topbar { margin-left: 148px }";
42 - $rules[] = "#article { margin-left:148px; margin-right: 4px; }";
43 - $rules[] = "body>#quickbar { position:fixed; left:4px; top:4px; overflow:auto ;bottom:4px;}"; # Hides from IE
 40+ $rules[] = "/* @noflip */#quickbar { position:absolute; left:4px }";
 41+ $rules[] = "/* @noflip */#topbar { margin-left: 148px }";
 42+ $rules[] = "/* @noflip */#article { margin-left:148px; margin-right: 4px; }";
 43+ $rules[] = "/* @noflip */body>#quickbar { position:fixed; left:4px; top:4px; overflow:auto; bottom:4px;}"; # Hides from IE
4444 } elseif ( 4 == $qb ) { # Floating right
45 - $rules[] = "#quickbar { position: fixed; right: 4px; }";
46 - $rules[] = "#topbar { margin-right: 148px }";
47 - $rules[] = "#article { margin-right: 148px; margin-left: 4px; }";
48 - $rules[] = "body>#quickbar { position: fixed; right: 4px; top: 4px; overflow: auto ;bottom:4px;}"; # Hides from IE
 45+ $rules[] = "/* @noflip */#quickbar { position: fixed; right: 4px; }";
 46+ $rules[] = "/* @noflip */#topbar { margin-right: 148px }";
 47+ $rules[] = "/* @noflip */#article { margin-right: 148px; margin-left: 4px; }";
 48+ $rules[] = "/* @noflip */body>#quickbar { position: fixed; right: 4px; top: 4px; overflow: auto; bottom:4px;}"; # Hides from IE
4949 }
5050 $style = implode( "\n", $rules );
51 - $out->addInlineStyle( $style );
 51+ $out->addInlineStyle( $style, /* flip css if RTL */true );
5252 }
5353
5454 }
Index: trunk/phase3/skins/Standard.php
——@@ -29,21 +29,21 @@
3030 $rules = array();
3131
3232 if ( 2 == $qb ) { # Right
33 - $rules[] = "#quickbar { position: absolute; top: 4px; right: 4px; border-left: 2px solid #000000; }";
34 - $rules[] = "#article, #mw-data-after-content { margin-left: 4px; margin-right: 152px; }";
 33+ $rules[] = "/* @noflip */#quickbar { position: absolute; top: 4px; right: 4px; border-left: 2px solid #000000; }";
 34+ $rules[] = "/* @noflip */#article, /* @noflip */#mw-data-after-content { margin-left: 4px; margin-right: 152px; }";
3535 } elseif ( 1 == $qb || 3 == $qb ) {
36 - $rules[] = "#quickbar { position: absolute; top: 4px; left: 4px; border-right: 1px solid gray; }";
37 - $rules[] = "#article, #mw-data-after-content { margin-left: 152px; margin-right: 4px; }";
 36+ $rules[] = "/* @noflip */#quickbar { position: absolute; top: 4px; left: 4px; border-right: 1px solid gray; }";
 37+ $rules[] = "/* @noflip */#article, /* @noflip */#mw-data-after-content { margin-left: 152px; margin-right: 4px; }";
3838 if( 3 == $qb ) {
39 - $rules[] = "#quickbar { position: fixed; padding: 4px; }";
 39+ $rules[] = "/* @noflip */#quickbar { position: fixed; padding: 4px; }";
4040 }
4141 } elseif ( 4 == $qb ) {
42 - $rules[] = "#quickbar { position: fixed; right: 0px; top: 0px; padding: 4px;}";
43 - $rules[] = "#quickbar { border-right: 1px solid gray; }";
44 - $rules[] = "#article, #mw-data-after-content { margin-right: 152px; margin-left: 4px; }";
 42+ $rules[] = "/* @noflip */#quickbar { position: fixed; right: 0px; top: 0px; padding: 4px;}";
 43+ $rules[] = "/* @noflip */#quickbar { border-right: 1px solid gray; }";
 44+ $rules[] = "/* @noflip */#article, /* @noflip */#mw-data-after-content { margin-right: 152px; margin-left: 4px; }";
4545 }
4646 $style = implode( "\n", $rules );
47 - $out->addInlineStyle( $style );
 47+ $out->addInlineStyle( $style, /* flip css if RTL */true );
4848 }
4949
5050 }
Index: trunk/phase3/includes/OutputPage.php
——@@ -2949,8 +2949,13 @@
29502950 /**
29512951 * Adds inline CSS styles
29522952 * @param $style_css Mixed: inline CSS
 2953+ * @param $flip Boolean: Whether to flip the CSS if needed
29532954 */
2954 - public function addInlineStyle( $style_css ){
 2955+ public function addInlineStyle( $style_css, $flip = false ) {
 2956+ if( $flip && $this->getLang()->isRTL() ) {
 2957+ # If wanted, and the interface is right-to-left, flip the CSS
 2958+ $style_css = CSSJanus::transform( $style_css, true, false );
 2959+ }
29552960 $this->mInlineStyles .= Html::inlineStyle( $style_css );
29562961 }
29572962
Index: trunk/phase3/includes/resourceloader/ResourceLoaderUserOptionsModule.php
——@@ -97,7 +97,7 @@
9898 // Underline: 2 = browser default, 1 = always, 0 = never
9999 if ( $options['underline'] < 2 ) {
100100 $rules[] = "a { text-decoration: " .
101 - ( $options['underline'] ? 'underline' : 'none' ) . "; }";
 101+ ( $options['underline'] ? 'underline' : 'none' ) . " !important; }";
102102 }
103103 if ( $options['highlightbroken'] ) {
104104 $rules[] = "a.new, #quickbar a.new { color: #ba0000; }\n";

Follow-up revisions

RevisionCommit summaryAuthorDate
r96263Fix r96261: don't add important if 'none', so it doesn't break underlining on...robin22:05, 4 September 2011
r96760* Per Aaron on r96261, use string 'flip' instead of boolean...robin23:51, 10 September 2011
r96842REL1_18: MFT r93288, r94212, r96261, r96263, r96384reedy14:42, 12 September 2011
r101445Follow-up r96261: remove the !important again, and then do the script default...robin00:17, 1 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r91432(see bug 29712) Removing the defaultUserOptionOverrides in MessagesXx.php bec...robin01:26, 5 July 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   22:57, 7 September 2011

It would be nice if the $flip param to addInlineStyle() took a string instead of an opaque boolean.

#Comment by SPQRobin (talk | contribs)   13:42, 8 September 2011

What would that string be set to? Would it contain the CSS to be flipped?

#Comment by Aaron Schulz (talk | contribs)   17:21, 8 September 2011

No, I mean something like checking if $flip === 'flip' or something.

#Comment by SPQRobin (talk | contribs)   23:51, 10 September 2011

Done in r96760

#Comment by TheDJ (talk | contribs)   23:09, 15 October 2011

"Mark underline css as !important. It should override other css"


This broke the "nounderlines" css styling on english wikipedia, which was used to remove underlines from certain glyphs with diacritics etc.

#Comment by Platonides (talk | contribs)   20:34, 27 October 2011

It also made tabs and p-personal underlined (when the given preference is given), which they shouldn't be.

#Comment by SPQRobin (talk | contribs)   00:20, 1 November 2011

I checked 1.17 and tabs and personal links were also underlined there. Anyway, the custom en.wiki styling should work with r101445.

#Comment by Platonides (talk | contribs)   22:36, 1 November 2011

I see such difference, unless caching is playing tricks on me, but indeed looks fixed now (toolboxes are still underlined, but that doesn't seem an issue).

Status & tagging log