Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1811753003:
Set m_{width,height}AvailableToChildrenChanged when scrollbars appear (Closed)
|
Created:
4 years, 9 months ago by cbiesinger Modified:
4 years, 9 months ago CC:
chromium-reviews, blink-reviews, dshwang, slimming-paint-reviews_chromium.org, blink-reviews-paint_chromium.org, szager1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet m_{width,height}AvailableToChildrenChanged when scrollbars appear
This is a regression from https://codereview.chromium.org/1734203002. Before
that change, we'd just call layoutBlock(true). However, the new approach
just marks for layout and eventually recurses down, so to ensure that
the children get laid out, we have to set
m_{width,height}AvailableToChildrenChanged.
BUG=594465, 593593, 590683, 593624, 593593
Committed: https://crrev.com/fbb25cec59deb052604d63036f1c5baf6efd2a5c
Cr-Commit-Position: refs/heads/master@{#382074}
Patch Set 1 #Patch Set 2 : add a test (minimized from trace viewer) #
Total comments: 5
Patch Set 3 : review comments for test #
Total comments: 1
Patch Set 4 : style issues in test #Patch Set 5 : abspos #
Total comments: 2
Patch Set 6 : review comments #Patch Set 7 : different/better approach #
Total comments: 2
Patch Set 8 : review comment #Messages
Total messages: 43 (19 generated)
Description was changed from ========== When doing the overflow:auto layout marking, we also have to mark children. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to explicitly mark them. BUG=594465,593209,593593,590683,593624,593593 ========== to ========== When doing the overflow:auto layout marking, we also have to mark children. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to explicitly mark them. BUG=594465,593209,593593,590683,593624,593593 ==========
cbiesinger@chromium.org changed reviewers: + eae@chromium.org, leviw@chromium.org
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/20001
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-dynamic-changes.html (right): https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-dynamic-changes.html:2: <title>This test should not have a horizontal scrollbar</title> I think we usually put the pass condition as readable text in the document, i.e. put it in a <p> or something. https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-dynamic-changes.html:3: <html> Wrong order. HTML should contain TITLE. But better just omit the HTML and HEAD tags instead. Normally BODY too, but in this case you set class names on it, so fine. https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:688: delayedLayoutScope->setNeedsLayout(child, LayoutInvalidationReason::ScrollbarChanged); Aren't you going to miss marking out-of-flow positioned descendants that aren't direct children?
https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:688: delayedLayoutScope->setNeedsLayout(child, LayoutInvalidationReason::ScrollbarChanged); On 2016/03/17 19:57:06, mstensho wrote: > Aren't you going to miss marking out-of-flow positioned descendants that aren't > direct children? Um, good question. I'm not familiar with that part of layout actually, for which cases does that happen? Are you suggesting I also iterate over positionedObjects()? (fixed the comments on the testcase)
https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:688: delayedLayoutScope->setNeedsLayout(child, LayoutInvalidationReason::ScrollbarChanged); On 2016/03/17 20:03:03, cbiesinger wrote: > On 2016/03/17 19:57:06, mstensho wrote: > > Aren't you going to miss marking out-of-flow positioned descendants that > aren't > > direct children? > > Um, good question. I'm not familiar with that part of layout actually, for which > cases does that happen? Maybe this (based on the test already in this CL)? <div id="root" class="root" style="position:relative;"> <div> <div id="history" style="position:absolute;"></div> </div> </div> Actually, we may have to wrap the abspos harder: <div id="root" class="root" style="position:relative;"> <div style="width:100px;"> <div style="width:100px;"> <div id="history" style="position:absolute;"></div> </div> </div> </div> I don't remember exactly how Blink decides to descend into children that aren't marked for layout, when the parent is. Nothing should be able to punch through two walls of fixed width, though. :) > Are you suggesting I also iterate over positionedObjects()? That should work. Maybe better introduce a method in LayoutBlock to handle all this mark-children-and-some-other-descendants-for-layout-as-well stuff, then? https://codereview.chromium.org/1811753003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-dynamic-changes.html (right): https://codereview.chromium.org/1811753003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/css3/flexbox/overflow-auto-dynamic-changes.html:49: </html> Please ploink this one too. :)
Adding a width makes this inconclusive (the point is about recalculating the width when we make this change)... but if I remove the width (and keep position relative/absolute) it doesn't even fail in current Chrome. I did add left:0;right:0; to make this work at all. I am hesitant to add that code when I can't find a failing testcase :/ I did update the testcase some more to remove </body>, </html> and an unnecessary id.
Got a failing testcase, the abspos needs to be a sibling of history. It does get fixed by the proposed code. But I don't want to introduce this method to LayoutBlock for two reasons: - The code in PaintLayerScrollableArea operates on LayoutBox so I'd still need separate code for LayoutBox vs LayoutBlock. Conversely it would be weird to add this to LayoutBox because it doesn't know about positionedObjects() - This is, I believe, the only place where we need this functionality, so no point in putting this in such a generic place
New version uploaded, PTAL
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/80001
Description was changed from ========== When doing the overflow:auto layout marking, we also have to mark children. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to explicitly mark them. BUG=594465,593209,593593,590683,593624,593593 ========== to ========== When doing the overflow:auto layout marking, we also have to mark children. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to explicitly mark them. BUG=594465,593593,590683,593624,593593 ==========
lgtm. I don't know any cases where a scrollable is something other than LayoutBlock (but then again I kind of hope that PaintLayerScrollableArea uses LayoutBox because sometimes it really isn't a LayoutBlock...), but I agree with you that this is special code, and keeping it in PaintLayerScrollableArea sounds fine. https://codereview.chromium.org/1811753003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:689: TrackedLayoutBoxListHashSet* positionedDescendants = box().isLayoutBlock() ? toLayoutBlock(box()).positionedObjects() : 0; 0 -> nullptr https://codereview.chromium.org/1811753003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:693: } Superfluous braces.
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com Link to the patchset: https://codereview.chromium.org/1811753003/#ps100001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I thought about this some more. I now think that maybe the real problem is a bug in updateLogicalWidthAndColumnWidth() in that it should return true. Let me follow down that path a bit (suspect that it should check contentWidth() instead of logicalWidth() or something along those lines)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/40002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/40002
mstensho - this is now an entirely different approach which I think works much better. Can you take another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm^2 - this is much better! https://codereview.chromium.org/1811753003/diff/40002/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/40002/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:686: block.scrollbarsChanged(horizontalScrollBarChanged, verticalScrollBarChanged); I'm not going to complain if you squash this into one line, and get rid of the curly braces in the process. :)
The CQ bit was checked by cbiesinger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com Link to the patchset: https://codereview.chromium.org/1811753003/#ps130001 (title: "review comment")
Thanks! https://codereview.chromium.org/1811753003/diff/40002/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1811753003/diff/40002/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:686: block.scrollbarsChanged(horizontalScrollBarChanged, verticalScrollBarChanged); On 2016/03/18 19:04:26, mstensho wrote: > I'm not going to complain if you squash this into one line, and get rid of the > curly braces in the process. :) Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811753003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811753003/130001
Description was changed from ========== When doing the overflow:auto layout marking, we also have to mark children. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to explicitly mark them. BUG=594465,593593,590683,593624,593593 ========== to ========== When doing the overflow:auto layout marking, we also have to note the width available to children changed. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to set m_{width,height}AvailableToChildrenChanged. BUG=594465,593593,590683,593624,593593 ==========
Description was changed from ========== When doing the overflow:auto layout marking, we also have to note the width available to children changed. This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to set m_{width,height}AvailableToChildrenChanged. BUG=594465,593593,590683,593624,593593 ========== to ========== Set m_{width,height}AvailableToChildrenChanged when scrollbars appear This is a regression from https://codereview.chromium.org/1734203002. Before that change, we'd just call layoutBlock(true). However, the new approach just marks for layout and eventually recurses down, so to ensure that the children get laid out, we have to set m_{width,height}AvailableToChildrenChanged. BUG=594465,593593,590683,593624,593593 ==========
As a sidenote, we can probably now pass false in the layoutBlock call a few lines down in PaintLayerScrollableArea.cpp and gain a small performance advantage in some cases
Message was sent while issue was closed.
Description was changed from
==========
Set m_{width,height}AvailableToChildrenChanged when scrollbars appear
This is a regression from https://codereview.chromium.org/1734203002. Before
that change, we'd just call layoutBlock(true). However, the new approach
just marks for layout and eventually recurses down, so to ensure that
the children get laid out, we have to set
m_{width,height}AvailableToChildrenChanged.
BUG=594465,593593,590683,593624,593593
==========
to
==========
Set m_{width,height}AvailableToChildrenChanged when scrollbars appear
This is a regression from https://codereview.chromium.org/1734203002. Before
that change, we'd just call layoutBlock(true). However, the new approach
just marks for layout and eventually recurses down, so to ensure that
the children get laid out, we have to set
m_{width,height}AvailableToChildrenChanged.
BUG=594465,593593,590683,593624,593593
==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from
==========
Set m_{width,height}AvailableToChildrenChanged when scrollbars appear
This is a regression from https://codereview.chromium.org/1734203002. Before
that change, we'd just call layoutBlock(true). However, the new approach
just marks for layout and eventually recurses down, so to ensure that
the children get laid out, we have to set
m_{width,height}AvailableToChildrenChanged.
BUG=594465,593593,590683,593624,593593
==========
to
==========
Set m_{width,height}AvailableToChildrenChanged when scrollbars appear
This is a regression from https://codereview.chromium.org/1734203002. Before
that change, we'd just call layoutBlock(true). However, the new approach
just marks for layout and eventually recurses down, so to ensure that
the children get laid out, we have to set
m_{width,height}AvailableToChildrenChanged.
BUG=594465,593593,590683,593624,593593
Committed: https://crrev.com/fbb25cec59deb052604d63036f1c5baf6efd2a5c
Cr-Commit-Position: refs/heads/master@{#382074}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fbb25cec59deb052604d63036f1c5baf6efd2a5c Cr-Commit-Position: refs/heads/master@{#382074} |
