Closed Bug 1353857 Opened 3 years ago Closed 3 years ago

Fix handling of entering/leaving Web App

Categories

(Firefox for Android :: Web Apps (PWAs), enhancement, P2)

All
Android
enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: JanH, Assigned: JanH)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

At the moment, the message passed by Gecko (Website:AppEntered/Left) doesn't even include the tab ID, which means that the address bar can be shown when it shouldn't be or vice versa.

To be done after bug 1352997, though, so we can do it properly...
Priority: -- → P2
Assignee: nobody → jh+bugzilla
Duplicate of this bug: 1357779
Attachment #8858824 - Flags: review?(dale)
Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135714

Minor nit, otherwise looks great, cheers

::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java:113
(Diff revision 2)
>      }
>  
>      @Override
>      public void handleMessage(final String event, final GeckoBundle message,
>                                final EventCallback callback) {
> +        if (message.containsKey("tabId") && message.getInt("tabId") == mLastSelectedTabId) {

I am not 100% clear on our or Java's coding style preferences, but early returns always seem cleaner for this 

   if (!message.containsKey("tabId") || message.getInt("tabid") != mLastSelectedTabId) { 
       return;
   }
   
Feel free to ignore especially if it goes against our standard
Attachment #8858824 - Flags: review?(dale) → review+
Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135718
Attachment #8858824 - Flags: review?(walkingice0204) → review+
Comment on attachment 8858824 [details]
Bug 1353857 - Include the tab ID when notifying about leaving/entering a web app's scope.

https://reviewboard.mozilla.org/r/130846/#review135714

> I am not 100% clear on our or Java's coding style preferences, but early returns always seem cleaner for this 
> 
>    if (!message.containsKey("tabId") || message.getInt("tabid") != mLastSelectedTabId) { 
>        return;
>    }
>    
> Feel free to ignore especially if it goes against our standard

As things currently stand, you're right. I was just thinking that eventually we might need to handle other events with possibly different pre-conditions which might not be compatible with early returns, but I suppose we can worry about that situation if we actually run into it.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/9ce0883ffd62
Include the tab ID when notifying about leaving/entering a web app's scope. r=daleharvey,walkingice
Backed out because the dependent bug failed to land: https://hg.mozilla.org/integration/autoland/rev/483340ef5362
Flags: needinfo?(jh+bugzilla)
Flags: needinfo?(jh+bugzilla)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/1219abce0035
Include the tab ID when notifying about leaving/entering a web app's scope. r=daleharvey,walkingice
https://hg.mozilla.org/mozilla-central/rev/1219abce0035
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Build: Nightly 2017-05-04
Devices: HTC 10 (Android 6) & Prestigio Grace X5 (Android 4.4.2)
On Twitter & Pokedex.org web apps, when you open an external link, it's opened in a new browser tab, not inside the web app.
Flags: needinfo?(jh+bugzilla)
Twitter for me opens the link in a new browser tab, which I believe is a change on twitter side so it now behaves the same as chrome

Pokedex behaves the same for me and navigates with the header
- That's not caused by this bug, so if we wanted to change this, this belongs in a separate bug.
- Yup, those links are opening and selecting a new (normal) tab, so because of bug 1351739 this'll trigger an activity switch.
- If we don't want this, this (https://dxr.mozilla.org/mozilla-central/rev/4a6a71f4aa22e4dc3961884ce505ce34bdd799a2/mobile/android/chrome/content/browser.js#3404) is the place to control this behaviour (whether to open a new tab or not), but that's a matter of taste (and if Chrome does it as well, that's one less reason to change our behaviour)
- Re testing with Pokedex, the link at the bottom of the page stays within the tab, but the "About" link in the sidebar menu opens a new tab.
Flags: needinfo?(jh+bugzilla)
Ah thats how it works, yeh I have been meaning to look into why twitter links were previously opened in place but hadnt got to it. So basically 1351739 fixed _blank links, which is perfect :)
Yup, you're right, it behaves the same with Chrome. I'll mark this as fixed, thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.