Closed Bug 1246044 Opened 5 years ago Closed 5 years ago

Split the context menu test into separate files to test icons, radio groups, checkboxes, and uninstalling.

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
mozilla50
Iteration:
48.3 - Apr 25
Tracking Status
firefox50 --- verified

People

(Reporter: mattw, Assigned: mattw, Mentored)

References

Details

(Whiteboard: [contextMenus][good first bug]triaged)

Attachments

(4 files, 1 obsolete file)

Actual:

When you remove an extension which modifies the context menu and install a new one, the context menu items added by the deleted extension still show up in the context menu.

Expected: 

The context menu items from the deleted extension should not show up in the context menu.
Interesting... It looks like it's because we delete the root item for the extension in the startup observer rather than in the shutdown observer.

Can you take this one, Matt, and hopefully add some tests for it as well?
Mentor: kmaglione+bmo
Whiteboard: [contextMenus][good first bug]
Sure, will do.
Assignee: nobody → mwein
Iteration: --- → 47.2 - Feb 22
Iteration: 47.2 - Feb 22 → ---
Duplicate of this bug: 1250642
Iteration: --- → 48.1 - Mar 21
Iteration: 48.1 - Mar 21 → ---
Iteration: --- → 48.3 - Apr 18
I confirmed manually that the change fixes the issue.  However, I can't seem to reproduce the bug in the tests, so I'm looking for feedback on how I might be able to do this.  Right now, if I load a new extension in the test without the fix, it seems to work fine.
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/1-2/
Attachment #8737867 - Attachment description: MozReview Request: Bug 1246044 Make sure context menu is properly cleaned up after uninstall. f?kmag → MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. f?kmag
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/2-3/
https://reviewboard.mozilla.org/r/44157/#review41809

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:59
(Diff revision 3)
> +  EventUtils.synthesizeMouseAtCenter(items[0].childNodes[0], {});
> +  yield popupShownPromise;
> +
> +  // Make sure the context menu items from the previous extension are gone.
> +  items = contentAreaContextMenu.getElementsByAttribute("ext-type", "top-level-menu");
> +  is(items.length, 1, "only one top level menu item should exist");

I confirmed that this test fails without the change and passes with the change (items.length equals 2 without the change).
Attachment #8737867 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review42445

Thanks! This is much clearer. Just need to fix some code duplication and it should be good to land.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_radioGroups.js:71
(Diff revision 3)
> +
> +  function* closeContextMenu(itemToSelect) {
> +    let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
> +    EventUtils.synthesizeMouseAtCenter(itemToSelect, {});
> +    yield popupHiddenPromise;
> +  }

