[Spec] Overhaul 'restricting the text fragment' section #239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bokand
force-pushed
the
fixSecFetchSite
branch
from
57aa8ac to
54b5a07
Compare
|
@annevk does this look right? In particular, I noticed that the "create and initialize a Document object" steps already check the source origin but in "navigate" we snapshot the source origin so I'm guessing it can change and the existing case for about:blank is special? |
bokand
force-pushed
the
fixSecFetchSite
branch
2 times, most recently
from
b401390 to
b012092
Compare
sec-fetch-site was being checked for 'none' to indicate that a navigation was initiated from browser UI. However, we cannot inspect request headers from this part of the algorithm. Instead, the navigate algorithm now has a userInvolvement parameter which provides this information explicitly. Plumb that into navigation params and use it instead. Additionally, this change removes the top-level browsing context check from the document's text directive user activation flag since that's a confusing place to check it. Instead, move it to where this flag is being read and remove a (now-obviously) redundant check below.
`sec-fetch-site: same-origin` was being checked to tell if a navigation was initiated by a different origin. However, request headers can't be inspected at this point of the algorithm. Plumb through the initiatorOrigin parameter when loading a document and compare that with navigation params's origin field, using the `is same site` steps.
The main change in this commit is that enables same-document navigations
(with restrictions) by moving the security checks to also happen from
the navigate to fragment. As part of this, we do a fairly substantial
clean up and refresh of the 'restricting a text fragment' section. The
summarized high level changes:
* Split out the restrictions into a single set of 'check if a text
directive can be scrolled' steps, taking the necessary input as
parameters.
* Remove the 'allow text fragment scroll` flag on Document, instead
computing this value and passing it through various steps into
'scroll to the fragment'.
* The restriction is placed only on the 'scroll to the fragment'
steps, meaning the text directive is still the 'indicated part' and
can remain highlighted.
Partially addresses WICG#240
bokand
force-pushed
the
fixSecFetchSite
branch
from
b012092 to
81c42d9
Compare
bokand
changed the title
|
Ping - I've built a bit more on top of this so it no longer performs the checks in the 'create and initialize a document object' steps. (Please see also questions in whatwg/html#8282 (comment), if you'd rather review once this is "ready" for PR into HTML I can do that too) |
|
@annevk in the interest of making progress, and since I have a number of other changes and fixes that are blocked on this, I'm going to merge this as-is. This will still be reviewed as part of the HTML PR. Happy to make changes post-merge if you find any. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The spec was using the
sec-fetch-siterequest header to determine the initiator properties of the navigation, to use in security restrictions. However, request headers are appended only just prior to performing the fetch, this part of the algorithm operates on a clone of the request without headers so this doesn't work.This PR fixes the issues and rearranges the checks to occur in the
finalize a cross document navigationandnavigate to a fragmentsteps, also enabling text directives from a same-document navigation (the behavior in both Safari and Chrome, see #240).Fixes #179
Preview | Diff