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

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

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).

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

Review commit: https://reviewboard.mozilla.org/r/196366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196366/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

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 :-( )

Comment 3

[Shane Caraveo (: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

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8925120 [details]
Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196366/diff/1-2/

Comment 6

[Shane Caraveo (: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/#review202042

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8925120 [details]
Bug 1414379 - Closing the extension popup tab should select the parent tab only if the popup tab was selected.

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

Comment 10

[Luca Greco [:rpl] [:luca] [: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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ec266337fc47

Comment 13

[CosminB]

Attachment: 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.