Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262373003:
Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds (Closed)
|
Created:
4 years, 4 months ago by sunyunjia Modified:
4 years, 3 months ago CC:
chromium-reviews, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClip PaintLayerScrollableArea::scrollIntoView return to layer bounds
In current implementation, the automatic scroll is always trying to make the
focused element visible. However, when the focused element is attached to a
smaller parent/ancestor, the browser is still trying to scroll the inner
element, causing extra space outside the parent/ancestor(or stuff of the inner
element hidden by its ancestor) scrolled by the browser.
In this patch, we only scroll the intersection of the focused element and its
parent to visible.
BUG=437025
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:
linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/29cea456ef3562dd7364e8e6e80aaddc0365a516
Cr-Commit-Position: refs/heads/master@{#415082}
Patch Set 1 #Patch Set 2 : Solve the intrinsic problem and pass the test #
Total comments: 1
Patch Set 3 : Solve the return in LayoutBox #Patch Set 4 : indentation #
Total comments: 6
Patch Set 5 : Fix the code style #
Total comments: 1
Messages
Total messages: 41 (30 generated)
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Initial attempt First attempt BUG=437025 ========== to ========== Initial attempt First attempt BUG=437025 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...
Description was changed from ========== Initial attempt First attempt BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org, dtapuska@chromium.org
Hello, please take a look at the patch. Thanks!
Approach looks fine to me. Some notes: Please wrap the CL description at 80 columns. The first line should also be the CL subject again. Once the CL is committed, only the description is used in the commit so the subject needs to be copied. (Sorry, your tooling should be improved here). See https://codereview.chromium.org/2228093003/ or other CLs for example. Also, please make the subject/first line more descriptive. Instead of "Solve the unnecessary scroll problem" something like "Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds". This CL will also need a test. You'll probably want to add a layout test and make sure it fails without your patch, passes with it. See https://codereview.chromium.org/1860273002 for an example with a similar bug. See https://www.chromium.org/developers/testing/webkit-layout-tests and https://www.chromium.org/blink/coding-style/layout-test-style-guidelines for info on writing tests. Note, the example CL I gave you above is a bit old, today we prefer to use testharness.js for testing so the test might look a little different but not too much. E.g. here's a CL with a testharness.js test: https://codereview.chromium.org/2269483002/ https://codereview.chromium.org/2262373003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2262373003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1419: return LayoutRect(box().localToAbsoluteQuad(FloatQuad(FloatRect(intersection(layerBounds, localExposeRect))), UseTransforms).boundingBox()); Taking the intersection now means we might end up with an empty rect (this can happen if the layer isn't being drawn or other edge cases). You should probably handle that in LayoutBox::scrollRectToVisible by returning if the returned |newRect| is empty.
Description was changed from ========== In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux: linux_layout_tests_slimming_paint_v2 ==========
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Added a test and dealt with the empty rect case in LayoutBox. PTAL, Thanks!
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 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.
Test looks good, just some style nits and final comments. https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html (right): https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html:4: <body> Nit: remove <body> https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html:5: <div id = 'space1' style = 'height: 200px'> </div> If you're just using this for space, there's better ways: Give the container a margin-top style Or Make container position: absolute; top: 200px https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html:6: <div id = 'container' style = 'height: 300px; overflow: auto;' > Nit: no spaces between attribute, =, and value. i.e. id = 'container' should be id='container' Also, please decide on either single or double quotes for attribute values and use them everywhere. (double quotes are probably more common for attribute values) https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html:7: <select id="content" multiple="multiple" style = 'height: 600px'> Nit: Please indent the <select> https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:575: if (newRect == LayoutRect()) use newRect.isEmpty() instead https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2262373003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1410: LayoutRect PaintLayerScrollableArea::scrollIntoView(const LayoutRect& rect, const ScrollAlignment& alignX, const ScrollAlignment& alignY, ScrollType scrollType) Please add a comment in the header file for this method explaining it's return value. e.g. "Returns the new position, after scrolling, of the given rect in absolute coordinates, clipped by the parent's client rect."
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) 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...
Fixed the nits. PTAL, thanks!
lgtm (just make sure the test still fails without the fix). https://codereview.chromium.org/2262373003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html (right): https://codereview.chromium.org/2262373003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/scrolling/scroll-into-view-small-size-ancestor.html:24: var content = document.getElementById("content"); Just FYI, single quotes in javascript are fairly common so they were fine. But double quotes are fine too so no need to change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sunyunjia@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux: linux_layout_tests_slimming_paint_v2 ========== to ========== Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux: linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux: linux_layout_tests_slimming_paint_v2 ========== to ========== Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds In current implementation, the automatic scroll is always trying to make the focused element visible. However, when the focused element is attached to a smaller parent/ancestor, the browser is still trying to scroll the inner element, causing extra space outside the parent/ancestor(or stuff of the inner element hidden by its ancestor) scrolled by the browser. In this patch, we only scroll the intersection of the focused element and its parent to visible. BUG=437025 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux: linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/29cea456ef3562dd7364e8e6e80aaddc0365a516 Cr-Commit-Position: refs/heads/master@{#415082} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/29cea456ef3562dd7364e8e6e80aaddc0365a516 Cr-Commit-Position: refs/heads/master@{#415082} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
