mediawiki.util: Fix roundtripping of tooltip in portlet links

In 558985f72a ctrl-option- for Chrome on Mac was added, but it
didn't add "option-" to the regex. Since then the tooltip in
Chrome on Mac (and later when we started referring to option
instead of alt on all Mac browsers) was never detected by this
regex, sometimes resulting in double tooltips (when adding) or
outdated tooltips (when updating).

Now we always look for an accesskey hint in the tooltip and
strip it it's there.

Also fixed a bug where an undefined error can occur if accesskey
is given but tooltip not (the method unconditinally appended
text to the tooltip varible which might be undefined causing
a tooltip like "undefined[a]").

Change-Id: I0bde1a228983c58b20cad0c09a8e5efe8225ea23

diff --git a/RELEASE-NOTES-1.22 b/RELEASE-NOTES-1.22
index cf31e51..d3ea77e 100644 (file)
--- a/RELEASE-NOTES-1.22
+++ b/RELEASE-NOTES-1.22
@@ -35,6 +35,10 @@ production.
   Pager-related WebRequest parameters by default, as this is overwhelmingly
   likely to be what was intended by users of the method. If any caller wishes
   to use these parameters, the new param 'useRequestParams' may be set to true.
+* mw.util.addPortletLink: Tooltip is no longer required to be plain (without
+  an accesskey in it already). As such it now rountrips. Creating a link with a
+  message as tooltip, grabbing the title attribute and using it to create
+  another portlet will work as expected.
 
 === Bug fixes in 1.22 ===
 * Disable Special:PasswordReset when $wgEnableEmail. Previously one could still
@@ -48,6 +52,8 @@ production.
 * (bug 47218) Special:BlockList now handles correctly user names with spaces
   when passed as subpage.
 * Pager's properly validate which fields are allowed to be sorted on.
+* mw.util.tooltipAccessKeyRegexp: The regex now matches "option-" as well.
+  Support for Mac "option" was added in 1.16, but the regex was never updated.
 
 === API changes in 1.22 ===
 * (bug 46626) xmldoublequote parameter was removed. Because of a bug, the
@@ -85,6 +91,8 @@ changes to languages because of Bugzilla reports.
 * Calling Linker methods using a skin will now output deprecation warnings.
 * (bug 46680) "Return to" links are no longer tagged with rel="next".
 * The Special:ActiveUsers special page was removed.
+* BREAKING CHANGE: mw.util.tooltipAccessKeyRegexp: The match group for the
+  accesskey character is now $6 instead of $5.
 
 == Compatibility ==
 

diff --git a/resources/mediawiki/mediawiki.util.js b/resources/mediawiki/mediawiki.util.js
index 1bd7430..60ef758 100644 (file)
--- a/resources/mediawiki/mediawiki.util.js
+++ b/resources/mediawiki/mediawiki.util.js
@@ -279,8 +279,17 @@
                /**
                 * @property {RegExp}
                 * Regex to match accesskey tooltips.
+                *
+                * Should match:
+                *
+                * - "ctrl-option-"
+                * - "alt-shift-"
+                * - "ctrl-alt-"
+                * - "ctrl-"
+                *
+                * The accesskey is matched in group $6.
                 */
