If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix handling of entering/leaving Web App

VERIFIED FIXED in Firefox 55

Status

()

Firefox for Android
Web Apps
P2
normal
VERIFIED FIXED
6 months ago
5 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
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)

Updated

5 months ago
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request)
Duplicate of this bug: 1357779
Comment hidden (mozreview-request)
Attachment #8858824 - Flags: review?(dale)

Comment 4

5 months ago
mozreview-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

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 5

5 months ago
mozreview-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+
(Assignee)

Comment 6

5 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

5 months ago
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)
(Assignee)

Updated

5 months ago
Flags: needinfo?(jh+bugzilla)

Comment 12

5 months ago
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

Comment 13

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1219abce0035
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 14

5 months ago
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
(Assignee)

Comment 16

5 months ago
- 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 :)

Comment 18

5 months ago
Yup, you're right, it behaves the same with Chrome. I'll mark this as fixed, thanks!
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.