r113214 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r113213‎ | r113214 | r113215 >
Date:02:58, 7 March 2012
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
[mw.util.tooltipAccessKeyPrefix] alt-shift for Chrome on Windows
* Fixes:
-- (bug 29753) mw.util.tooltipAccessKeyPrefix should be alt-shift for Chrome on Windows
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.util.js (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -258,6 +258,7 @@
259259 * (bug 34600) Older skins using useHeadElement=false were broken in 1.18.
260260 * (bug 34604) [mw.config] wgActionPaths should be an object instead of a numeral
261261 array.
 262+* (bug 29753) mw.util.tooltipAccessKeyPrefix should be alt-shift for Chrome on Windows
262263
263264 === API changes in 1.19 ===
264265 * Made action=edit less likely to return "unknownerror", by returning the actual error
Index: trunk/phase3/resources/mediawiki/mediawiki.util.js
——@@ -29,9 +29,17 @@
3030
3131 // Chrome on any platform
3232 } else if ( profile.name === 'chrome' ) {
33 - // Chrome on Mac or Chrome on other platform ?
34 - util.tooltipAccessKeyPrefix = ( profile.platform === 'mac'
35 - ? 'ctrl-option-' : 'alt-' );
 33+
 34+ util.tooltipAccessKeyPrefix = (
 35+ // Chrome on Mac
 36+ profile.platform === 'mac' ? 'ctrl-option-' :
 37+ // Chrome on Windows
 38+ // (both alt- and alt-shift work, but alt-f triggers Chrome wrench menu
 39+ // which alt-shift-f does not)
 40+ profile.platform === 'win' ? 'alt-shift-' :
 41+ // Chrome on Ubuntu (and other?)
 42+ 'alt-'
 43+ );
3644
3745 // Non-Windows Safari with webkit_version > 526
3846 } else if ( profile.platform !== 'win'

Follow-up revisions

RevisionCommit summaryAuthorDate
r113268Follows-up r113214: span ternary operator over multiple lines for readabilitykrinkle19:00, 7 March 2012
r113935MFT r112918, r113214, r113394, r113415, r113617, r113710, r113727, r113737, r...reedy17:27, 15 March 2012
r114015MFT r112918, r113214, r113268, r113277, r113312, r113415, r113454, r113737, r...reedy15:18, 16 March 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r91652Partial (?) fix for bug 29753 -- wrong Firefox version comparison for non-Mac...brion17:17, 7 July 2011

Comments

#Comment by Reedy (talk | contribs)   15:11, 7 March 2012

The nested ternary look awful, it's not obvious that the else for the mac one is the windows one..

#Comment by Krinkle (talk | contribs)   18:18, 7 March 2012

Hm.. it's something I picked up from diving into jQuery. It is weird but i'm not sure it's awful. Some people prefer it to be written as a tree. So instead of 

		util.tooltipAccessKeyPrefix = (
			// Chrome on Mac
			profile.platform === 'mac' ? 'ctrl-option-' :
			// Chrome on Windows
			// (both alt- and alt-shift work, but alt-f triggers Chrome wrench menu
			// which alt-shift-f does not)
			profile.platform === 'win' ? 'alt-shift-' :
			// Chrome on Ubuntu (and other?)
			'alt-'
		);

it would be: 

		util.tooltipAccessKeyPrefix = (
			profile.platform === 'mac'
				// Chrome on Mac
				? 'ctrl-option-'
				: profile.platform === 'win'
					// Chrome on Windows
					// (both alt- and alt-shift work, but alt-f triggers Chrome wrench menu
					// which alt-shift-f does not)
					? 'alt-shift-'
					// Chrome on other (Ubuntu?)
					: 'alt-'
		);

Is that better?

#Comment by Reedy (talk | contribs)   18:54, 7 March 2012

Yeah, the extra indenting makes it clearer to read

Status & tagging log