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

Align navigate and reload with the HTML spec #861

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Jan 16, 2025

Closes #858


Preview | Diff

@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 16, 2025

I am not sure if accessing the navigation ID in this way is correct (it looked like it might not be for reload). Any suggestions are welcome.

@OrKoN OrKoN requested a review from sadym-chromium January 17, 2025 08:57
@OrKoN OrKoN requested a review from jgraham January 17, 2025 10:31
@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 17, 2025

@jgraham what do you think about using ongoing navigation this way? PTAL

index.bs Outdated

1. Return the result of [=await a navigation=] with |navigable|, |request| and
|wait condition|.
1. Let |navigation id| be |navigable|'s [=ongoing navigation=].
Copy link
Member

Choose a reason for hiding this comment

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

This actually doesn't work, and may require changes on the HTML side. The problem is that we can return from the above algorithm without starting a new navigation, and this step will get the id of some other navigation (or maybe null). Previously that was OK because we already knew the navigation id at this point, so in the next step we'd notice that a failure event had been dispatched and return. But now we need the navigation to be initiated to find the correct id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if the part using await for events after the navigate is also flawed, since by the time the await is called some events might have already happened?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looks like it probably is because await doesn't have a way to synchronously check if the resume condition was already met.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I am ready to propose a change to the HTML spec right now, but perhaps we could specify here that we want to yield the navigation id back to our algorithm as soon as it is defined on the step 6 and run the rest of the navigate/reload algorithm in parallel?

index.bs Outdated Show resolved Hide resolved
@OrKoN OrKoN force-pushed the orkon/align-navigation-with-html branch 2 times, most recently from 316f7d4 to 61f8f59 Compare January 17, 2025 13:02
@OrKoN OrKoN force-pushed the orkon/align-navigation-with-html branch from 61f8f59 to c7c8f7d Compare January 17, 2025 13:03
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.

Navigate and reload calls are out of sync with the HTML spec?
3 participants