Closing an extension popup tab should select the parent tab only if the popup tab was selected

VERIFIED FIXED in Firefox 58

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified, firefox59 verified)

Details

Attachments

(2 attachments)

As described in Bug 1373170 Comment 11, this issue has been introduced in Bug 1383160.

The issue also triggers an intermittent failure on the android pageAction popup test (test_ext_pageAction_getPopup_setPopup.html), which unfortunately was already disabled when Bug 1383160 has been landed.

This behavior of the extension popup should be restricted to just the scenario where the popup tab is removed while it is still the selected tab, so that (besides helping to fix the above intermittent failure) it is not surprising for the user (because if the user explicitly selects a different tab, the popup tab should still auto-close itself, but without changing the selected tab).
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1373170
Comment on attachment 8925120 [details]
Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

https://reviewboard.mozilla.org/r/196366/#review201552

::: mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:147
(Diff revision 1)
> +  // Selecting a Firefox for Android tab is asynchronous,
> +  // wait for 500ms (or once an unexpected "Tab:Selected" event has been
> +  // received).
> +  await Promise.race([
> +    new Promise(resolve => setTimeout(resolve, 500)),
> +    promiseDispatchedWindowEvent("Tab:Selected"),
> +  ]);

I'm highlighting this fragment because I'm very unhappy about using this approach 

(but I've not yet found a better strategy, all the other approaches that I tried were not able to make the test to fail consistently without the fix :-( )
Attachment #8925120 - Flags: review?(mixedpuppy)
Comment on attachment 8925120 [details]
Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

https://reviewboard.mozilla.org/r/196366/#review201556

::: mobile/android/components/extensions/ext-utils.js:258
(Diff revision 1)
>      super();
>  
>      // Keep track of the extension popup tab.
>      this._extensionPopupTabWeak = null;
> +    // Keep track of the last selected tabId
> +    this._lastSelectedTabId = null;

I would think about just calling this "_selectedTabId".  Using "last" makes me feel like it is the id of the tab selected prior to the currently selected tab.  Then change the comment to reflect that BrowserApp.selectedTab may not be set during operations initiated in tab:selected so we should rely on selectedTabId in those cases.

::: mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:20
(Diff revision 1)
> +
> +var {BrowserActions} = SpecialPowers.Cu.import("resource://gre/modules/BrowserActions.jsm", {});
> +var {Services} = SpecialPowers.Cu.import("resource://gre/modules/Services.jsm", {});
> +
> +function pageLoadedContentScript() {
> +  browser.test.sendMessage("page-loaded", window.location.href);

seems like you could just send this from a tabs.onUpdated listener in the background script that watches for the tab load being "complete"

::: mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:127
(Diff revision 1)
> +  is(popupParentTabId, tabId2, "The parent of the first popup tab is the second tab");
> +
> +  // Close the popup and test that its parent tab is selected.
> +  let onceTabClosed = promiseDispatchedWindowEvent("Tab:Closed");
> +  BrowserApp.closeTab(BrowserApp.selectedTab);
> +  await onceTabClosed;

the tab close/wait is repeated several times, perhaps a helper function to simplify reading the test?

::: mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:141
(Diff revision 1)
> +     "The second popup tab has been selected");
> +
> +  popupParentTabId = BrowserApp.selectedTab.parentId;
> +  is(popupParentTabId, tabId2, "The parent of the second popup tab is the second tab");
> +
> +  // Switch to the second tab and expect it to still be the selected tab

seems to be switching to the "first" tab

::: mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:149
(Diff revision 1)
> +  BrowserApp.selectTab(tab1);
> +  await onceTabClosed;
> +
> +  // Selecting a Firefox for Android tab is asynchronous,
> +  // wait for 500ms (or once an unexpected "Tab:Selected" event has been
> +  // received).

Cant we just wait for the tab:selected event where the selected tab is tab1?  Actually, given the tab switching just code above, you might do:

done = promise.all([wait tab:closed, wait tab:selected])
select tab1
await done
Duplicate of this bug: 1414917
Comment on attachment 8925120 [details]
Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

https://reviewboard.mozilla.org/r/196366/#review202042
Attachment #8925120 - Flags: review?(mixedpuppy) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/1d43c5bf597b
Closing the extension popup tab should select the parent tab only if the popup tab was selected. r=mixedpuppy
Backed out for eslint failure at mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:162:61 | Missing semicolon. (semi):

https://hg.mozilla.org/integration/autoland/rev/0f976e0fb3db972cd7d30b6d6063d0de3a17c2d3

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d43c5bf597bae8ac4b7dee718a02a0564c0085b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=142705277&repo=autoland

> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/components/extensions/test/mochitest/test_ext_popup_behavior.html:162:61 | Missing semicolon. (semi)
Flags: needinfo?(lgreco)
Sorry, I didn't notice that my last push to try didn't include the eslint step.

I've just reopened the mozreview request and fixed the eslint error.
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/ec266337fc47
Closing the extension popup tab should select the parent tab only if the popup tab was selected. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/ec266337fc47
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Posted file Bug1414379.zip
I can reproduce this issue on Firefox 57.0 (20171112125738) under Android 7.1.2.

From the pop-up page, if you select a tab that is not the parent tab, the focus will move to the parent tab and not to the selected tab.

This issue is verified as fixed on Firefox 59.0a1(20171113100237) and Firefox 58.0b3 (20171113173459) under Android 7.1.2. 

Please see the attached videos.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.