1383160 - popup behavior (browserAction & pageAction)

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

[lemnis.martens]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

It's not a bug, it's a feature request.

I was looking at the current behavior of a popup within Android. I do not think the current behavior is the best experience, so I wanted to start a discussion around it.

If this is already discussed and I failed to find it, just ignore my opinion.


Actual results:

1. open a tab where a pageAction is shown
2. click on the icon so that a popup is opened
3. open the app drawer
    - You will see that the popup is always the last created tab
4. switch back to the previous tab
5. click on the icon again so that another popup is opened



Expected results:

I expect that:
- Never multiple popups are created of 1 parent tab
- The newly created popup is placed after the parent tab or is never shown in the tab drawer
- A popup is closed after you switched to another tab

But mostly I want to start a discussion or somebody that tells me that the experience will get improved in the future.

Comment 1

[Andy McKay]

Agreed, this was the plan but we never had the time to implement. I thought we'd filed a bug about it, but looks we never did.

Comment 2

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

Attachment: Bug 1383160 - Fix Android pageAction popup behavior.

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

Comment 3

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

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

Comment 4

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

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

Comment 5

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

Hi Shane
This patch contains my proposal to fix the "extension popup tab" behavior on Android, mostly as described in Comment 0, e.g.:

- there is only one "extension popup tab" opened at any time
- the "extension popup tab" is automatically closed as soon as it has been unselected (e.g. the user switch to another tab)
- the "extension popup tab" is never considered active, when it is selected the active tab should be the tab that was selected when the "extension popup tab" has been opened

The patch currently contains only one test (which tests the behaviors described above and the activeTab permission), I think that I'm going to add some additional tests around these changes and unrelated to the activeTab test, but first I'd like to hear your opinion on the strategy sketched by this patch before adding more code in it.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

This looks good.  

Is there any issue in browserAction with the activeTab from this?

Is the TabTracker constructor necessary?  If Tab:Selected needs to be listened for that early, maybe just do that in the constructor and let init get called by the base class?

Comment 7

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

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

Comment 8

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Is the TabTracker constructor necessary?  If Tab:Selected needs to be
> listened for that early, maybe just do that in the constructor and let init
> get called by the base class?

As briefly discussed over IRC, about the TabTracker init method, I think that it was an existent bug of the Android implementation, that I noticed when I started to look into this "popup behavior" patch:

In the desktop WE internals, it is called lazily here:

  http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browser.js#242

but it looks that it has never been called in the Android version.

Comment 9

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

(In reply to Luca Greco [:rpl] from comment #8)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > Is the TabTracker constructor necessary?  If Tab:Selected needs to be
> > listened for that early, maybe just do that in the constructor and let init
> > get called by the base class?
> ...
> but it looks that it has never been called in the Android version.

I wasn't still convinced about this ^ (e.g. because the tabs.onCreated event is tested and it is sent by TabTracker, after subscribing the TabOpen event), I digged into it again and it looks that `this.init()` is also called by TabTrackerBase when the first event listener is subscribed to the browser.tabs API, you were right:

- http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/extensions/ext-tabs-base.js#1043

I'm going to follow the same approach and defer to call `this.init()` until a popup extension tab has been created,
how that sounds to you?

Comment 10

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169666/diff/4-5/

Comment 11

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

I've added Bug 1394846 as a dependency (I tested this patch against intermittent failures and it seems that this also need the fixes on the pageAction show/hide test from Bug 1394846 patch).

I've also raised the priority of this issue (mostly because I think that making the popup able to detect the original tab as the active tab is probably needed by most of the real world extension popups)

Comment 12

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169666/diff/5-6/

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

https://reviewboard.mozilla.org/r/169666/#review180152

::: mobile/android/components/extensions/ext-utils.js:298
(Diff revision 6)
> +    return undefined;
> +  }
> +
> +  /**
> +   * Open a pageAction/browserAction popup url in a tab and keep track of
> +   * its weak reference (to be to customize the activedTab using the tab parentId,

fix comment

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

https://reviewboard.mozilla.org/r/169666/#review180162

Comment 15

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

Comment on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169666/diff/6-7/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc5c052296aa
Fix Android pageAction popup behavior. r=mixedpuppy

Comment 17

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

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

Comment 18

[CosminB]

Attachment: Bug1383160.gif

This issue is verified as fixed on Fennec 57.0a1 (2017-09-07) under Android 6.0.1.

The “extension popup tab” behavior described in comment 5 is working as expected.
The extension used for the testing is: 
https://addons-dev.allizom.org/en-US/firefox/addon/beastify-for-android/ 

Please see the attached video.