Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1063573002:
history.pushState() should take care of scrolling state (Closed)
|
Created:
5 years, 8 months ago by Abhijeet Kandalkar Slow Modified:
5 years, 8 months ago CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionhistory.pushState() should take care of scrolling state.
Current implementation don't save scrolling state while calling history
pushState() therefore history entries doesn't have information about
scrolloffset of page. While calling history.back(), wrong values of scroll
offset are considered and page jump to top. The patch adds an implementation
to save scrolling state while calling history.pushState().
BUG=472023
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193603
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Patch for landing #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 19 (5 generated)
abhijeet.k@samsung.com changed reviewers: + japhet@chromium.org, vivekg@chromium.org
Please review patch.
https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:26: }, 10); Is the 10ms delay required here? Could we get away with a 0ms setTimeout? https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:35: // Click 'Push State' button twice Does this test work with direct history.pushState() and history.back() calls? I don't think we need a user gesture to test the correct behavior, so eventSender is probably overkill. https://codereview.chromium.org/1063573002/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1063573002/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:310: saveScrollState(); I believe there's a saveScrollState() call in FrameLoader::loadInSameDocument, which saves scroll state for non-history same document navigations. Could we move that to a location where it will also save state for history navigations?
Updated patch as per review comments. Please review it. https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:26: }, 10); On 2015/04/06 17:17:15, Nate Chapin wrote: > Is the 10ms delay required here? Could we get away with a 0ms setTimeout? Done. Code is removed. https://codereview.chromium.org/1063573002/diff/1/LayoutTests/http/tests/navi... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:35: // Click 'Push State' button twice On 2015/04/06 17:17:15, Nate Chapin wrote: > Does this test work with direct history.pushState() and history.back() calls? I > don't think we need a user gesture to test the correct behavior, so eventSender > is probably overkill. Done. eventSender dependency is removed. https://codereview.chromium.org/1063573002/diff/1/Source/core/loader/FrameLoa... File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1063573002/diff/1/Source/core/loader/FrameLoa... Source/core/loader/FrameLoader.cpp:310: saveScrollState(); On 2015/04/06 17:17:15, Nate Chapin wrote: > I believe there's a saveScrollState() call in FrameLoader::loadInSameDocument, > which saves scroll state for non-history same document navigations. Could we > move that to a location where it will also save state for history navigations? Done.
Code change looks good, just a few nitpicks on the test. https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:6: <button id="b">Back</button> Nit: I don't think these buttons are needed any more. https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:14: history.pushState({}, '', window.location.href.split("#")[0] + "#a"); Nit: You shouldn't need to parse the url, setting the url to "#a" should resolve to the same thing. https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:18: document.getElementById('log').innerHTML = 'scrollY should be ' + scrollOffsetY + ': ' + (window.scrollY==scrollOffsetY ? "PASS" : "FAIL"); Does this fail without your fix? I would expect the history.back() to scroll asynchronously.
Please review latest patch https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:6: <button id="b">Back</button> On 2015/04/07 18:17:09, Nate Chapin wrote: > Nit: I don't think these buttons are needed any more. Done. https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:14: history.pushState({}, '', window.location.href.split("#")[0] + "#a"); On 2015/04/07 18:17:09, Nate Chapin wrote: > Nit: You shouldn't need to parse the url, setting the url to "#a" should resolve > to the same thing. Done. https://codereview.chromium.org/1063573002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:18: document.getElementById('log').innerHTML = 'scrollY should be ' + scrollOffsetY + ': ' + (window.scrollY==scrollOffsetY ? "PASS" : "FAIL"); On 2015/04/07 18:17:09, Nate Chapin wrote: > Does this fail without your fix? I would expect the history.back() to scroll > asynchronously. On windows sometime it fails while manual testing. Asynchronous implementation is added.
https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:20: document.getElementById('log').innerHTML = 'scrollY should be ' + scrollOffsetY + ': ' + (window.scrollY==scrollOffsetY ? "PASS" : "FAIL"); Use shouldBe version instead of using getElementById(...). Please refer to [1] [1] https://docs.google.com/document/d/1Yl4SnTLBWmY1O99_BTtQvuoffP8YM9HZx2YPkEsad...
https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:22: }, 100); 100ms is way too long. Ideally this should be called with 0ms.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Please review latest patch. https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:20: document.getElementById('log').innerHTML = 'scrollY should be ' + scrollOffsetY + ': ' + (window.scrollY==scrollOffsetY ? "PASS" : "FAIL"); On 2015/04/10 09:00:52, vivekg_ wrote: > Use shouldBe version instead of using getElementById(...). Please refer to [1] > > [1] > https://docs.google.com/document/d/1Yl4SnTLBWmY1O99_BTtQvuoffP8YM9HZx2YPkEsad... Done. https://codereview.chromium.org/1063573002/diff/40001/LayoutTests/http/tests/... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:22: }, 100); On 2015/04/10 17:32:31, Nate Chapin wrote: > 100ms is way too long. Ideally this should be called with 0ms. Done.
LGTM with a couple nits before landing https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:5: <button id="a">Push State</button> Nit: Remove this line. https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:21: testRunner.notifyDone(); Nit: this should have an "if (window.testRunner)"
Patch is updated as per review comments. https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... File LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html (right): https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:5: <button id="a">Push State</button> On 2015/04/10 19:57:03, Nate Chapin wrote: > Nit: Remove this line. Done. https://codereview.chromium.org/1063573002/diff/100001/LayoutTests/http/tests... LayoutTests/http/tests/navigation/same-document-scroll-position-restore-pushstate.html:21: testRunner.notifyDone(); On 2015/04/10 19:57:03, Nate Chapin wrote: > Nit: this should have an "if (window.testRunner)" Done.
Patch is updated as per review comments.
The CQ bit was checked by abhijeet.k@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1063573002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063573002/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193603
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/1219923007/ by abhijeet.k@samsung.com. The reason for reverting is: Reason for revert: Reverting because of regression on Windows 8. Refer bug url https://code.google.com/p/chromium/issues/detail?id=500453. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
