menus.onShown fails when context is a bookmark

RESOLVED FIXED in Firefox 62

Status

RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: pts+bmo, Assigned: pts+bmo)

Tracking

Trunk
mozilla63
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
Posted file Testcase
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

9 months 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

9 months 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

9 months ago
Attachment #8990116 - Flags: review?(mixedpuppy)

Comment 5

9 months 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

9 months 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

9 months 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+
Assignee: nobody → pts+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: qe-verify-
(Assignee)

Comment 9

9 months 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
Need approval before landing.
Needs proper mozreview approval before it can be landed
Flags: needinfo?(pts+bmo)
Keywords: checkin-needed
(Assignee)

Comment 12

9 months ago
Should be ready to go now.
Flags: needinfo?(pts+bmo)
Keywords: checkin-needed

Comment 13

9 months 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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4561d6ed48b6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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?
status-firefox62: --- → affected
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+

Updated

8 months ago
Blocks: 1215376
status-firefox61: --- → wontfix
status-firefox-esr60: --- → wontfix
You need to log in before you can comment on or make changes to this bug.