-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking: Change requirement to block off-scope navigation. #701
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. A few suggestions.
index.html
Outdated
Unlike previous versions of this specification, user agents are no | ||
longer required or allowed to block off-scope navigations, or open | ||
them in a new <a>top-level browsing context</a>. This practice broke | ||
a lot of sites that navigate to a URL on another origin (e.g., to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps remove the "a lot of" qualifier?
index.html
Outdated
Unlike previous versions of this specification, user agents are no | ||
longer required or allowed to block off-scope navigations, or open | ||
them in a new <a>top-level browsing context</a>. This practice broke | ||
a lot of sites that navigate to a URL on another origin (e.g., to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to a URL on another origin" -> "to an off-scope URL" (since scope can be narrower than an origin)
index.html
Outdated
them in a new <a>top-level browsing context</a>. This practice broke | ||
a lot of sites that navigate to a URL on another origin (e.g., to | ||
perform third-party authentication), which would then navigate back to | ||
the originating site. See |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove everything from comma to full stop? Bug has more details.
index.html
Outdated
The <a>start URL</a> is not necessarily the value of the | ||
<a data-link-for="WebAppManifest"><code>start_url</code></a> member: | ||
the user or user agent could have changed it when the application | ||
was added to home-screen or otherwise bookmarked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"was added to...." -> "installed"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, moved this to #702 since it's kind of off-topic for this breaking change.
32ccc73
to
83643d9
Compare
I responded by completing all of your suggestions, @dominickng |
index.html
Outdated
"!HTML#active-document">active document</a>'s <a data-cite= | ||
"!DOM#concept-document-url">URL</a> is not <a>within scope</a> of the | ||
navigation scope of the <a>application context</a>'s manifest, the user | ||
agent SHOULD show prominent UI indicating the <a data-cite= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI elements? UI just means user interface
index.html
Outdated
<a>origin</a>, including whether it is served over a secure connection. | ||
This UI SHOULD differ from any UI used when the <a data-cite= | ||
"!DOM#concept-document-url">document URL</a> is <a>within scope</a>, in | ||
order to alert the user of the off-scope navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not such a fan of "alert"
in order to make it obvious to the user that they are navigating off-scope.
algorithm. As such, the following algorithm monkey patches [[!HTML]]. | ||
<a href="https://www.w3.org/Bugs/Public/show_bug.cgi?id=27653">Bug | ||
27653</a> has been filed to address this. | ||
Unlike previous versions of this specification, user agents are no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already in a "NOTE" block (don't like redundantly writing "Note" everywhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, great
index.html
Outdated
not to say that the web application cannot be navigated: just that | ||
the set of URLs to which the manifest applies is restricted by the | ||
<a>navigation scope</a>. | ||
The above recommendation to show UI when the <a>application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is to show?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, can you split up the sentence instead?
security -> security reasons?
The previous text did not match the way the HTML navigate algorithm works. The new text aligns with the wording in the HTML algorithm, while preserving the currently specified behaviour. The second part (attempting to capture redirects off-scope) is a little vague, but should capture the intention.
Previously, user agents were required to prevent off-scope navigation (or open it in a new top-level browsing context). Now, they are not allowed to do this, as it broke existing sites not designed to be run under these conditions. Replaced with a recommendation to show the URL when off-scope. Closes w3c#646.
83643d9
to
1dbac2e
Compare
Responded to @kenchris comments. |
@dominickng and I have been talking and want to proceed with this. We're getting a lot of requests to fix our broken navigation within PWAs. Chrome has already implemented this on Android, and we're committed to changing our Desktop implementation to match. However, none of the other major browsers do this. So we need a commitment from at least one other browser (Edge, Firefox, Safari) before we land this. @boyofgreen @marcoscaceres @davidquesada please give us your thoughts. As we've discussed on #646, even though this change breaks the existing spec (which other browsers — including Chrome on Desktop — are implementing), what we're really doing is bringing PWAs in line with normal HTML navigations. Currently, user agents following the Manifest spec are forced to break the normal behaviour of target=self links on existing websites. |
I'm personally supportive with the change, but I can't commit to implementation until next year 😢 Meeting with my colleagues from Mozilla in December to take stock of where we are at with a product strategy. |
Hi @marcoscaceres. Thanks. I'll take your support and understand that you can't commit to implementing in Firefox until next year. I haven't heard back from any of the others. I'd like to move forward with this change regardless because the current behaviour is untenable for us (we are constantly receiving reports about this). I've compiled a document summarizing the issue with a list of reports we've gotten internally, and some external bug reports: Since I've already got an approval from Ken, I'm going to merge this later today unless there are objections. |
Go for it. Just please reach out to the Apple folks and make sure they are aware. I'm sure they are probably getting similar reports. |
Previously, user agents were required to prevent off-scope navigation (or open it in a new top-level browsing context). Now, they are not allowed to do this, as it broke existing sites not designed to be run under these conditions. Replaced with a recommendation to show the URL when off-scope. Closes w3c#646.
…e theming. This further clarifies that a manifest is allowed to apply to URLs that are not within its scope (that this was not clear was a left-over mistake from w3c#701), and that a document within scope is allowed to override the theme color. Adds a new recommendation that an out of scope document not be allowed to override the theme color. This introduces new normative recommendations, only insofar as they were already commonly understood, to my knowledge. Navigation scope: No longer defined in terms of the URLs that a manifest is allowed to apply to (since a manifest may apply to any URL). Rather, it's just a set of URLs; the other normative requirements around navigation scope define what it means. Applied: Clarified that it is the user agent's discretion whether or not to apply a manifest when a top-level browsing context is created, with examples of when you would and would not apply it (previously, there was simply no text around when a manifest should be applied). Theme color: Fixes a "MAY" requirement imposed on the document (requirements can only be imposed on user agents). Clarifies this recommendation as only applying to in-scope documents, and adds a recommendation against applying document theme for out-of-scope documents. Closes w3c#755.
Previously, user agents were required to prevent off-scope navigation (or open it in a new top-level browsing context). Now, they are not allowed to do this, as it broke existing sites not designed to be run under these conditions.
Replaced with a recommendation to show the URL when off-scope.
Closes #646.
Preview | Diff