Closed Bug 1364052 Opened 4 years ago Closed 3 years ago

Web Apps - Some webapps show the internal page url

Categories

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

55 Branch
ARM
Android
defect

Tracking

(firefox55 affected)

RESOLVED WORKSFORME
Tracking Status
firefox55 --- affected

People

(Reporter: oana.horvath, Unassigned)

References

Details

(Keywords: stale-bug)

Attachments

(1 file, 1 obsolete file)

Device: Any Android device.
Build: Nightly 55.0a1 (2017-05-11);

Steps to reproduce:
1. Go to pwa.rocks and install Babe news & RioRun apps.
2. Open the web apps and check the URL bar.

Expected result:
No URL bar displayed on the homepage/internal pages.

Actual result:
The homepage displays the mini URL bar.
The bar is kept while you navigate trough the app.
Also see https://deanhume.github.io/beer/ web app.
Priority: -- → P1
Note: It seems due to location changes once we open the page, 

from "https://babe.news/menu/home/populer/app"
to "https://babe.news/menu/home/populer"

and we cannot find corresponding manifest, then regards this situation as "Left WebApp". So far I don't have idea about dealing with location change in PWA.
Try to read Web App Manifest document[1]

I might be wrong. If I understand correctly, our current implementation of checking-URL-within-scope[2] makes sense to me: the targetURL is really out of scopeURL.

Since the web site's manifest haven't defined `scope` member, by the point 3 of definition of scope-member[3], should we return `undefined` when accessing scope? [3][4]


[1] https://w3c.github.io/manifest/#navigation-scope
[2] https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/dom/manifest/Manifest.jsm#176
[3] https://w3c.github.io/manifest/#scope-member
[4] https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/dom/manifest/Manifest.jsm#104
Per Julian's findings in comment 3, our current implementation is not fully aligned with w3c living standard though I don't really understand why the default scope is whole world when scopeURL undefined.
Flags: needinfo?(dale)
Yeh the spec looks wrong here, should probably get that fixed

The issues is likely to do with https://github.com/mozilla/gecko-dev/blob/master/dom/manifest/Manifest.jsm#L104, we use start_url as the default scope without stripping the filename, so if example.org/app/index.html is the start_url, we want example.org/app/ to be the scope
Assignee: nobody → dale
Flags: needinfo?(dale)
That means this bug is either invalid or worksforme. 
Is there any public discussion around this W3C spec?
No the bug is valid, I will put a patch up for it later today hopefully.

As for the spec, Marcos will have to review the patch so he can give us some idea about whether the spec will get fixed or not.
Previously we used the start_url of an app at its default scope if it did not define one in the manifest, this however meant if you installed https://deanhume.github.io/beer/index.html then https://deanhume.github.io/beer/about.html was considered out of scope.

This fix removes the filename from the start_url to find the new default scope, ie https://deanhume.github.io/beer/
Attachment #8870771 - Flags: review?(mcaceres)
And Marcos, additionally to the review, all of our reading of https://w3c.github.io/manifest/#navigation-scope seems to suggest that if the scope is not defined in the manifest then everything is within the scope of the app, ie if the user travels to totallydangerous.com they should not be given any indication they have left the current app. This seems wrong.
(In reply to Dale Harvey (:daleharvey) from comment #9)
> And Marcos, additionally to the review, all of our reading of
> https://w3c.github.io/manifest/#navigation-scope seems to suggest that if
> the scope is not defined in the manifest then everything is within the scope
> of the app, ie if the user travels to totallydangerous.com they should not
> be given any indication they have left the current app. This seems wrong.

Apologies, that's a misreading. The browser if free to display a warning, but shouldn't prevent the navigation from occurring (or could even stop the navigation, and ask for permission to leave foo.com). So, we can totally throw up a banner or show the address bar (that's what chrome does if you leave the origin). 

I'll add a note to the spec.
Comment on attachment 8870771 [details] [diff] [review]
Fix the default scope of an installed webapp

Review of attachment 8870771 [details] [diff] [review]:
-----------------------------------------------------------------

Leaving review unfinished till we hash out how we want this to work a bit more.

::: dom/manifest/Manifest.jsm
@@ +109,5 @@
> +    // If the manifest does not specify a scope, the default scope
> +    // is the root of the corrent page, so strip everything after last /
> +    // ie: example.org/app/index.html has scope example.org/app/
> +    let scope = this._store.data.manifest.start_url;
> +    return scope.substring(0, scope.lastIndexOf("/"));

I'm not sure this is correct, tbh. We should just return null and use the origin of the app as a soft boundary (as I commented in the bug). If the user leaves the origin, then we should somehow (UX/UI) convey to the user that they've left a safe space.
So a little surprised but chrome does use the origin as the default scope, this means that, https://daleharvey.github.io/testapp/ links to https://daleharvey.github.io/flagfiend/ and the navigation happens as within the same app. 

I dont think that makes this implementation wrong, in reality /flagfiend/ and /testapp/ are complete separate applications although from a permissions perspective at the moment the permissions are all tied to the origin, in future we may want to implicitly grant permissions for installed apps.

One possible issue with the /path/ implementation is that there may be some webapps that use example.org/home/ as their homepage with other pages hanging off example.org/about/ which the above implementation would flag as out of scope

I think without strong reasons not too its probably best to copy chromes implementation here, makes it easier for web devs and we dont have enough resources to push a compelling alternative right now
Match chrome and use origin as the default scope
Attachment #8870771 - Attachment is obsolete: true
Attachment #8870771 - Flags: review?(mcaceres)
Attachment #8871239 - Flags: review?(mcaceres)
Comment on attachment 8871239 [details] [diff] [review]
Fix the default scope of an installed webapp

Review of attachment 8871239 [details] [diff] [review]:
-----------------------------------------------------------------

Eventually we might need to make this a "soft scope" - to still allow unscoped navigations, but this is a good compromise.
Attachment #8871239 - Flags: review?(mcaceres) → review+
Cheers Marcos, the tests arent passing and this is likely not going to be needed as the scope testing is done on the Java side of the GeckoView implementation, will pick this up again when it lands and close it / refix in java as needed
Assignee: dale → nobody
This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.
Keywords: stale-bug
This is working in current Nightly
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.