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

Fragment navigation should use replacement enabled #2869

Merged
merged 2 commits into from Aug 19, 2017

Conversation

@annevk
Copy link
Member

commented Jul 27, 2017

Fixes #2796.

@annevk annevk added the needs tests label Jul 27, 2017

@annevk

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2017

FYI: I'm not really happy with the language here, but changing "replacement enabled" into a proper flag would be quite a bit of work given how hand-wavy we go about passing it through at the moment. 😟

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

I haven't checked, but @ForbesLindesay's test at https://github.com/ForbesLindesay/jsdom/blob/3316c3c3126dd7aaa3886d3d274e49343ee82634/test/web-platform-tests/to-upstream/html/browsers/browsing-the-web/navigating-across-documents/fragment-replace.html may be adaptable here, although it's worth noting it fails in Edge and Firefox (but passes in Chrome). I'm not sure if the interop issue it thus reveals is related to this issue, or not.

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

Well, I've confirmed at least that manually running @ForbesLindesay's test case from the OP of #2796 works the same in Firefox as in Chrome (URL updates to #cat). So I guess it is a different issue; in fact the assert message is pretty helpful, and reveals that history.back() doesn't seem to be async in Firefox.

Which is all to say, maybe there is a simpler test that passes in all browsers and still tests the issue here, that we can extract out of @ForbesLindesay's test case.

@domenic
domenic approved these changes Aug 1, 2017
Copy link
Member

left a comment

Actual text LGTM (modulo build failures).

@annevk

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

I can't get there programmatically.

<iframe src=/></iframe>
<script>
onload = () => {
  self[0].location.hash = "t1"
  w(self[0].location.hash)
  self[0].location.hash = "t2"
  w(self[0].location.hash)
  self[0].location.hash = "t3"
  w(self[0].location.hash)
  history.back();
  setTimeout(() => {
    w(self[0].location.hash)
    self[0].location.replace("/#something");
    setTimeout(() => {
      w(self[0].location.hash)
      history.forward();
      setTimeout(() => {
        w(self[0].location.hash);
      }, 200);
    }, 200);
  }, 200);
}
</script>

In Live DOM Viewer does something entirely different in Firefox compared to Chrome. Firefox basically doesn't navigate back or forward.

cc @smaug----

@annevk

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

(Build failures fixed btw, apparently forgot to push a fix.)

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

In my experience testing in live-dom-viewer or similar does not work well because of initial about:blank issues; testing in isolated files I saved to my desktop improved interop a decent bit. Not sure if that's enough to fix things though.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

If I add w = al => alert(al); to the above and load it locally I get the same results in Fx and Chrome navigates back to the "New Tab" page after three alerts.

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

I'll try fiddling around with it tomorrow and see if I can make any progress. (Today I'm off as I've got family visiting.) Worst-case I guess we do a manual test...

domenic added a commit to web-platform-tests/wpt that referenced this pull request Aug 14, 2017
Add tests for replacement enabled and hashchange
Follows whatwg/html#2869 by adapting whatwg/html#2796. In the process, discovered a Firefox-specific bug around history.forward() and hashchange, so added a pared-down version of the test for that in particular.
@domenic

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

A couple weeks later than promised, but I managed to get a test case working at web-platform-tests/wpt#6877. Happy to merge this when you're ready.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Aug 15, 2017
Add tests for replacement enabled and hashchange
Follows whatwg/html#2869 by adapting whatwg/html#2796. In the process, discovered a Firefox-specific bug around history.forward() and hashchange, so added a pared-down version of the test for that in particular.

@domenic domenic removed the needs tests label Aug 15, 2017

@annevk annevk merged commit cf02423 into master Aug 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@annevk annevk deleted the annevk/fragment-replacement branch Aug 19, 2017

rachelandrew added a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
Add tests for replacement enabled and hashchange
Follows whatwg/html#2869 by adapting whatwg/html#2796. In the process, discovered a Firefox-specific bug around history.forward() and hashchange, so added a pared-down version of the test for that in particular.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.