r69336 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69335‎ | r69336 | r69337 >
Date:18:31, 14 July 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
By flipping '"Courier New", monospace' to 'monospace, "Courier New"', we can get the beneifts of allowing the user's monospace font override to still be used. Improves on r69335 which is itself an improvement on r65391.
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/skins/vector/main-ltr.css (modified) (history)
  • /trunk/phase3/skins/vector/main-rtl.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/vector/main-ltr.css
——@@ -725,7 +725,7 @@
726726 * It's important for this rule to first reference an actual font name, some browsers will render the monospace text
727727 * too small otherwise, namely Firefox, Chrome and Safari
728728 */
729 - font-family: "Courier New", monospace;
 729+ font-family: monospace, "Courier New";
730730 }
731731 code {
732732 background-color: #f9f9f9;
Index: trunk/phase3/skins/vector/main-rtl.css
——@@ -725,7 +725,7 @@
726726 * It's important for this rule to first reference an actual font name, some browsers will render the monospace text
727727 * too small otherwise, namely Firefox, Chrome and Safari
728728 */
729 - font-family: "Courier New", monospace;
 729+ font-family: monospace, "Courier New";
730730 }
731731 code {
732732 background-color: #f9f9f9;
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -1547,7 +1547,7 @@
15481548 * to ensure that client-side caches do not keep obsolete copies of global
15491549 * styles.
15501550 */
1551 -$wgStyleVersion = '296';
 1551+$wgStyleVersion = '297';
15521552
15531553 /**
15541554 * This will cache static pages for non-logged-in users to reduce

Follow-up revisions

RevisionCommit summaryAuthorDate
r697291.16wmf4: Merge Vector fixes from trunk: r65063, r68798, r69335, r69336, r693...catrope14:09, 22 July 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r65391Actually fix bug #20706 - Next time I will read more carefully and test more ...tparscal14:19, 21 April 2010
r69335Improves on the technique used in r65391 to fix the font-size issues in Firef...tparscal17:50, 14 July 2010

Comments

#Comment by Entlinkt (talk | contribs)   21:25, 14 July 2010

As reported in r65391, monospace, sans-serif somehow managed to trigger sans-serif, so I wonder whether this could then trigger Courier New unexpectedly. Maybe it would be safer to use a fake string instead, as suggested on http://virtuelvis.com/archives/2005/02/monospace-firefox-braindeath.

#Comment by Entlinkt (talk | contribs)   22:17, 14 July 2010

FYI, http://www.brunildo.org/test/monospace_fsize.html suggests that Firefox has the tiny fonts if monospace is the only alternative, while [old versions of] Safari (current versions seem to behave like Firefox) have them if monospace is the last alternative. That's why the original font-family: monospace, sans-serif; from r65391 worked and why this should work as well, but font-family: monospace, "Placeholder to get normal-sized fonts"; could be safer.

#Comment by Trevor Parscal (WMF) (talk | contribs)   00:01, 15 July 2010

Until the person who reported their browser falling back from "monospace" installs a font called "Placeholder to get normal-sized fonts". :)

#Comment by MZMcBride (talk | contribs)   23:04, 17 July 2010 
 	 * It's important for this rule to first reference an actual font name, some browsers will render the monospace text
 	 * too small otherwise, namely Firefox, Chrome and Safari

Is this comment still accurate?

#Comment by Entlinkt (talk | contribs)   04:50, 18 July 2010

No. And my comment above isn't entirely correct either. http://code.google.com/p/chromium/issues/detail?id=10524 and https://bugs.webkit.org/show_bug.cgi?id=19161 get it right.

  • Firefox uses the small setting only if monospace is the only alternative.
  • WebKit formerly used the small setting if monospace is the last generic family. A specific font after the generic family will not prevent old WebKit versions from using the small setting. Another generic family would, but can have side-effects, cf. r65391.
  • WebKit switched to Firefox' behaviour in August 2009.

Therefore, this will not prevent tiny fonts in Safari 4.0, but in Safari 4.0.5. WebKit users update fast (as do Opera users), so this may not be a problem. I'd suggest to update the comment, replace Courier New with something fabricated (for extra safety and because it's not useful anyway), and include kbd and samp elements (https://bugzilla.wikimedia.org/show_bug.cgi?id=20706#c11).

#Comment by Od1n (talk | contribs)   03:07, 9 January 2012

Since this commit, the CSS comments don't match the actual code. They should be changed to something like this: 

It's important for this rule to reference a font name in addition to monospace (...)

Status & tagging log