Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2336013002:
ScrollRectToVisible should bubble up through its layout container. (Closed)
DescriptionScrollRectToVisible should bubble up through its layout container.
Previously, ScrollRectToVisible bubbles up through its dom parent. It works
fine when its layout container is its dom parent, but this is not the case for
absolute-positioned elements. Simply scrolling the dom parent of an
absolute-positioned element wouldn't put the desired rect into view. This patch
changes the behavior by bubbling up through its layout parent.
BUG=645841
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/57da1c75484e5a3f0af03d038c43e6f23e1549e8
Cr-Commit-Position: refs/heads/master@{#419197}
Patch Set 1 #Patch Set 2 : Modify the test for Mac #
Total comments: 2
Patch Set 3 : maximum==minimum #
Total comments: 2
Patch Set 4 : Use LayoutBox::canBeProgramaticallyScrolled #Patch Set 5 : Bubbling up through the layout container. #
Total comments: 1
Patch Set 6 : Use containingBox() instead. #Patch Set 7 : Improve the visual effect of the test. #
Messages
Total messages: 66 (46 generated)
Description was changed from ========== Renable autoscrolling for the element with absolute position and a parent that has overflowclip. BUG=645841 ========== to ========== Renable autoscrolling for the element with absolute position and a parent that has overflowclip. BUG=645841 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 sunyunjia@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...
Description was changed from ========== Renable autoscrolling for the element with absolute position and a parent that has overflowclip. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Preserve PaintLayerScrollableArea::scrollIntoView return as input rect when scroll is not allowed. Previously, PaintLayerScrollableArea::scrollIntoView returns the intersection of the input rect and the layerbound. However, when the current layer is not scrollable (e.g, with absolute position), and the input rect is out of view, the return would be zero, preventing its ancestor from scrolling. In this patch, we return the original input rect instead of the intersection when the layer is not scrollable. BUG=645841 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
PTAL, thanks!
https://codereview.chromium.org/2336013002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scrolling/absolute-position-overflow-clip.html (right): https://codereview.chromium.org/2336013002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scrolling/absolute-position-overflow-clip.html:18: document.getElementById("mydiv").appendChild(focusable); The test would be more clear if you didn't dynamically create elements. e.g. Instead of the 500 <br> elements above, just add one div with `style="height:2000px"` to the HTML above. And just add an <input> element below that. https://codereview.chromium.org/2336013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2336013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1417: if (scrollPositionDouble() + roundedIntSize(r.location()) != scrollPositionDouble()) There's no need to get this far. Just check at the beginning of the method if maximumScrollPosition == minimumScrollPosition and return rect if so.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
All done. PTAL, thanks!
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...
https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1420: return LayoutRect(box().localToAbsoluteQuad(FloatQuad(FloatRect(intersection(layerBounds, localExposeRect))), UseTransforms).boundingBox()); Sorry, probably should have asked this before but I don't understand how this situation arises (at least in your test case and the bug report). How come the intersection results in an empty rect? In an unscrollable layer, shouldn't layerBounds contain the localExposeRect? Taking your test as the example, the layer should be the position: absolute div and since it has no explicit height, the box should grow to accommodate the children's height. So I would expect layerBounds and the expose rect to be (roughly) the same.
https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1420: return LayoutRect(box().localToAbsoluteQuad(FloatQuad(FloatRect(intersection(layerBounds, localExposeRect))), UseTransforms).boundingBox()); On 2016/09/13 15:32:29, bokan wrote: > Sorry, probably should have asked this before but I don't understand how this > situation arises (at least in your test case and the bug report). How come the > intersection results in an empty rect? In an unscrollable layer, shouldn't > layerBounds contain the localExposeRect? > > Taking your test as the example, the layer should be the position: absolute div > and since it has no explicit height, the box should grow to accommodate the > children's height. So I would expect layerBounds and the expose rect to be > (roughly) the same. As I investigated, for my test case and the website in the bug report, seems the layerBounds box doesn't grow to accommodate the children's height due to "absolute" position (you can try document.body.offsetHeight at the bug webpage). It just ignores its "absolute" children when calculating size (or maybe we should solve this intrinsic problem?). When the element to be scrolled is out of view and scroll is not allowed, the intersection will be empty.
On 2016/09/13 16:00:44, sunyunjia wrote: > https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1420: return > LayoutRect(box().localToAbsoluteQuad(FloatQuad(FloatRect(intersection(layerBounds, > localExposeRect))), UseTransforms).boundingBox()); > On 2016/09/13 15:32:29, bokan wrote: > > Sorry, probably should have asked this before but I don't understand how this > > situation arises (at least in your test case and the bug report). How come the > > intersection results in an empty rect? In an unscrollable layer, shouldn't > > layerBounds contain the localExposeRect? > > > > Taking your test as the example, the layer should be the position: absolute > div > > and since it has no explicit height, the box should grow to accommodate the > > children's height. So I would expect layerBounds and the expose rect to be > > (roughly) the same. > > As I investigated, for my test case and the website in the bug report, > seems the layerBounds box doesn't grow to accommodate the children's > height due to "absolute" position (you can try > document.body.offsetHeight at the bug webpage). It just ignores its > "absolute" children when calculating size (or maybe we should solve > this intrinsic problem?). When the element to be scrolled is out of > view and scroll is not allowed, the intersection will be empty. Ah, you're right. That's where my confusion came from. In that case, I think it'd be best to use LayoutBox::canBeProgramaticallyScrolled() which has several checks for scrollability. Use that rather than the minimum == maximum check.
On 2016/09/13 16:12:30, bokan wrote: > On 2016/09/13 16:00:44, sunyunjia wrote: > > > https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp > (right): > > > > > https://codereview.chromium.org/2336013002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1420: return > > > LayoutRect(box().localToAbsoluteQuad(FloatQuad(FloatRect(intersection(layerBounds, > > localExposeRect))), UseTransforms).boundingBox()); > > On 2016/09/13 15:32:29, bokan wrote: > > > Sorry, probably should have asked this before but I don't understand how > this > > > situation arises (at least in your test case and the bug report). How come > the > > > intersection results in an empty rect? In an unscrollable layer, shouldn't > > > layerBounds contain the localExposeRect? > > > > > > Taking your test as the example, the layer should be the position: absolute > > div > > > and since it has no explicit height, the box should grow to accommodate the > > > children's height. So I would expect layerBounds and the expose rect to be > > > (roughly) the same. > > > > As I investigated, for my test case and the website in the bug report, > > seems the layerBounds box doesn't grow to accommodate the children's > > height due to "absolute" position (you can try > > document.body.offsetHeight at the bug webpage). It just ignores its > > "absolute" children when calculating size (or maybe we should solve > > this intrinsic problem?). When the element to be scrolled is out of > > view and scroll is not allowed, the intersection will be empty. > > Ah, you're right. That's where my confusion came from. In that case, I think > it'd be best to use LayoutBox::canBeProgramaticallyScrolled() which has several > checks for scrollability. Use that rather than the minimum == maximum check. Oh, and add a comment above it explaining why we're returning if there's no scrolling. (To avoid clipping the returned rect to the layer bounds).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Done. PTAL, thanks!
lgtm! thanks!
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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
lgtm to the minimum == maximum check
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/2336013002/#ps120001 (title: "Bubbling up through the layout parent.")
The CQ bit was unchecked by sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
The CQ bit was unchecked by sunyunjia@chromium.org
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...
Description was changed from ========== Preserve PaintLayerScrollableArea::scrollIntoView return as input rect when scroll is not allowed. Previously, PaintLayerScrollableArea::scrollIntoView returns the intersection of the input rect and the layerbound. However, when the current layer is not scrollable (e.g, with absolute position), and the input rect is out of view, the return would be zero, preventing its ancestor from scrolling. In this patch, we return the original input rect instead of the intersection when the layer is not scrollable. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== ScrollRectToVisible should bubble up through its layout container. Previously, ScrollRectToVisible bubbles up through its dom parent. It works fine when its layout container is its dom parent, but this is not the case for absolute-positioned elements. Simply scrolling the dom parent of an absolute-positioned element wouldn't put the desired rect into view. This patch changes the behavior by bubbling up through its layout parent. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@chromium.org changed reviewers: + skobes@chromium.org
Thanks! This looks better to me. +skobes though as he's more familiar with layout structures. Steve: it seems scrollRectToVisible doesn't account correctly for out-of-flow elements since it uses parent(). Does this look ok? https://codereview.chromium.org/2336013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2336013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:579: parentBox = container()->enclosingBox(); I think we could use containingBlock rather than container here and then you wouldn't need enclosingBox (since block is a box).
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 change seems reasonable. However the bug says this is a regression since M54... do we know what change broke it? How did it work before?
On 2016/09/16 01:43:48, skobes wrote: > The change seems reasonable. However the bug says this is a regression since > M54... do we know what change broke it? How did it work before? That might be a bit hard to explain but I will try. tl;dr, the original solution would work fine with the web page in the bug report, but the logic was not correct so it would still fail the test case in this patch (you can try loading it in Chrome and compare with Firefox). The previous patch fixed some other problem but revealed this hidden bug. Original solution: scroll the desired rect all the way up through the dom tree. Previous patch: scroll the desired rect all the way up through the dom tree, clipping it along the way if the parent's size is smaller than the rect being scrolled. This patch: scroll the desired rect all the way up through the layout tree, clipping it along the way if the parent's size is smaller than the rect being scrolled. The bug happens when the rect being scrolled is absolute-positioned. As the dom parent's layoutbox does not contain the absolute-positioned element, the parent's layoutbox's size may even be zero. Clipping the rect may result an empty rect to be scrolled, that's how the previous patch fails. The original solution does not clip so we always have something to scroll. However, the absolute-positioned element doesn't scroll with its dom parent. The parent thought it's scrolled but the absolute-positioned element still stays at the original position, and at the next level, the rect would refer to some other relative-positioned element in the parent rather than the actual rect we want to scroll.
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.
lgtm On 2016/09/16 13:57:06, sunyunjia wrote: > On 2016/09/16 01:43:48, skobes wrote: > > The change seems reasonable. However the bug says this is a regression since > > M54... do we know what change broke it? How did it work before? > > That might be a bit hard to explain but I will try. tl;dr, the original solution > would work fine with the web page in the bug report, but the logic was not > correct so it would still fail the test case in this patch (you can try loading > it in Chrome and compare with Firefox). The previous patch fixed some other > problem but revealed this hidden bug. > > Original solution: scroll the desired rect all the way up through the dom tree. > Previous patch: scroll the desired rect all the way up through the dom tree, > clipping it along the way if the parent's size is smaller than the rect being > scrolled. > This patch: scroll the desired rect all the way up through the layout tree, > clipping it along the way if the parent's size is smaller than the rect being > scrolled. > > The bug happens when the rect being scrolled is absolute-positioned. As the dom > parent's layoutbox does not contain the absolute-positioned element, the > parent's layoutbox's size may even be zero. Clipping the rect may result an > empty rect to be scrolled, that's how the previous patch fails. The original > solution does not clip so we always have something to scroll. However, the > absolute-positioned element doesn't scroll with its dom parent. The parent > thought it's scrolled but the absolute-positioned element still stays at the > original position, and at the next level, the rect would refer to some other > relative-positioned element in the parent rather than the actual rect we want to > scroll. Got it. Thanks for the thorough explanation.
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/2336013002/#ps160001 (title: "Improve the visual effect of the test.")
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 ========== ScrollRectToVisible should bubble up through its layout container. Previously, ScrollRectToVisible bubbles up through its dom parent. It works fine when its layout container is its dom parent, but this is not the case for absolute-positioned elements. Simply scrolling the dom parent of an absolute-positioned element wouldn't put the desired rect into view. This patch changes the behavior by bubbling up through its layout parent. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== ScrollRectToVisible should bubble up through its layout container. Previously, ScrollRectToVisible bubbles up through its dom parent. It works fine when its layout container is its dom parent, but this is not the case for absolute-positioned elements. Simply scrolling the dom parent of an absolute-positioned element wouldn't put the desired rect into view. This patch changes the behavior by bubbling up through its layout parent. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== ScrollRectToVisible should bubble up through its layout container. Previously, ScrollRectToVisible bubbles up through its dom parent. It works fine when its layout container is its dom parent, but this is not the case for absolute-positioned elements. Simply scrolling the dom parent of an absolute-positioned element wouldn't put the desired rect into view. This patch changes the behavior by bubbling up through its layout parent. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== ScrollRectToVisible should bubble up through its layout container. Previously, ScrollRectToVisible bubbles up through its dom parent. It works fine when its layout container is its dom parent, but this is not the case for absolute-positioned elements. Simply scrolling the dom parent of an absolute-positioned element wouldn't put the desired rect into view. This patch changes the behavior by bubbling up through its layout parent. BUG=645841 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/57da1c75484e5a3f0af03d038c43e6f23e1549e8 Cr-Commit-Position: refs/heads/master@{#419197} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/57da1c75484e5a3f0af03d038c43e6f23e1549e8 Cr-Commit-Position: refs/heads/master@{#419197} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
