Closed Bug 1383160 Opened 2 years ago Closed 2 years ago

popup behavior (browserAction & pageAction)

Categories

(WebExtensions :: Android, defect, P1)

55 Branch
defect

Tracking

(firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified

People

(Reporter: lemnis.martens, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Flags: needinfo?(amckay)
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.
Blocks: 1263005
Flags: needinfo?(amckay)
Priority: -- → P3
Assignee: nobody → lgreco
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8898292 - Flags: feedback?(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?
Attachment #8898292 - Flags: feedback?(mixedpuppy) → feedback+
(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.
(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?
Blocks: 1370333
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)
Depends on: 1394846
Priority: P3 → P1
Attachment #8898292 - Flags: review?(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 on attachment 8898292 [details]
Bug 1383160 - Fix Android pageAction popup behavior.

https://reviewboard.mozilla.org/r/169666/#review180162
Attachment #8898292 - Flags: review?(mixedpuppy) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/dc5c052296aa
Fix Android pageAction popup behavior. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/dc5c052296aa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attached image 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.