Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541513004:
Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. (Closed)
DescriptionEnsure scrollToFragmentAnchor called before restoreScrollPositionAndViewState.
This issue is caused by FrameLoader::restoreScrollPositionAndViewState
being called by FrameView::setContentsSize when reloading.
restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which
clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position.
In this patch we move scrollToFragmentAnchor to setContentsSize before
restoreScrollPositionAndViewState.
BUG=656658
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/6d072f7c3b55dbe96358cb9623dfce5e756bad54
Cr-Commit-Position: refs/heads/master@{#436682}
Patch Set 1 : move code to linux #Patch Set 2 : move test to browser test #Patch Set 3 : revert navigation_controller_impl_browsertest.cc style #
Total comments: 1
Patch Set 4 : bokan@ comments addressed #
Total comments: 3
Patch Set 5 : bokan@ comments addressed #Patch Set 6 : creis@ comments addressed #
Total comments: 1
Patch Set 7 : add scrollToFragmentAnchor back to performPostLayoutTasks #
Messages
Total messages: 41 (28 generated)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we add a check for document.isLoadCompleted, only call the restoreScrollPositionAndViewState when document.isLoadCompleted in setContentsSize. BUG=656658 ========== to ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 ========== to ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
PTAL
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2541513004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2541513004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:649: scrollToFragmentAnchor(); I think you can remove it from performPostLayoutTasks.
Also, the title is wrong, you're not preventing restoreScrollPositionAndViewState. You're scrolling to the fragment before restoring history scroll position.
Description was changed from ========== Prevent restoreScrollPositionAndViewState called in setContentsSize when loading This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure scrollToFragmentAnch called before restoreScrollPositionAndViewState when reload This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2016/12/02 17:12:21, bokan wrote: > Also, the title is wrong, you're not preventing > restoreScrollPositionAndViewState. You're scrolling to the fragment before > restoring history scroll position. Updated, PTAL.
Description was changed from ========== Ensure scrollToFragmentAnch called before restoreScrollPositionAndViewState when reload This issue is cause by FrameView::setContentsSize called FrameLoader::restoreScrollPositionAndViewState when reloading the page and restoreScrollPositionAndViewState called ScrollableArea::setScrollOffset that clear FrameView::m_fragmentAnchor. That casuse FrameView::scrollToFragmentAnchor had no m_fragmentAnchor to scroll to. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameView::setContentsSize being called by FrameLoader::restoreScrollPositionAndViewState when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
I tweaked the commit message a bit but lgtm.
Description was changed from ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameView::setContentsSize being called by FrameLoader::restoreScrollPositionAndViewState when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor. In this patch we call scrollToFragmentAnchor before restoreScrollPositionAndViewState in setContentsSize. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameLoader::restoreScrollPositionAndViewState being called by FrameView::setContentsSize when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position. In this patch we move scrollToFragmentAnchor to setContentsSize before restoreScrollPositionAndViewState. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
chaopeng@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content
Thanks! LGTM with minor nits. https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1339: // Verify that reload page with url anchor scroll to correct position. nit: s/reload/reloading a/ nit: s/scroll/scrolls/ https://codereview.chromium.org/2541513004/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1351: // reload Minor nit: Capitalize and end with period. https://codereview.chromium.org/2541513004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2541513004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:648: // restoreScrollPositionAndViewState when reload nit: when reloading. nit: End with period.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2541513004/#ps180001 (title: "creis@ comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2541513004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/2541513004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2382: scrollToFragmentAnchor(); Ok, leaving this in sgtm.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2541513004/#ps200001 (title: "add scrollToFragmentAnchor back to performPostLayoutTasks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481043499609210,
"parent_rev": "5543db75953bee7c88caa42e171e9a5e4513d19d", "commit_rev":
"f08d1b2707fa987ac2428208c9cf66677c906a6c"}
Message was sent while issue was closed.
Description was changed from ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameLoader::restoreScrollPositionAndViewState being called by FrameView::setContentsSize when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position. In this patch we move scrollToFragmentAnchor to setContentsSize before restoreScrollPositionAndViewState. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameLoader::restoreScrollPositionAndViewState being called by FrameView::setContentsSize when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position. In this patch we move scrollToFragmentAnchor to setContentsSize before restoreScrollPositionAndViewState. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameLoader::restoreScrollPositionAndViewState being called by FrameView::setContentsSize when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position. In this patch we move scrollToFragmentAnchor to setContentsSize before restoreScrollPositionAndViewState. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure scrollToFragmentAnchor called before restoreScrollPositionAndViewState. This issue is caused by FrameLoader::restoreScrollPositionAndViewState being called by FrameView::setContentsSize when reloading. restoreScrollPositionAndViewState calls ScrollableArea::setScrollOffset which clears FrameView::m_fragmentAnchor so the DIV scroll to incorrect position. In this patch we move scrollToFragmentAnchor to setContentsSize before restoreScrollPositionAndViewState. BUG=656658 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/6d072f7c3b55dbe96358cb9623dfce5e756bad54 Cr-Commit-Position: refs/heads/master@{#436682} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d072f7c3b55dbe96358cb9623dfce5e756bad54 Cr-Commit-Position: refs/heads/master@{#436682} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
