From 6692fe7d2804af626ebc136760a6c221f91c98f7 Mon Sep 17 00:00:00 2001 From: ChiaYen Kan Date: Thu, 15 Aug 2019 10:54:54 +0800 Subject: [PATCH] Support access keys in extension menus base on following bugs: Bug 1320462 - Support access keys in extension menus. r=mixedpuppy Bug 1484914 - Never append duplicate access key hints r=bzbarsky --- browser/components/extensions/ext-menus.js | 16 ++ .../test/browser/browser-common.ini | 1 + .../browser/browser_ext_menus_accesskey.js | 183 ++++++++++++++++++ layout/xul/nsTextBoxFrame.cpp | 7 +- .../passwordmgr/nsLoginManagerPrompter.js | 2 +- 5 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 browser/components/extensions/test/browser/browser_ext_menus_accesskey.js diff --git a/browser/components/extensions/ext-menus.js b/browser/components/extensions/ext-menus.js index 0e70d0c778fd9..1c71c13f35401 100644 --- a/browser/components/extensions/ext-menus.js +++ b/browser/components/extensions/ext-menus.js @@ -173,6 +173,22 @@ var gMenuBuilder = { customizeElement(element, item, contextData) { let label = item.title; if (label) { + let accessKey; + label = label.replace(/&([\S\s]|$)/g, (_, nextChar, i) => { + if (nextChar === "&") { + return "&"; + } + if (accessKey === undefined) { + if (nextChar === "%" && label.charAt(i + 2) === "s") { + accessKey = ""; + } else { + accessKey = nextChar; + } + } + return nextChar; + }); + element.setAttribute("accesskey", accessKey || ""); + if (contextData.isTextSelected && label.indexOf("%s") > -1) { let selection = contextData.selectionText.trim(); // The rendering engine will truncate the title if it's longer than 64 characters. diff --git a/browser/components/extensions/test/browser/browser-common.ini b/browser/components/extensions/test/browser/browser-common.ini index 50075b7dc933d..0ee07f4c3a342 100644 --- a/browser/components/extensions/test/browser/browser-common.ini +++ b/browser/components/extensions/test/browser/browser-common.ini @@ -79,6 +79,7 @@ skip-if = (os == 'win' && !debug) # bug 1352668 [browser_ext_incognito_popup.js] [browser_ext_lastError.js] [browser_ext_menus.js] +[browser_ext_menus_accesskey.js] [browser_ext_omnibox.js] skip-if = debug || asan # Bug 1354681 [browser_ext_optionsPage_browser_style.js] diff --git a/browser/components/extensions/test/browser/browser_ext_menus_accesskey.js b/browser/components/extensions/test/browser/browser_ext_menus_accesskey.js new file mode 100644 index 0000000000000..fd242c1b23e8e --- /dev/null +++ b/browser/components/extensions/test/browser/browser_ext_menus_accesskey.js @@ -0,0 +1,183 @@ +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ +/* vim: set sts=2 sw=2 et tw=80: */ +"use strict"; + +const PAGE = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html"; + +add_task(async function accesskeys() { + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE); + gBrowser.selectedTab = tab; + + async function background() { + // description is informative. + // title is passed to menus.create. + // label and key are compared with the actual values. + const TESTCASES = [{ + description: "Amp at start", + title: "&accesskey", + label: "accesskey", + key: "a", + }, { + description: "amp in between", + title: "A& b", + label: "A b", + key: " ", + }, { + description: "lonely amp", + title: "&", + label: "", + key: "", + }, { + description: "amp at end", + title: "End &", + label: "End ", + key: "", + }, { + description: "escaped amp", + title: "A && B", + label: "A & B", + key: "", + }, { + description: "amp before escaped amp", + title: "A &T&& before", + label: "A T& before", + key: "T", + }, { + description: "amp after escaped amp", + title: "A &&&T after", + label: "A &T after", + key: "T", + }, { + // Only the first amp should be used as the access key. + description: "amp, escaped amp, amp to ignore", + title: "First &1 comes && first &2 serves", + label: "First 1 comes & first 2 serves", + key: "1", + }, { + description: "created with amp, updated without amp", + title: "temp with &X", // will be updated below. + label: "remove amp", + key: "", + }, { + description: "created without amp, update with amp", + title: "temp without access key", // will be updated below. + label: "add ampY", + key: "Y", + }]; + + let menuIds = TESTCASES.map(({title}) => browser.menus.create({title})); + + // Should clear the access key: + await browser.menus.update(menuIds[menuIds.length - 2], {title: "remove amp"}); + + // Should add an access key: + await browser.menus.update(menuIds[menuIds.length - 1], {title: "add amp&Y"}); + // Should not clear the access key because title is not set: + await browser.menus.update(menuIds[menuIds.length - 1], {enabled: true}); + + browser.test.sendMessage("testCases", TESTCASES); + } + let extension = ExtensionTestUtils.loadExtension({ + manifest: { + permissions: ["menus"], + }, + background, + }); + + await extension.startup(); + + const TESTCASES = await extension.awaitMessage("testCases"); + let menu = await openExtensionContextMenu(); + let items = menu.getElementsByTagName("menuitem"); + is(items.length, TESTCASES.length, "Expected menu items for page"); + TESTCASES.forEach(({description, label, key}, i) => { + is(items[i].label, label, `Label for item ${i} (${description})`); + is(items[i].accessKey, key, `Accesskey for item ${i} (${description})`); + }); + + await closeExtensionContextMenu(); + await extension.unload(); + BrowserTestUtils.removeTab(tab); +}); + +add_task(async function accesskeys_selection() { + const PAGE_WITH_AMPS = "data:text/plain;charset=utf-8,PageSelection&Amp"; + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE_WITH_AMPS); + gBrowser.selectedTab = tab; + + async function background() { + const TESTCASES = [{ + description: "Selection without amp", + title: "percent-s: %s.", + label: "percent-s: PageSelection&Amp.", + key: "", + }, { + description: "Selection with amp after %s", + title: "percent-s: %s &A.", + label: "percent-s: PageSelection&Amp A.", + key: "A", + }, { + description: "Selection with amp before %s", + title: "percent-s: &B %s.", + label: "percent-s: B PageSelection&Amp.", + key: "B", + }, { + description: "Amp-percent", + title: "Amp-percent: &%.", + label: "Amp-percent: %.", + key: "%", + }, { + // "&%s" should be treated as "%s", and "ignore this" with amps should be ignored. + description: "Selection with amp-percent-s", + title: "Amp-percent-s: &%s.&i&g&n&o&r&e& &t&h&i&s", + label: "Amp-percent-s: PageSelection&Amp.ignore this", + // Chrome uses the first character of the selection as access key. + // Let's not copy that behavior... + key: "", + }, { + description: "Selection with amp before amp-percent-s", + title: "Amp-percent-s: &_ &%s.", + label: "Amp-percent-s: _ PageSelection&Amp.", + key: "_", + }]; + + let lastMenuId; + for (let {title} of TESTCASES) { + lastMenuId = browser.menus.create({contexts: ["selection"], title}); + } + // Round-trip to ensure that the menus have been registered. + await browser.menus.update(lastMenuId, {}); + browser.test.sendMessage("testCases", TESTCASES); + } + let extension = ExtensionTestUtils.loadExtension({ + manifest: { + permissions: ["menus"], + }, + background, + }); + + await extension.startup(); + + // Select all + await ContentTask.spawn(gBrowser.selectedBrowser, { }, function* (arg) { + let doc = content.document; + let range = doc.createRange(); + let selection = content.getSelection(); + selection.removeAllRanges(); + range.selectNodeContents(doc.body); + selection.addRange(range); + }); + + const TESTCASES = await extension.awaitMessage("testCases"); + let menu = await openExtensionContextMenu(); + let items = menu.getElementsByTagName("menuitem"); + is(items.length, TESTCASES.length, "Expected menu items for page"); + TESTCASES.forEach(({description, label, key}, i) => { + is(items[i].label, label, `Label for item ${i} (${description})`); + is(items[i].accessKey, key, `Accesskey for item ${i} (${description})`); + }); + + await closeExtensionContextMenu(); + await extension.unload(); + BrowserTestUtils.removeTab(tab); +}); diff --git a/layout/xul/nsTextBoxFrame.cpp b/layout/xul/nsTextBoxFrame.cpp index 9b05cc250b5c2..5683c9d278669 100644 --- a/layout/xul/nsTextBoxFrame.cpp +++ b/layout/xul/nsTextBoxFrame.cpp @@ -845,7 +845,7 @@ nsTextBoxFrame::UpdateAccessTitle() /* * Note that if you change appending access key label spec, * you need to maintain same logic in following methods. See bug 324159. - * toolkit/content/commonDialog.js (setLabelForNode) + * toolkit/components/prompts/src/CommonDialog.jsm (setLabelForNode) * toolkit/content/widgets/text.xml (formatAccessKey) */ int32_t menuAccessKey; @@ -868,6 +868,11 @@ nsTextBoxFrame::UpdateAccessTitle() return; } + if (StringEndsWith(mTitle, accessKeyLabel)) { + // Never append another "(X)" if the title already ends with "(X)". + return; + } + const nsDependentString& kEllipsis = nsContentUtils::GetLocalizedEllipsis(); uint32_t offset = mTitle.Length(); if (StringEndsWith(mTitle, kEllipsis)) { diff --git a/toolkit/components/passwordmgr/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/nsLoginManagerPrompter.js index 8ed4e58f19c14..d0344f55e6437 100644 --- a/toolkit/components/passwordmgr/nsLoginManagerPrompter.js +++ b/toolkit/components/passwordmgr/nsLoginManagerPrompter.js @@ -1076,7 +1076,7 @@ LoginManagerPrompter.prototype = { // Ugh. We can't use the strings from the popup window, because they // have the access key marked in the string (eg "Mo&zilla"), along // with some weird rules for handling access keys that do not occur - // in the string, for L10N. See commonDialog.js's setLabelForNode(). + // in the string, for L10N. See CommonDialog.jsm's setLabelForNode(). var neverButtonText = this._getLocalizedString("notifyBarNeverRememberButtonText2"); var neverButtonAccessKey =