Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857583007:
[RLS] Don't add to ScrollableAreaSet if the size is zero (Closed)
Description[RLS] Don't add to ScrollableAreaSet if the size is zero
Add ScrollableArea only if the box size is not zero and
there is a horizontal or vertical overflow scroll.
BUG=711474
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2857583007
Cr-Commit-Position: refs/heads/master@{#487525}
Committed: https://chromium.googlesource.com/chromium/src/+/03a08a02a028bdc0042b34cf36d229f02f9ce5fc
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add box size check for determining scrollable area #
Total comments: 2
Patch Set 3 : address review comments #
Messages
Total messages: 40 (25 generated)
Description was changed from ========== RLS BUG= ========== to ========== RLS BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== RLS BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Add ScrollableArea if it's scrollable. Add ScrollableArea if it's scrollable. If it's not scrollable don't add the scrollable area to scrollablearea set. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
sataya.m@samsung.com changed reviewers: + skobes@chromium.org, srirama.m@samsung.com
The CQ bit was checked by sataya.m@samsung.com 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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1780: if (scrolls_overflow_ && frame_view->IsScrollable()) { IsScrollable is calling GetScrollingReasons and that internally checks for 4 conditions. I think what is missing here is the first condition (If there is an actual overflow). Ideally adding the first condition alone here should work but it is not. Incase of NO-RLS flow contents_size is coming {0, 0} where as in RLS flow, contents_size is coming as {1, 8}. So i think we need to debug more to find out how size is coming as {1, 8}.
On 2017/05/05 06:21:23, Srirama wrote: > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1780: if > (scrolls_overflow_ && frame_view->IsScrollable()) { > IsScrollable is calling GetScrollingReasons and that internally checks for 4 > conditions. I think what is missing here is the first condition (If there is an > actual overflow). Ideally adding the first condition alone here should work but > it is not. Incase of NO-RLS flow contents_size is coming {0, 0} where as in RLS > flow, contents_size is coming as {1, 8}. So i think we need to debug more to > find out how size is coming as {1, 8}. @skobes, any suggestion from your side?
On 2017/05/19 12:22:48, Srirama wrote: > On 2017/05/05 06:21:23, Srirama wrote: > > > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp > (right): > > > > > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1780: if > > (scrolls_overflow_ && frame_view->IsScrollable()) { > > IsScrollable is calling GetScrollingReasons and that internally checks for 4 > > conditions. I think what is missing here is the first condition (If there is > an > > actual overflow). Ideally adding the first condition alone here should work > but > > it is not. Incase of NO-RLS flow contents_size is coming {0, 0} where as in > RLS > > flow, contents_size is coming as {1, 8}. So i think we need to debug more to > > find out how size is coming as {1, 8}. > > @skobes, any suggestion from your side? Sorry, I'm missing some context on this patch. Why do we want to call FrameView::IsScrollable here?
On 2017/05/19 20:31:41, skobes wrote: > On 2017/05/19 12:22:48, Srirama wrote: > > On 2017/05/05 06:21:23, Srirama wrote: > > > > > > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp > > (right): > > > > > > > > > https://codereview.chromium.org/2857583007/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1780: if > > > (scrolls_overflow_ && frame_view->IsScrollable()) { > > > IsScrollable is calling GetScrollingReasons and that internally checks for 4 > > > conditions. I think what is missing here is the first condition (If there is > > an > > > actual overflow). Ideally adding the first condition alone here should work > > but > > > it is not. Incase of NO-RLS flow contents_size is coming {0, 0} where as in > > RLS > > > flow, contents_size is coming as {1, 8}. So i think we need to debug more to > > > find out how size is coming as {1, 8}. > > > > @skobes, any suggestion from your side? > > Sorry, I'm missing some context on this patch. Why do we want to call > FrameView::IsScrollable here? FrameView::IsScrollable may not be required. As i mentioned in comment#9, may be should we check why the size is coming wrong? Or do you have any other suggestion?
On 2017/05/20 13:11:40, Srirama wrote: > FrameView::IsScrollable may not be required. As i mentioned in comment#9, may be > should we check why the size is coming wrong? > Or do you have any other suggestion? If I understand correctly: - you are trying to fix scrollable-area-frame-zero-size-and-border.html - this test asserts that an iframe with no overflow is not added to the parent frame's ScrollableArea set - with RLS, the iframe is added despite not having overflow Is that about right? In that case the next question would be: what is the has_overflow parameter when PLSA::UpdateScrollableAreaSet is called? If it is false we will not call FrameView::AddScrollableArea. If it is true, what causes it to be true?
On 2017/05/22 14:56:40, skobes wrote: > On 2017/05/20 13:11:40, Srirama wrote: > > FrameView::IsScrollable may not be required. As i mentioned in comment#9, may > be > > should we check why the size is coming wrong? > > Or do you have any other suggestion? > > If I understand correctly: > - you are trying to fix scrollable-area-frame-zero-size-and-border.html > - this test asserts that an iframe with no overflow is not added to the parent > frame's ScrollableArea set > - with RLS, the iframe is added despite not having overflow > > Is that about right? In that case the next question would be: what is the > has_overflow parameter when PLSA::UpdateScrollableAreaSet is called? If it is > false we will not call FrameView::AddScrollableArea. If it is true, what causes > it to be true? sorry for the delayed response. I debugged further. Following is the finding. PLSA::UpdateScrollableAreaSet is getting called with true from PLSA::UpdateAfterLayout(). The reason for "true" is that HasScrollableHorizontalOverflow() and HasScrollableVerticalOverflow() are returning true which are internally dependent on PixelSnappedScrollWidth() and PixelSnappedScrollHeight(). PixelSnappedScrollWidth() is coming as 1 and PixelSnappedScrollHeight() is coming as 8 instead of 0, 0.
On 2017/06/30 13:58:50, Srirama wrote: > sorry for the delayed response. I debugged further. Following is the finding. > PLSA::UpdateScrollableAreaSet is getting called with true from > PLSA::UpdateAfterLayout(). > The reason for "true" is that HasScrollableHorizontalOverflow() and > HasScrollableVerticalOverflow() are returning true which are internally > dependent on PixelSnappedScrollWidth() and PixelSnappedScrollHeight(). > PixelSnappedScrollWidth() is coming as 1 and PixelSnappedScrollHeight() is > coming as 8 instead of 0, 0. Thanks for the update. So the iframe content size is different in RLS, and due to this it is treated as having overflow with RLS but no overflow without RLS, leading to the discrepancy in the ScrollableArea set. Yes it would be good to understand where the content size difference comes from. The height == 8 sounds like the default margin on <body>. But it might also make sense for PLSA::HasScrollableHorizontalOverflow and PLSA::HasScrollableVerticalOverflow to return false when Box().Size() is 0 x 0. If there is no visible content or space for scrollbars then the overflow is not really user-scrollable even if it exists.
The CQ bit was checked by sataya.m@samsung.com 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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by srirama.m@samsung.com 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.
On 2017/07/05 18:43:15, skobes wrote: > On 2017/06/30 13:58:50, Srirama wrote: > > sorry for the delayed response. I debugged further. Following is the finding. > > PLSA::UpdateScrollableAreaSet is getting called with true from > > PLSA::UpdateAfterLayout(). > > The reason for "true" is that HasScrollableHorizontalOverflow() and > > HasScrollableVerticalOverflow() are returning true which are internally > > dependent on PixelSnappedScrollWidth() and PixelSnappedScrollHeight(). > > PixelSnappedScrollWidth() is coming as 1 and PixelSnappedScrollHeight() is > > coming as 8 instead of 0, 0. > > Thanks for the update. So the iframe content size is different in RLS, and due > to this it is treated as having overflow with RLS but no overflow without RLS, > leading to the discrepancy in the ScrollableArea set. > > Yes it would be good to understand where the content size difference comes from. > The height == 8 sounds like the default margin on <body>. > > But it might also make sense for PLSA::HasScrollableHorizontalOverflow and > PLSA::HasScrollableVerticalOverflow to return false when Box().Size() is 0 x 0. > If there is no visible content or space for scrollbars then the overflow is not > really user-scrollable even if it exists. Thanks for the suggestion, for now we have added the Box().Size() check and it solves the problem here. PTAL.
https://codereview.chromium.org/2857583007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2857583007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1018: if (Box().Size().IsZero()) Actually it would be simpler to put this logic inside UpdateScrollableAreaSet, and remove the has_overflow param. Then the methods HasScrollableHorizontalOverflow and HasScrollableVerticalOverflow can be deleted since they are not used elsewhere.
The CQ bit was checked by sataya.m@samsung.com 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. https://codereview.chromium.org/2857583007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2857583007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1018: if (Box().Size().IsZero()) On 2017/07/15 00:44:31, skobes wrote: > Actually it would be simpler to put this logic inside UpdateScrollableAreaSet, > and remove the has_overflow param. Then the methods > HasScrollableHorizontalOverflow and HasScrollableVerticalOverflow can be deleted > since they are not used elsewhere. Done.
Please update the change description to be more clear, like "[RLS] Don't add to ScrollableAreaSet if the size is zero."
Description was changed from ========== [RLS] Add ScrollableArea if it's scrollable. Add ScrollableArea if it's scrollable. If it's not scrollable don't add the scrollable area to scrollablearea set. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/07/17 17:38:15, skobes wrote: > Please update the change description to be more clear, like "[RLS] Don't add to > ScrollableAreaSet if the size is zero." Thanks. Updated description to reflect new changes.
The CQ bit was checked by skobes@chromium.org
lgtm
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": 40001, "attempt_start_ts": 1500393757302690,
"parent_rev": "5ca5cdd5604fa069fa93fbf2bbd0ca2f08dbf692", "commit_rev":
"03a08a02a028bdc0042b34cf36d229f02f9ce5fc"}
Message was sent while issue was closed.
Description was changed from ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [RLS] Don't add to ScrollableAreaSet if the size is zero Add ScrollableArea only if the box size is not zero and there is a horizontal or vertical overflow scroll. BUG=711474 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2857583007 Cr-Commit-Position: refs/heads/master@{#487525} Committed: https://chromium.googlesource.com/chromium/src/+/03a08a02a028bdc0042b34cf36d2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/03a08a02a028bdc0042b34cf36d2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