Can you move these shared helper functions to `head.js` or another shared script?

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:22
(Diff revision 3)
> +    },
> +
> +    background: function() {
> +      browser.contextMenus.create({title: "a"});
> +      browser.contextMenus.create({title: "b"});
> +      browser.test.notifyPass();

Please always pass a relevant string to `notifyPass` and `awaitFinish`

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:69
(Diff revision 3)
> +  EventUtils.synthesizeMouseAtCenter(items[0], {});
> +  yield popupHiddenPromise;
> +
> +  // Uninstall the extension.
> +  yield extension.unload();
> +  yield BrowserTestUtils.removeTab(tab1);

Please check that there are no more context menu items after the second extension is unloaded.
Attachment #8737867 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/3-4/
Attachment #8737867 - Attachment description: MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. f?kmag → MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall.
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/4-5/
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/5-6/
Kris, this should be ready for review now, but I'm not sure if it makes sense to have the method for opening the extension menu as well as a method for opening the context menu in head.js, although I do use them both.

Also, I'm not sure if the method for closing the extension menu should support checking the click info, since doing so requires it to have a reference to the extension.

What do you think?
Attachment #8737867 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review43491

::: browser/components/extensions/test/browser/browser.ini:73
(Diff revisions 3 - 6)
> +[browser_ext_windows.js]
>  [browser_ext_windows_create.js]
>  tags = fullscreen
>  [browser_ext_windows_create_tabId.js]
> -[browser_ext_windows.js]
> +[browser_ext_windows_events.js]
>  [browser_ext_windows_size.js]

I'm fine with re-sorting this file, but please do it in a second commit, since it's likely to cause merge conflicts.

::: browser/components/extensions/test/browser/head.js:145
(Diff revision 6)
>  }
>  
> +function* openContextMenu() {
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
> +  let popupShownPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popupshown");
> +  yield BrowserTestUtils.synthesizeMouseAtCenter("#img1", {type: "contextmenu", button: 2}, gBrowser.selectedBrowser);

"#img1" should be passed in as an argument.

::: browser/components/extensions/test/browser/head.js:161
(Diff revision 6)
> +}
> +
> +function* openExtensionContextMenu() {
> +  yield openContextMenu();
> +
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");

This is returned by `openContextMenu`.

::: browser/components/extensions/test/browser/head.js:185
(Diff revision 6)
> +  }
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
> +  let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
> +  EventUtils.synthesizeMouseAtCenter(itemToSelect, {});
> +
> +  if (extension) {

This `if` statement and the `checkClickInfo` function are too context-specific to be in `head.js`. I'm fine with just duplicating them for each test file.
Attachment #8737867 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/6-7/
Attachment #8737867 - Attachment description: MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. → MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/7-8/
Attachment #8737867 - Attachment description: MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n → MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall.
https://reviewboard.mozilla.org/r/44157/#review43491

> I'm fine with re-sorting this file, but please do it in a second commit, since it's likely to cause merge conflicts.

I un-sorted the file, but I'm confused because the original revision looks like it's sorted...
https://reviewboard.mozilla.org/r/44157/#review43491

> I un-sorted the file, but I'm confused because the original revision looks like it's sorted...

Actually, I think the file was sorted already in Bug 1257583, and it got sorted here when I pulled down the latest from fx-team in revision 7.  Should I re-sort it, or do you think isn't the case?
Attachment #8737867 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/44157/#review43491

> Actually, I think the file was sorted already in Bug 1257583, and it got sorted here when I pulled down the latest from fx-team in revision 7.  Should I re-sort it, or do you think isn't the case?

Clarification: I unsorted it in revision 7 per your request, but I think it should stay sorted since that's what's arleady in fx team.
https://reviewboard.mozilla.org/r/44157/#review43491

> Clarification: I unsorted it in revision 7 per your request, but I think it should stay sorted since that's what's arleady in fx team.

s/Clarification/Correction/g :)
(In reply to Matthew Wein [:mattw] from comment #21
> Actually, I think the file was sorted already in Bug 1257583, and it got
> sorted here when I pulled down the latest from fx-team in revision 7. 
> Should I re-sort it, or do you think isn't the case?

Please use the same sorting as is used in the parent revision.
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review45885

::: browser/components/extensions/test/browser/browser_ext_contextMenus_checkboxes.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_radioGroups.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.
Attachment #8737867 - Flags: review?(kmaglione+bmo) → review+
Tested this issue in nightly 49.0a1 and found that after disabling or removing an extension the custom entry is disappeared from context menu but enabling the extension or clicking "Undo" in Addons Manager duplicates the menu entry as many times as you disable/enable the extension and only the last of them actually works.
Priority: -- → P2
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
Summary: When you delete an extension which modifies the context menu and then install an extension, the menu items from the deleted extension still show up → Split the context menu test into separate files to test icons, radio groups, checkboxes, and uninstalling.
Blocks: 1284265
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/8-9/
Attachment #8737867 - Attachment description: MozReview Request: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. → Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall.
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/9-10/
Attachment #8737867 - Attachment description: Bug 1246044 Split context menu tests into separate files, and make sure context menu is properly cleaned up after uninstall. → Bug 1246044 Split context menu tests into separate files, add tests for icons, and make sure context menu is properly cleaned up after uninstall.
Attachment #8737867 - Flags: review+ → review?(kmaglione+bmo)
No longer blocks: 1284265
Review commit: https://reviewboard.mozilla.org/r/62566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62566/
Attachment #8737867 - Attachment description: Bug 1246044 Split context menu tests into separate files, add tests for icons, and make sure context menu is properly cleaned up after uninstall. → Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.
Attachment #8768244 - Flags: review?(kmaglione+bmo)
Attachment #8768245 - Flags: review?(kmaglione+bmo)
Attachment #8768246 - Flags: review?(kmaglione+bmo)
Attachment #8768247 - Flags: review?(kmaglione+bmo)
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/10-11/
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review59504

::: browser/components/extensions/test/browser/.eslintrc:28
(Diff revision 11)
>      "focusWindow": true,
>      "makeWidgetId": true,
> +    "openContextMenu": true,
> +    "closeContextMenu": true,
> +    "openExtensionContextMenu": true,
> +    "closeExtensionContextMenu": true,

Please sort the symbols in this list.
Attachment #8737867 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

https://reviewboard.mozilla.org/r/62566/#review59510
Attachment #8768244 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

https://reviewboard.mozilla.org/r/62568/#review59512

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:9
(Diff revision 1)
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;

This isn't necessary.
Attachment #8768245 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8768246 [details]
Part 4 - Bug 1246044 - Fix icon regression caused by bug 1270742.

https://reviewboard.mozilla.org/r/62570/#review59514

This commit summary is inaccurate.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:9
(Diff revision 1)
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;

This isn't necessary.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:11
(Diff revision 1)
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;
> +
> +  let dataURI = "iVBORw0KGgoAAAANSUhEUgAAACQAAAAkCAYAAADhAJiYAAAC4klEQVRYhdWXLWzbQBSADQtDAwsHC1tUhUxqfL67lk2tdn+OJg0ODU0rLByqgqINBY6tmlbn7LMTJ5FaFVVBk1G0oUGjG2jT2Y7jxmmcbU/6iJ+f36fz+e5sGP9riCGm9hB37RG+scd4Yo/wsDXCZyIE2xuXsce4bY+wXkAsQtzYmExrfFgvkJkRbkzo1ehoxx5iXcgI/9iYUGt8WH9MqDXEcmNChmEYrRCf2SHWeYgQx3x0tLNRIeKQLTtEFyJEep4NTuhk8BC+yMrwEE3+iozo42d8gK7FAOkMsRiiN8QhW2ttSK5QTfRRV4QoymVeJMvPvDp7gCZigD613MN6yRFA3SWarow9QB9LCfG+NeF9qCtjAKOSQjCqVKhfVsiHEQ+grgx/lRGqUihAc1uL8EFD+KCRO+GrF4J61phcoRoPoEzkYhZYpykh5sMb7kOdIeY+jHKur4QI4Feh4AFX1nVeLxrAvQchGsBz5ls6wa2QdwcvIcE2863bTH79KOvsz/uUYJsp+J0pSzNlDckVqqVGUAF+n6uS7txcOl6wot4JVy70ufDLy4pWLUQVPE81pRI0mGe9oxLMHSeohHvMs/STUNaUK6vDPCvOyxMFDx4achehRDJmHnydnkPww5OFfLxrGIZBFDyYl4LpMzlTQFIP6AQx86w2UeYBccFpJrcKv5L9eGDtUAU6RIELqsB74uynjy/UBRF1gS5BTFxwQT1wTiXoUg9MH7m/3NZRRoi5IJytUbMgzv4Wc832+oQkiKgEehmyMkkpKsFkQV11QsRJL5rJYBLItQgRaUZEmnoZXsomz3vGiWw+I9KMF9SVFOqZEemZekli1jN3U/UOqhHHvC6oWWGElhfSpGdOk6+O9prdwvtLj5BjRsQxdRnot+Zeifpy/2/0stktKTRNLmbk0mwXyl8253fyojj+8rxOHNAhjjm5n0/5OOCGOKBzkrMO0Z75lvSAzKlrF32Z/3z8BqLAn+yMV7VhAAAAAElFTkSuQmCC";

This is not actually a data URI.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:47
(Diff revision 1)
> +  });
> +
> +  let confirmContextMenuIcon = (rootElement) => {
> +    let expectedURL = new RegExp(String.raw`^moz-extension://[^/]+/extension\.png$`);
> +    let imageUrl = rootElement.getAttribute("image");
> +    is(expectedURL.test(imageUrl), true, "The context menu should display the extension icon next to the root element");

`ok(expectedURL.test(...), "...")`
Attachment #8768246 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

https://reviewboard.mozilla.org/r/62572/#review59516

This commit summary is inaccurate, this is fixed from a patch in another bug, and this patch adds a test file to the manifest which was created in the previous patch, rather than this one.
Attachment #8768247 - Flags: review?(kmaglione+bmo)
Depends on: 1284265
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/11-12/
Attachment #8768247 - Attachment description: Part 5 - Bug 1246044 - Add unit tests for context menu icons. → Part 4 - Bug 1246044 - Add unit tests for context menu icons.
Attachment #8768247 - Flags: review?(kmaglione+bmo)
Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62566/diff/1-2/
Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62568/diff/1-2/
Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62572/diff/1-2/
Attachment #8768246 - Attachment is obsolete: true
Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

https://reviewboard.mozilla.org/r/62572/#review59590
Attachment #8768247 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/12-13/
Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62566/diff/2-3/
Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62568/diff/2-3/
Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62572/diff/2-3/
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b113ed488daf
Part 1 - Split out the testing for radio groups and checkboxes into separate files. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/2eb259ef1448
Part 2 -  Fix issue where context menu items show up after extension is uninstalled. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/db20be5bef8a
Part 3 - Add unit tests for uninstall bug. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/811b5f9b7ae8
Part 4 - Add unit tests for context menu icons. r=kmag
Keywords: checkin-needed
Duplicate of this bug: 1288695
Duplicate of this bug: 1288312
I was able to reproduce the initial issue on Firefox 48.0.2 (20160823121617) under Windows 10 64-bit.

Verified as fixed on Firefox 50.0a2 (2016-09-05) and Firefox 51.0a1 (2016-09-05) under Windows 10 64-bit and here are the results:
- context menu item is no longer multiplied while disabling/enabling/reloading the webextension
- context menu item is completely erased when the webextension is removed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.