Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576963002:
Scroll the position into view when the content is not visible. (Closed)
|
Created:
4 years ago by sunyunjia Modified:
3 years, 11 months ago Reviewers:
bokan CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, dshwang, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScroll the position into view when the content is not visible.
Previously, we implement scrollIntoView by scrolling the intersection of the
viewport and the content. However, when the content is not visible in the
viewport, e.g. outside boundary and the viewport's overflow attribute is
hidden, the intersection would be empty and the browser would stop the
scrolling logic. In this case, we presume the webpage wants to scroll the
position of the content but for some reason doesn't want to show it. In this
patch, we scroll the content's position for this special case.
BUG=673396
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/961e2d024412588edffbef5b3f6dfdfd2646761f
Cr-Commit-Position: refs/heads/master@{#441469}
Patch Set 1 #Patch Set 2 : typo #
Total comments: 4
Patch Set 3 : Move the logic to a static helper function. #
Total comments: 2
Patch Set 4 : Scroll the content rather than the viewport. #
Total comments: 2
Patch Set 5 : Merge the duplicate blocks. #
Total comments: 1
Patch Set 6 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into anchor-pr1 #Patch Set 7 : remove the if condition. #
Messages
Total messages: 52 (40 generated)
Description was changed from ========== fix-bug BUG= ========== to ========== fix-bug BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by sunyunjia@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_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== fix-bug BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Scroll the viewport into view when the content inside is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the viewport but for some reason doesn't want to show the content inside. In this patch, we scroll the viewport for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
The CQ bit was checked by sunyunjia@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 sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #2 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, thanks!
https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1650: .localToAbsoluteQuad(FloatQuad(FloatRect(intersection( this long call chain is really obscuring the intent here. Could you put this into a static helper above, something like: LayoutRect localToAbsolute(LayoutBox, LayoutRect) https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1674: if (intersect.isEmpty() && !layerBounds.isEmpty() && Hmm, doesn't this break the case that we introduced the intersection logic for? Namely crbug.com/437025
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by sunyunjia@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
PTAL, thanks! https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1650: .localToAbsoluteQuad(FloatQuad(FloatRect(intersection( On 2016/12/15 21:31:19, bokan wrote: > this long call chain is really obscuring the intent here. Could you put this > into a static helper above, something like: > > LayoutRect localToAbsolute(LayoutBox, LayoutRect) Done. https://codereview.chromium.org/2576963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1674: if (intersect.isEmpty() && !layerBounds.isEmpty() && On 2016/12/15 21:31:19, bokan wrote: > Hmm, doesn't this break the case that we introduced the intersection logic for? > Namely crbug.com/437025 I have to say that the bug itself is an edge case. It is trying to scroll an invisible element into view (the element can not be scrolled into its closest scrollable area). In this case, the intersection of the layerbounds and localExposeRect is empty. We assume the page just want to scroll the position of the element into view, and doesn't care whether the element is visible.
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1658: return localToAbsolute(box(), localExposeRect); This changed from layerBounds in your previous patch, accident?
Description was changed from ========== Scroll the viewport into view when the content inside is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the viewport but for some reason doesn't want to show the content inside. In this patch, we scroll the viewport for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Scroll the position into view when the content is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the position of the content but for some reason doesn't want to show it. In this patch, we scroll the content's position for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1658: return localToAbsolute(box(), localExposeRect); On 2016/12/19 20:29:36, bokan wrote: > This changed from layerBounds in your previous patch, accident? Not an accident, I just feel it makes more sense to scroll the position of the element. What's your opinion?
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: This issue passed the CQ dry run.
On 2016/12/19 21:35:09, sunyunjia wrote: > https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2576963002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1658: return > localToAbsolute(box(), localExposeRect); > On 2016/12/19 20:29:36, bokan wrote: > > This changed from layerBounds in your previous patch, accident? > > Not an accident, I just feel it makes more sense to scroll the position of the > element. What's your opinion? It feels a little hard to reason about...if the box isn't in view we try to scroll the whole box but if it's at least a little in the view we clip it to the view. I think interoperability should be our guide here: could you check how Firefox handles the case in this bug and the previous one (where the exposeRect is overflowing the layerBounds). If we match them then this is fine and I'll stamp.
https://codereview.chromium.org/2576963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1654: LayoutRect intersect = This part is duplicated below. I think the three lines following this block are no-op if newScrollOffset == oldScrollOffset so I don't think we need this block at all? Otherwise just wrap them in `if (newScrollOffset != oldScrollOffset)` instead and remove this block.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
https://codereview.chromium.org/2576963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1654: LayoutRect intersect = On 2016/12/20 15:43:56, bokan wrote: > This part is duplicated below. I think the three lines following this block are > no-op if newScrollOffset == oldScrollOffset so I don't think we need this block > at all? Otherwise just wrap them in `if (newScrollOffset != oldScrollOffset)` > instead and remove this block. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % suggestion https://codereview.chromium.org/2576963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2576963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1653: if (newScrollOffset != oldScrollOffset) { I think there's no reason to keep the `if`, it's still correct if newScrollOffset == oldScrollOffset, it'll just move by 0
The CQ bit was checked by sunyunjia@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: This issue passed the CQ dry run.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2576963002/#ps160001 (title: "remove the if condition.")
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": 160001, "attempt_start_ts": 1483564679324770,
"parent_rev": "9fee92d475d34a27da2191c34fc3b7efebb54d9f", "commit_rev":
"0f39927fe6f201fa81f3aaa1c2b5fd14ede618f6"}
Message was sent while issue was closed.
Description was changed from ========== Scroll the position into view when the content is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the position of the content but for some reason doesn't want to show it. In this patch, we scroll the content's position for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Scroll the position into view when the content is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the position of the content but for some reason doesn't want to show it. In this patch, we scroll the content's position for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2576963002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Scroll the position into view when the content is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the position of the content but for some reason doesn't want to show it. In this patch, we scroll the content's position for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2576963002 ========== to ========== Scroll the position into view when the content is not visible. Previously, we implement scrollIntoView by scrolling the intersection of the viewport and the content. However, when the content is not visible in the viewport, e.g. outside boundary and the viewport's overflow attribute is hidden, the intersection would be empty and the browser would stop the scrolling logic. In this case, we presume the webpage wants to scroll the position of the content but for some reason doesn't want to show it. In this patch, we scroll the content's position for this special case. BUG=673396 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/961e2d024412588edffbef5b3f6dfdfd2646761f Cr-Commit-Position: refs/heads/master@{#441469} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/961e2d024412588edffbef5b3f6dfdfd2646761f Cr-Commit-Position: refs/heads/master@{#441469} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