-               tooltipAccessKeyRegexp: /\[(ctrl-)?(alt-)?(shift-)?(esc-)?(.)\]$/,
+               tooltipAccessKeyRegexp: /\[(ctrl-)?(option-)?(alt-)?(shift-)?(esc-)?(.)\]$/,
 
                /**
                 * Add the appropriate prefix to the accesskey shown in the tooltip.
@@ -301,9 +310,9 @@
                        }
 
                        $nodes.attr( 'title', function ( i, val ) {
-                               if ( val && util.tooltipAccessKeyRegexp.exec( val ) ) {
+                               if ( val && util.tooltipAccessKeyRegexp.test( val ) ) {
                                        return val.replace( util.tooltipAccessKeyRegexp,
-                                               '[' + util.tooltipAccessKeyPrefix + '$5]' );
+                                               '[' + util.tooltipAccessKeyPrefix + '$6]' );
                                }
                                return val;
                        } );
@@ -406,19 +415,27 @@
                        if ( id ) {
                                $item.attr( 'id', id );
                        }
-                       if ( accesskey ) {
-                               $link.attr( 'accesskey', accesskey );
-                               tooltip += ' [' + accesskey + ']';
+
+                       if ( tooltip ) {
+                               // Trim any existing accesskey hint and the trailing space
+                               tooltip = $.trim( tooltip.replace( util.tooltipAccessKeyRegexp, '' ) );
+                               if ( accesskey ) {
+                                       tooltip += ' [' + accesskey + ']';
+                               }
                                $link.attr( 'title', tooltip );
+                               if ( accesskey ) {
+                                       util.updateTooltipAccessKeys( $link );
+                               }
                        }
-                       if ( accesskey && tooltip ) {
-                               util.updateTooltipAccessKeys( $link );
+
+                       if ( accesskey ) {
+                               $link.attr( 'accesskey', accesskey );
                        }
 
                        // Where to put our node ?
                        // - nextnode is a DOM element (was the only option before MW 1.17, in wikibits.js)
                        if ( nextnode && nextnode.parentNode === $ul[0] ) {
-                               $(nextnode).before( $item );
+                               $( nextnode ).before( $item );
 
                        // - nextnode is a CSS selector for jQuery
                        } else if ( typeof nextnode === 'string' && $ul.find( nextnode ).length !== 0 ) {

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
index bba3160..713ec4b 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
@@ -1,5 +1,13 @@
 ( function ( mw, $ ) {
-       QUnit.module( 'mediawiki.util', QUnit.newMwEnvironment() );
+       QUnit.module( 'mediawiki.util', QUnit.newMwEnvironment( {
+               setup: function () {
+                       this.taPrefix = mw.util.tooltipAccessKeyPrefix;
+                       mw.util.tooltipAccessKeyPrefix = 'ctrl-alt-';
+               },
+               teardown: function () {
+                       mw.util.tooltipAccessKeyPrefix = this.taPrefix;
+               }
+       } ) );
 
        QUnit.test( 'rawurlencode', 1, function ( assert ) {
                assert.equal( mw.util.rawurlencode( 'Test:A & B/Here' ), 'Test%3AA%20%26%20B%2FHere' );
@@ -108,10 +116,14 @@
                assert.strictEqual( mw.util.getParamValue( 'TEST', url ), 'a b+c d', 'Bug 30441: getParamValue must understand "+" encoding of space (multiple spaces)' );
        } );
 
-       QUnit.test( 'tooltipAccessKey', 3, function ( assert ) {
-               assert.equal( typeof mw.util.tooltipAccessKeyPrefix, 'string', 'mw.util.tooltipAccessKeyPrefix must be a string' );
-               assert.ok( mw.util.tooltipAccessKeyRegexp instanceof RegExp, 'mw.util.tooltipAccessKeyRegexp instance of RegExp' );
-               assert.ok( mw.util.updateTooltipAccessKeys, 'mw.util.updateTooltipAccessKeys' );
+       QUnit.test( 'tooltipAccessKey', 4, function ( assert ) {
+               assert.equal( typeof mw.util.tooltipAccessKeyPrefix, 'string', 'tooltipAccessKeyPrefix must be a string' );
+               assert.equal( $.type( mw.util.tooltipAccessKeyRegexp ), 'regexp', 'tooltipAccessKeyRegexp is a regexp' );
+               assert.ok( mw.util.updateTooltipAccessKeys, 'updateTooltipAccessKeys is non-empty' );
+
+               'Example [a]'.replace( mw.util.tooltipAccessKeyRegexp, function ( sub, m1, m2, m3, m4, m5, m6 ) {
+                       assert.equal( m6, 'a', 'tooltipAccessKeyRegexp finds the accesskey hint' );
+               } );
        } );
 
        QUnit.test( '$content', 2, function ( assert ) {
@@ -125,7 +137,7 @@
         * Previously, test elements where invisible to the selector since only
         * one element can have a given id.
         */
-       QUnit.test( 'addPortletLink', 8, function ( assert ) {
+       QUnit.test( 'addPortletLink', 10, function ( assert ) {
                var pTestTb, pCustom, vectorTabs, tbRL, cuQuux, $cuQuux, tbMW, $tbMW, tbRLDM, caFoo;
 
                pTestTb = '\
@@ -154,7 +166,8 @@
                $( '#qunit-fixture' ).append( pTestTb, pCustom, vectorTabs );
 
                tbRL = mw.util.addPortletLink( 'p-test-tb', '//mediawiki.org/wiki/ResourceLoader',
-                       'ResourceLoader', 't-rl', 'More info about ResourceLoader on MediaWiki.org ', 'l' );
+                       'ResourceLoader', 't-rl', 'More info about ResourceLoader on MediaWiki.org ', 'l'
+               );
 
                assert.ok( $.isDomElement( tbRL ), 'addPortletLink returns a valid DOM Element according to $.isDomElement' );
 
@@ -162,14 +175,32 @@
                        'MediaWiki.org', 't-mworg', 'Go to MediaWiki.org ', 'm', tbRL );
                $tbMW = $( tbMW );
 
+               assert.propEqual(
+                       $tbMW.getAttrs(),
+                       {
+                               id: 't-mworg'
+                       },
+                       'Validate attributes of created element'
+               );
+
+               assert.propEqual(
+                       $tbMW.find( 'a' ).getAttrs(),
+                       {
+                               href: '//mediawiki.org/',
+                               title: 'Go to MediaWiki.org [ctrl-alt-m]',
+                               accesskey: 'm'
+                       },
+                       'Validate attributes of anchor tag in created element'
+               );
 
-               assert.equal( $tbMW.attr( 'id' ), 't-mworg', 'Link has correct ID set' );
                assert.equal( $tbMW.closest( '.portlet' ).attr( 'id' ), 'p-test-tb', 'Link was inserted within correct portlet' );
                assert.equal( $tbMW.next().attr( 'id' ), 't-rl', 'Link is in the correct position (by passing nextnode)' );
 
-               cuQuux = mw.util.addPortletLink( 'p-test-custom', '#', 'Quux' );
+               cuQuux = mw.util.addPortletLink( 'p-test-custom', '#', 'Quux', null, 'Example [shift-x]', 'q' );
                $cuQuux = $( cuQuux );
 
+               assert.equal( $cuQuux.find( 'a' ).attr( 'title' ), 'Example [ctrl-alt-q]', 'Existing accesskey is stripped and updated' );
+
                assert.equal(
                        $( '#p-test-custom #c-barmenu ul li' ).length,
                        1,