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

Attempt to test replace before load #28480

Merged
merged 18 commits into from
Jun 24, 2021
Merged

Attempt to test replace before load #28480

merged 18 commits into from
Jun 24, 2021

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 14, 2021

Follows whatwg/html#6714.

@domenic

This comment has been minimized.

Copy link
Contributor

@natechapin natechapin left a comment

Choose a reason for hiding this comment

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

I know it's what you documented in whatwg/html#6714, but I find the special cases for following hyperlinks and window.open() a little jarring. I wonder if that's a necessary permanent behavior or something that we could adjust.


t.step_timeout(() => {
asssert_equals(w.location.href, absoluteStartURL, "1 second after attempting to go back, it indeed went back");
}, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below: any way to shorten this? 1 second is a long time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to waiting for a posted message

@domenic
Copy link
Member Author

domenic commented Jun 23, 2021

I know it's what you documented in whatwg/html#6714, but I find the special cases for following hyperlinks and window.open() a little jarring.

I suspect window.open() could be changed, as could aElement.click(). But user-triggered hyperlinks are probably the way they are for a reason: if you as a user click on a link while a large page (like the HTML spec) is still loading, you probably want your back button to take you back to that large page, and not to skip the large page and go back to whatever you were on previously.

w.history.back();

t.step_timeout(() => {
asssert_equals(w.location.href, absoluteStartURL, "1 second after attempting to go back, it indeed went back");
Copy link
Contributor

Choose a reason for hiding this comment

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

You've got a couple of 'asssert_equals' typos throughout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dear, the fact that these tests pass is troubling then. Thanks so much for the careful review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@domenic domenic merged commit c1bb1fd into master Jun 24, 2021
@domenic domenic deleted the replace-before-load branch June 24, 2021 18:12
@foolip
Copy link
Member

foolip commented Jun 24, 2021

@natechapin I've sent you an invite for the wpt reviewers team so that next time your review is enough and you can also merge PRs. (It's write access, just the team isn't named as such.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants