Skip to content
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

Improve integration with navigation #225

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

bokand
Copy link
Collaborator

@bokand bokand commented Apr 7, 2023

This PR makes it so that the fragment directive is stripped from the URL whenever it is set to a history entry. Instead, the history entry keeps it in a separate directive state.

The directive state is shared between contiguous history entries where the only difference is the non-directive portion of the fragment.

Moving between session history entries will cause the directive state, if it differs from the current entry, to be set into an uninvoked directives member on the Document. The "scroll to the fragment" steps will read directives from this member and attempt to perform the text search.

When the fragment search ends, uninvoked directives is always cleared.

The changes here are primarily in the "3.3 The Fragment Directive" and "3.4 Text Directives" sections. Most changes in other sections are minor and/or text that was moved out into these two section (and heavily rewritten).

Fixes #217


Preview | Diff

@bokand bokand requested review from annevk and jakearchibald April 7, 2023 01:01
@bokand
Copy link
Collaborator Author

bokand commented Apr 7, 2023

@jakearchibald @annevk, how does this look?

Copy link
Collaborator

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Mostly small changes needed. And handling the redirect case.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@bokand
Copy link
Collaborator Author

bokand commented Apr 18, 2023

@jakearchibald - latest commits should have addressed all your comments, WDYT?

@bokand
Copy link
Collaborator Author

bokand commented Apr 20, 2023

@annevk - in the interest of rationing your attention I'll merge this based on Jake's review (and the fact it'll be reviewed again on merge to HTML). But feel free to lmk if you'd prefer I wait on your review in future PRs (also happy to fix any issues post-merge)

@bokand bokand merged commit cc59249 into WICG:main Apr 20, 2023
@annevk
Copy link
Collaborator

annevk commented Apr 28, 2023

Thanks, that's fine. General approach seems good to me.

@bokand bokand mentioned this pull request Dec 13, 2023
@bokand bokand deleted the improveNavigationIntegration branch December 13, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The spec should be more explicit about how text directives interact with history
3 participants