Cleanup Fragment Scroll Code |
|||||
Issue descriptionThe fragment scrolling code dates back to ~2008 and has been monkey patched since then. The current incarnation of this code relies on various places in the lifecycle calling into LocalFrameView::ProcessFragmentUrl and LocalFrameView::ScrollToFragment. A few issues I can see that could be cleaned up: 1) ProcessFragmentUrl should only cause the fragment to be set and let layout worrying about scrolling to it. Today it tries to scroll the fragment into view, but because of that it must defer if called when rendering is blocked. This requires us to kick off a layout later if one isn't scheduled which leads to brittle timing issues. 2) Fragment processing is called ad-hoc from Document::UpdateStyleAndLayout in case it was deferred. We can remove the deferal state once #1 is done. 3) Focus is also tied up with ProcessFragmentUrl - it should occur at the same time as scrolling into view.
,
Dec 19 2017
,
Dec 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79a2d86fdc00055f5f0cf76ae517c2694e69d86c commit 79a2d86fdc00055f5f0cf76ae517c2694e69d86c Author: David Bokan <bokan@chromium.org> Date: Thu Dec 21 18:47:13 2017 Revert "Simplify scroll to fragment code" This reverts commit 00710b48b6d06931a868dc6982b0daafd8875560. Reason for revert: Causes script to run during layout: https://crbug.com/796222 Original change's description: > Simplify scroll to fragment code > > This patch simplifies the code to scroll to fragment (i.e. scrolling to > an Element with id="foo" when navigating to http://ex.pl/index.html#foo) > code by moving more of it into LocalFrameView rather than in Document. > > Previously, we kept some state whether rendering was blocked when the > URL fragment was processed and if it was we'd try to perform the scroll > later in the document lifecycle. This patch removes the state tracking > and relies entirely on the scroll happening in PerformPostLayoutTasks if > it can't be done immediately. > > It also moves focusing on the fragment element into the method that > performs the scrolling, tying these together. This appears to have been > the original intent based on the comment in ProcessUrlFragment but > changes behavior slightly. > > Bug: 795381 > Change-Id: Ie7331ab89be9cf32d3d2c86e42132e76569d990f > Reviewed-on: https://chromium-review.googlesource.com/831008 > Reviewed-by: Steve Kobes <skobes@chromium.org> > Commit-Queue: David Bokan <bokan@chromium.org> > Cr-Commit-Position: refs/heads/master@{#524916} TBR=bokan@chromium.org,skobes@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 795381 Change-Id: Ice0151ab6b67bb921fbd2e4809d78922c8d2723d Reviewed-on: https://chromium-review.googlesource.com/840120 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#525752} [modify] https://crrev.com/79a2d86fdc00055f5f0cf76ae517c2694e69d86c/third_party/WebKit/Source/core/dom/Document.cpp [modify] https://crrev.com/79a2d86fdc00055f5f0cf76ae517c2694e69d86c/third_party/WebKit/Source/core/dom/Document.h [modify] https://crrev.com/79a2d86fdc00055f5f0cf76ae517c2694e69d86c/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/79a2d86fdc00055f5f0cf76ae517c2694e69d86c/third_party/WebKit/Source/core/frame/LocalFrameView.h
,
Dec 21 2017
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dbd4eed1920a2aafbc87236db7efb98f108ba063 commit dbd4eed1920a2aafbc87236db7efb98f108ba063 Author: David Bokan <bokan@chromium.org> Date: Tue Apr 17 22:14:05 2018 [Reland] Simplify scroll to fragment code This patch simplifies the code to scroll to fragment (i.e. scrolling to an Element with id="foo" when navigating to http://ex.pl/index.html#foo) code by moving more of it into LocalFrameView rather than in Document. Previously, we kept some state whether rendering was blocked when the URL fragment was processed and if it was we'd try to perform the scroll later in the document lifecycle. This patch removes the state tracking and relies entirely on the scroll happening in PerformPostLayoutTasks if it can't be done immediately. It also moves focusing on the fragment element into the method that performs the scrolling, tying these together. This appears to have been the original intent based on the comment in ProcessUrlFragment but changes behavior slightly. Re-land Note: This was originally reverting because script could run inside the ScriptForbiddenScope in UpdateLayout ( https://crbug.com/796222 ). This patch removes the ScriptForbiddenScope in Document::UpdateStyleAndLayout since both UpdateStyleAndLayoutTree and UpdateLayout are guarded internally by ScriptForbiddenScope. It also narrows the scope of the ScriptForbidden section in UpdateLayout to not include the call to Document::LayoutUpdated and moves the scrolling and focus related methods there. Bug: 795381 , 830881 Change-Id: I571932217a0664f21fae8463d26b2c807a5f565e Reviewed-on: https://chromium-review.googlesource.com/1010264 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Commit-Position: refs/heads/master@{#551499} [modify] https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063/third_party/blink/renderer/core/dom/document.cc [modify] https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063/third_party/blink/renderer/core/dom/document.h [modify] https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063/third_party/blink/renderer/core/frame/local_frame_view.cc [modify] https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063/third_party/blink/renderer/core/frame/local_frame_view.h
,
Apr 18 2018
,
Apr 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/88d0d3a66e49f78140ad6084b69dc185d3225101 commit 88d0d3a66e49f78140ad6084b69dc185d3225101 Author: David Bokan <bokan@chromium.org> Date: Mon Apr 23 20:11:59 2018 [Reland] Simplify scroll to fragment code This patch simplifies the code to scroll to fragment (i.e. scrolling to an Element with id="foo" when navigating to http://ex.pl/index.html#foo) code by moving more of it into LocalFrameView rather than in Document. Previously, we kept some state whether rendering was blocked when the URL fragment was processed and if it was we'd try to perform the scroll later in the document lifecycle. This patch removes the state tracking and relies entirely on the scroll happening in PerformPostLayoutTasks if it can't be done immediately. It also moves focusing on the fragment element into the method that performs the scrolling, tying these together. This appears to have been the original intent based on the comment in ProcessUrlFragment but changes behavior slightly. Re-land Note: This was originally reverting because script could run inside the ScriptForbiddenScope in UpdateLayout ( https://crbug.com/796222 ). This patch removes the ScriptForbiddenScope in Document::UpdateStyleAndLayout since both UpdateStyleAndLayoutTree and UpdateLayout are guarded internally by ScriptForbiddenScope. It also narrows the scope of the ScriptForbidden section in UpdateLayout to not include the call to Document::LayoutUpdated and moves the scrolling and focus related methods there. Bug: 795381 , 830881 Change-Id: I571932217a0664f21fae8463d26b2c807a5f565e Reviewed-on: https://chromium-review.googlesource.com/1010264 Commit-Queue: David Bokan <bokan@chromium.org> Reviewed-by: Steve Kobes <skobes@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551499}(cherry picked from commit dbd4eed1920a2aafbc87236db7efb98f108ba063) Reviewed-on: https://chromium-review.googlesource.com/1024852 Reviewed-by: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#236} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/88d0d3a66e49f78140ad6084b69dc185d3225101/third_party/blink/renderer/core/dom/document.cc [modify] https://crrev.com/88d0d3a66e49f78140ad6084b69dc185d3225101/third_party/blink/renderer/core/dom/document.h [modify] https://crrev.com/88d0d3a66e49f78140ad6084b69dc185d3225101/third_party/blink/renderer/core/frame/local_frame_view.cc [modify] https://crrev.com/88d0d3a66e49f78140ad6084b69dc185d3225101/third_party/blink/renderer/core/frame/local_frame_view.h |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Dec 19 2017