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

Fix history step continuation in a new Window #9901

Closed
wants to merge 2 commits into from

Conversation

domenic
Copy link
Member

@domenic domenic commented Nov 1, 2023

Closes #9869, by making it clearer that we figure out "navigable's active window" when the steps are run, not when they are declared.

Good commit message to be written once we figure out the right path here

@kalenikaliaksandr, can you take a look and confirm this clarifies the issue?


/browsing-the-web.html ( diff )

Closes #9869, by making it clearer that we figure out "navigable's active window" when the steps are run, not when they are declared.
@domenic domenic added clarification Standard could be clearer topic: navigation labels Nov 1, 2023
Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

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

This change fixes the problem I described in the issue. Thank you!

@kalenikaliaksandr
Copy link
Member

ok, I tried to apply this fix in my implementation and found another issue: the queued task is still never executed because the document associated with newWindow only becomes active once afterDocumentPopulated is complete.

@domenic
Copy link
Member Author

domenic commented Nov 2, 2023

Dang, right, OK.

So I think actually queueing a task might not be necessary at all. Nothing in afterDocumentPopulated needs to run on an event loop. So I think we can just set completionSteps to afterDocumentPopulated. I'll update the PR to do that for now.

However, I think that just delays the issue. The continuation at https://whatpr.org/html/9901/browsing-the-web.html#continuations-dequeue , in step 14.11, has a similar task-queuing issue.

We could try to fix that by moving steps 14.11.1.1-3 out of the queued task. However, a lot of those steps do want to be on the event loop. "Unload" explicitly asserts that. And "Make active" touches a lot of state which is associated with the (new) document, and thus should really ideally be only accessed while on that document's event loop. Hmm.

I guess we have a pretty fundamental conflict here which is that the unload event needs to be fired while the old Document is active, but "attempt to populate"'s whole deal is updating the current entry's document, which makes the old Document inactive.

It seems this problem is unique to reloads, because reloads are the only case where the active session history entry's document is modified in this way.

Do you have any ideas on what might be a better architecture? Would we have to do something dramatic like move away from the paradigm where "attempt to populate" modifies targetEntry? Or is there a more surgical fix? Any thoughts from @domfarolino @jakearchibald?

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Nov 2, 2023
@domenic domenic changed the title Editorial: clarify history step continuation in a new Window Fix history step continuation in a new Window Nov 2, 2023
@domenic domenic removed the clarification Standard could be clearer label Nov 2, 2023
@domenic
Copy link
Member Author

domenic commented Dec 13, 2023

Closing since this particular fix is unlikely to work; we'll continue discussing potential paths in #9869.

@domenic domenic closed this Dec 13, 2023
@domenic domenic deleted the clarify-reload-steps branch December 13, 2023 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: navigation
Development

Successfully merging this pull request may close these issues.

Navigables reloading does not work
2 participants