Closed
Bug 1473720
Opened 6 years ago
Closed 6 years ago
menus.onShown fails when context is a bookmark
Categories
(WebExtensions :: Frontend, defect)
WebExtensions
Frontend
Tracking
(firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)
RESOLVED
FIXED
mozilla63
People
(Reporter: pts+bmo, Assigned: pts+bmo)
References
Details
Attachments
(2 files)
459 bytes,
application/x-xpinstall
|
Details | |
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
STR: Install the attached testcase extension, which just sets a menus.onShown listener that prints to the console. Right-click on tabs or in the content area; the extension prints to the console as expected. Right-click on a bookmark (in the bookmarks menu or toolbar). The extension code doesn't get to run; instead an error is printed to the console.
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items https://reviewboard.mozilla.org/r/255116/#review261980 Code analysis found 18 defects in this patch: - 18 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/extensions/test/browser/browser_ext_menus.js:161 (Diff revision 1) > BrowserTestUtils.removeTab(tab); > await extension.unload(); > }); > > +add_task(async function test_bookmarkContextMenu() { > + async function showBookmarksToolbar(visible=true) { Error: Infix operators must be spaced. [eslint: space-infix-ops] ::: browser/components/extensions/test/browser/browser_ext_menus.js:162 (Diff revision 1) > await extension.unload(); > }); > > +add_task(async function test_bookmarkContextMenu() { > + async function showBookmarksToolbar(visible=true) { > + let bt = document.getElementById('PersonalToolbar'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:164 (Diff revision 1) > > +add_task(async function test_bookmarkContextMenu() { > + async function showBookmarksToolbar(visible=true) { > + let bt = document.getElementById('PersonalToolbar'); > + let transitionPromise = > + BrowserTestUtils.waitForEvent(bt, 'transitionend', Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:165 (Diff revision 1) > +add_task(async function test_bookmarkContextMenu() { > + async function showBookmarksToolbar(visible=true) { > + let bt = document.getElementById('PersonalToolbar'); > + let transitionPromise = > + BrowserTestUtils.waitForEvent(bt, 'transitionend', > + e => e.propertyName == 'max-height'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:172 (Diff revision 1) > + await transitionPromise; > + } > + > + const ext = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ['menus', 'bookmarks'], Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:172 (Diff revision 1) > + await transitionPromise; > + } > + > + const ext = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ['menus', 'bookmarks'], Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:175 (Diff revision 1) > + const ext = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ['menus', 'bookmarks'], > + }, > + async background() { > + await browser.menus.create({title: 'blarg', contexts: ['bookmark']}); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:175 (Diff revision 1) > + const ext = ExtensionTestUtils.loadExtension({ > + manifest: { > + permissions: ['menus', 'bookmarks'], > + }, > + async background() { > + await browser.menus.create({title: 'blarg', contexts: ['bookmark']}); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:177 (Diff revision 1) > + permissions: ['menus', 'bookmarks'], > + }, > + async background() { > + await browser.menus.create({title: 'blarg', contexts: ['bookmark']}); > + browser.menus.onShown.addListener(() => { > + browser.test.sendMessage('hello'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:179 (Diff revision 1) > + async background() { > + await browser.menus.create({title: 'blarg', contexts: ['bookmark']}); > + browser.menus.onShown.addListener(() => { > + browser.test.sendMessage('hello'); > + }); > + browser.test.sendMessage('ready'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:185 (Diff revision 1) > + }, > + }); > + > + await showBookmarksToolbar(); > + await ext.startup(); > + await ext.awaitMessage('ready'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:187 (Diff revision 1) > + > + await showBookmarksToolbar(); > + await ext.startup(); > + await ext.awaitMessage('ready'); > + > + let menu = await openChromeContextMenu('placesContext', Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:188 (Diff revision 1) > + await showBookmarksToolbar(); > + await ext.startup(); > + await ext.awaitMessage('ready'); > + > + let menu = await openChromeContextMenu('placesContext', > + '#PlacesToolbarItems .bookmark-item'); Error: Expected indentation of 41 spaces but found 6. [eslint: indent] ::: browser/components/extensions/test/browser/browser_ext_menus.js:188 (Diff revision 1) > + await showBookmarksToolbar(); > + await ext.startup(); > + await ext.awaitMessage('ready'); > + > + let menu = await openChromeContextMenu('placesContext', > + '#PlacesToolbarItems .bookmark-item'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:191 (Diff revision 1) > + > + let menu = await openChromeContextMenu('placesContext', > + '#PlacesToolbarItems .bookmark-item'); > + let children = Array.from(menu.children); > + let item = children[children.length - 1]; > + is(item.label, 'blarg', 'Menu item label is correct'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:191 (Diff revision 1) > + > + let menu = await openChromeContextMenu('placesContext', > + '#PlacesToolbarItems .bookmark-item'); > + let children = Array.from(menu.children); > + let item = children[children.length - 1]; > + is(item.label, 'blarg', 'Menu item label is correct'); Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:192 (Diff revision 1) > + let menu = await openChromeContextMenu('placesContext', > + '#PlacesToolbarItems .bookmark-item'); > + let children = Array.from(menu.children); > + let item = children[children.length - 1]; > + is(item.label, 'blarg', 'Menu item label is correct'); > + await ext.awaitMessage('hello'); // onShown listener fired Error: Strings must use doublequote. [eslint: quotes] ::: browser/components/extensions/test/browser/browser_ext_menus.js:194 (Diff revision 1) > + let children = Array.from(menu.children); > + let item = children[children.length - 1]; > + is(item.label, 'blarg', 'Menu item label is correct'); > + await ext.awaitMessage('hello'); // onShown listener fired > + > + closeChromeContextMenu('placesContext', item); Error: Strings must use doublequote. [eslint: quotes]
Assignee | ||
Comment 3•6 years ago
|
||
Whoops, meant to fix my quotes but forgot. I'm not sure if the "infix operators" lint is intentional though, since a default argument is not really an operator... leaving as is because that's how I'm used to seeing default arguments and I don't see any nearby to follow local style. Please let me know if spaces are really preferred.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8990116 -
Flags: review?(mixedpuppy)
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items https://reviewboard.mozilla.org/r/255116/#review262004 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: browser/components/extensions/test/browser/browser_ext_menus.js:188 (Diff revision 2) > + await showBookmarksToolbar(); > + await ext.startup(); > + await ext.awaitMessage("ready"); > + > + let menu = await openChromeContextMenu("placesContext", > + "#PlacesToolbarItems .bookmark-item"); Error: Expected indentation of 41 spaces but found 6. [eslint: indent]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items I didn't mean to cancel the review request. Not sure what happened.
Attachment #8990116 -
Flags: review?(mixedpuppy)
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items https://reviewboard.mozilla.org/r/255116/#review262180 nice. I kicked off a try run, land given it passes. As this is a simple fix for broken behavior, I think it's a good candidate for uplift.
Attachment #8990116 -
Flags: review?(mixedpuppy) → review+
Updated•6 years ago
|
Assignee: nobody → pts+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: qe-verify-
Assignee | ||
Comment 9•6 years ago
|
||
Try looks good, thanks. I don't have permission to land it myself ("You must have scm_level_3 access to land").
Keywords: checkin-needed
Comment 10•6 years ago
|
||
Need approval before landing.
Comment 11•6 years ago
|
||
Needs proper mozreview approval before it can be landed
Flags: needinfo?(pts+bmo)
Keywords: checkin-needed
Assignee | ||
Comment 12•6 years ago
|
||
Should be ready to go now.
Flags: needinfo?(pts+bmo)
Keywords: checkin-needed
Comment 13•6 years ago
|
||
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4561d6ed48b6 Fix firing menus.onShown for bookmark items r=mixedpuppy
Keywords: checkin-needed
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4561d6ed48b6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 15•6 years ago
|
||
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items Approval Request Comment [Feature/Bug causing the regression]: context menu on bookmarks [User impact if declined]: some extension functionality may not work [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: very simple change, covered by tests [String changes made/needed]: none
Attachment #8990116 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
status-firefox62:
--- → affected
Comment 16•6 years ago
|
||
Comment on attachment 8990116 [details] Bug 1473720: Fix firing menus.onShown for bookmark items Has good automated test coverage, adds a new test, let's uplift for beta 10.
Attachment #8990116 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/fff218cba849
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•