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

Remove page url check during `history.pushState` #9468

Merged
merged 4 commits into from Jun 5, 2017

Conversation

Projects
None yet
2 participants
@tonyganch
Member

tonyganch commented May 15, 2017

This should fix #9296.
Test repo: https://github.com/tonyganch/electron-quick-start/tree/history-regression

TL;DR: Current implementation of NavigationController does not allow using
history.pushState() if page url is not changed.
I think it worked by mistake in versions < 1.3.6 and got visible after fix 180a77e.

The issue: live.com website uses history.pushState() and history.back() to navigate between different views of login form. Click on "Sign in" button calls history.pushState({viewId: viewId}, '') and click on "Back" button calls history.back().
On history.pushState() NavigationController checks if the url of the page has changed and if not, then it doesn't push new state to history, meaning that history.pushState() called from js does absolutely nothing.
When the page then tries to call history.back(), NavigationController checks if the history is empty, sees that it is and does nothing (which makes sense).

Electron has its implementation of NavigationController which was introduced by this commit: 0143a45.
You can see that on line 19 we added the check if the page has the same url or a new one:
0143a45#diff-35a86eec0f3148dfbe7621bd8f9bab7cR19
I don't see such check in previous code which makes me think that maybe it was introduced by mistake?

Commit after which everything "broke" is 180a77e, but i honestly think that it just made the situation obvious and the things didn't work properly even before that.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 15, 2017

Test repo: https://github.com/tonyganch/electron-quick-start/tree/history-regression

@tonyganch would it be possible to get a test in this pull request that mirrors the behavior in your test repo?

tonyganch added a commit to tonyganch/electron that referenced this pull request May 16, 2017

@tonyganch

This comment has been minimized.

Member

tonyganch commented May 16, 2017

@kevinsawicki, I added a test to api-web-contents-spec.js as it seemed to be the most appropriate place. I checked that the test fails without the fix and passes with it.

tonyganch added a commit to tonyganch/electron that referenced this pull request May 16, 2017

tonyganch added a commit to tonyganch/electron that referenced this pull request May 23, 2017

tonyganch added some commits May 16, 2017

Remove page url check in `history.pushState`
Current implementation of NavigationController does not allow using
`history.pushState()` if page url is not changed.
It worked by mistake in versions < 1.3.6 and got visible after fix 180a77e.
@tonyganch

This comment has been minimized.

Member

tonyganch commented May 24, 2017

@kevinsawicki, i fixed a couple of linter errors, now all checks pass

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 5, 2017

Thanks for this @tonyganch 👍 🚀

@kevinsawicki kevinsawicki merged commit 53b6ee0 into electron:master Jun 5, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment