Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1218023004:
Fix a regression when position:static with absolute child changed to relative (Closed)
|
Created:
5 years, 5 months ago by kojii Modified:
5 years, 5 months ago CC:
blink-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, blink-reviews-rendering, jchaffraix+rendering Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix a regression when position:static with absolute child changed to relative
This patch fixes a regression that was introduced by a CL[1] that fixed
incorrect order of the positioned objects list.
As part of the fix, the CL replaced a partial duplication of the
containingBlock() implementation with a call to containingBlock().
This part of the fix was wrong. It was a code path for when an element
has absolute descendants and becomes a new container for them. In this
case, the absolute descendants are not removed from the list of the old
containing block, and thus belong to two containing blocks simultaneously.
To fix this, the new containing block is not position:absolute either
in styleWillChange/styleDidChange, but it needs to calculate by
pretending it is position:absolute.
Rather than reverting to the copy of the logic, this patch extracts the
logic from containingBlock() to containingBlockForAbsolutePosition().
[1] https://codereview.chromium.org/1181983002
BUG=505389, 407356, 416210, 351709, 144608, 500511
TEST=fast/dynamic/static-to-relative-with-absolute-and-static-child.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198389
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add ASSERT(gPositionedContainerMap->get(o)->size() == 1) #
Total comments: 4
Patch Set 3 : mstensho@ review in #10, and test from #11 #
Messages
Total messages: 19 (2 generated)
kojii@chromium.org changed reviewers: + jchaffraix@chromium.org, leviw@chromium.org, mstensho@opera.com
PTAL.
https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:290: if (LayoutBlock* cb = containerForAbsolutePosition()) How about doing this in styleDidChange() instead, then? Then containingBlock() will return what you want, no?
On 2015/06/30 19:47:17, mstensho wrote: > https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... > File Source/core/layout/LayoutBlock.cpp (right): > > https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... > Source/core/layout/LayoutBlock.cpp:290: if (LayoutBlock* cb = > containerForAbsolutePosition()) > How about doing this in styleDidChange() instead, then? Then containingBlock() > will return what you want, no? A bit long reply, sorry for not being able to write in short. Cases when |this| is changed to absolute were the cases we discussed in the previous CL, and is covered by the the change to |styleDidChange| in the previous CL. This |if| was actually redundant from that perspective, but it is needed for a different case, when: <body> <div id=outer> <div id=middle> <div id=inner position:absolute> and change |middle| to "relative". In this case, |inner| is in |body|'s list, and we need to clear it because |inner|'s containing block is changed to |middle|. However, |middle| is changing to "relative", not to "absolute", so |middle->containingBlock()| in |styleDidChange| is |outer|, not |body|. We need to calculate the containingBlock as if |middle| is absolute, so that we can find contaningBlock of |middle|'s absolute descendants, not |middle| itself. When I re-read the comment after seeing this bug, it is talking about this specific case, I just failed to read correctly before. Note that this bug is invalidation issue, not the ordered list issue, so no assertions fail. But not clearing |body|'s list here fails to invalidate the descendant, and thus failed to layout correctly only in the next layout. When |body|'s list is recalculated for other changes, |inner| will be removed from the list and layout becomes correct. You can observe this at: http://www.w3schools.com/tags/tag_dir.asp The menu bar moves to incorrect position but returns to the correct position when the page inserts ad, or opening the test and resizing window fixes this issue. If you're concerned about performance by extracting containerForAbsolute(), I'm fine to keep a copy, probably leaving a comment saying these two parts of the code must be in sync. But since containgBlock() calls |return containerForAbsolute|, so it's a jump, not a call. I believe it's not expensive.
On 2015/07/01 01:05:14, kojii wrote: > On 2015/06/30 19:47:17, mstensho wrote: > > > https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... > > File Source/core/layout/LayoutBlock.cpp (right): > > > > > https://codereview.chromium.org/1218023004/diff/1/Source/core/layout/LayoutBl... > > Source/core/layout/LayoutBlock.cpp:290: if (LayoutBlock* cb = > > containerForAbsolutePosition()) > > How about doing this in styleDidChange() instead, then? Then containingBlock() > > will return what you want, no? > > A bit long reply, sorry for not being able to write in short. > > Cases when |this| is changed to absolute were the cases we discussed in the > previous CL, and is covered by the the change to |styleDidChange| in the > previous CL. > > This |if| was actually redundant from that perspective, but it is needed for a > different case, when: > <body> > <div id=outer> > <div id=middle> > <div id=inner position:absolute> > and change |middle| to "relative". In this case, |inner| is in |body|'s list, > and we need to clear it because |inner|'s containing block is changed to > |middle|. > > However, |middle| is changing to "relative", not to "absolute", so > |middle->containingBlock()| in |styleDidChange| is |outer|, not |body|. We need > to calculate the containingBlock as if |middle| is absolute, so that we can find > contaningBlock of |middle|'s absolute descendants, not |middle| itself. > > When I re-read the comment after seeing this bug, it is talking about this > specific case, I just failed to read correctly before. > > Note that this bug is invalidation issue, not the ordered list issue, so no > assertions fail. But not clearing |body|'s list here fails to invalidate the > descendant, and thus failed to layout correctly only in the next layout. When > |body|'s list is recalculated for other changes, |inner| will be removed from > the list and layout becomes correct. You can observe this at: > http://www.w3schools.com/tags/tag_dir.asp > The menu bar moves to incorrect position but returns to the correct position > when the page inserts ad, or opening the test and resizing window fixes this > issue. > > If you're concerned about performance by extracting containerForAbsolute(), I'm > fine to keep a copy, probably leaving a comment saying these two parts of the > code must be in sync. But since containgBlock() calls |return > containerForAbsolute|, so it's a jump, not a call. I believe it's not expensive. I'm not that concerned about performance here. It's just I would like understand why we need to make this change. Pretending to be absolutely positioned when you're not sounds weird, although your change does correct something: Leaving #abs behind in the list of its former containing block isn't good. BTW: this is not a repaint issue - it's a layout issue. The test you attached had this: <div id=outer> <div id=abs></div> <div id=next></div> </div> #abs is absolutely positioned. #next is statically positioned. Then you change #outer's position from being static to relative, so that #abs gets a new containing block (#outer instead of the initial containing block). #abs has no "left" or "top" specified, so we have to figure out where it would have been had it been statically positioned. All boxes have a m_frameRect, which is relative to the containing block of the box. The containing block of #abs used to be the initial containing block. #next has a top margin of 50px (which collapses with the default 8px top-margin on BODY). So the vertical distance from the top of #abs to its containing block (initial containing block) will be 50px. Now, if we change the containing block of #abs to be #outer, but forget to update its m_frameRect, things will look different. The vertical position of #outer (relative to the viewport) is 50px, due to the aforementioned margin-collapsing. Since #abspos doesn't get its m_frameRect corrected, it remains at 50px too. 50px + 50px = 100px, and it gets mispositioned. Since margin collapsing is just a pain, and it's not necessary to trigger the bug, here's a simpler test case: <body style="margin:0;"> <div id="filler" style="height:100px;"></div> <div id="outer"> <div id="abs" style="position:absolute; width:100px; height:50px; background:blue;"></div> <div id="dummy"></div> </div> </div> The vertical distance from #abs to its containing block (and to the viewport) is 100px (thanks to #filler). The vertical distance from #outer to its containing block (and to the viewport) is also 100px. If you change #outer to become position:relative, #abspos will incorrectly think that it's still 100px away from its containing block (while the actual distance to #outer (the new containing block) is 0), and it will end up at 100px + 100px = 200px, instead of 100px. Oh, and the #dummy element? It needs to be there to trigger the bug, because we need #outer to have block children, or the bug won't occur. The reason for this can be found in the old version of needsLayoutDueToStaticPosition() in LayoutBlock.cpp, where we returned false if we had block children. I fixed this function yesterday in https://codereview.chromium.org/1217833007 - and now your bug is gone too. :) So I'm not sure what to do with this CL. The test you wrote is valuable in any case as a regression test, but we need a new one that fails without your code changes.
On 2015/07/01 09:14:29, mstensho wrote:
>
> I'm not that concerned about performance here. It's just I would like
understand
> why we need to make this change.
Superb. I'd be more than happy to share what I found in this bug, appreciate
your comments if you see any logic problem in my understanding.
> Pretending to be absolutely positioned when
> you're not sounds weird, although your change does correct something: Leaving
> #abs behind in the list of its former containing block isn't good.
Can't agree more, it's weird. I too thought it was a bug that I fixed in the
previous CL.
> BTW: this is not a repaint issue - it's a layout issue.
Right, sorry using ambiguous words, when I said "invalidate", I meant "mark
element's m_frameRect as dirty for re-layout", not "invalidate paint area of
screen."
I have to admit that I'm not clear whether we recalculate immediately, or "mark
as dirty" and run layout cycle, so I'm using this as an ambiguous term here.
> The test you attached had this:
>
> <div id=outer>
> <div id=abs></div>
> <div id=next></div>
> </div>
>
> #abs is absolutely positioned. #next is statically positioned.
>
> Then you change #outer's position from being static to relative, so that #abs
> gets a new containing block (#outer instead of the initial containing block).
One of the key factor to understand this weird code is to think about the box
tree (which adds LayoutFrame to DOM tree):
<LayoutFrame> <== ICB / CB of abs
<body>
<div id=outer (static)>
<div id=abs></div>
<div id=next></div>
</div>
</body>
<LayoutFrame>
In this tree, |abs|'s containing block is <LayoutFrame>. Then change |outer| to
relative:
<LayoutFrame> <== ICB / was CB of abs, so abs is in this list
<body> <== CB of outer
<div id=outer (relative)> <== CB of abs
<div id=abs></div>
<div id=next></div>
</div>
</body>
<LayoutFrame>
With this change, |abs|'s containing block is changed from <LayoutFrame> to
|outer|, so we need to clear <LayoutFrame>'s list.
When |outer.styleWillChange()| is called, |outer|'s containing block is <body>
(because outer is static/relative, its containing block is its parent block.) So
the current code clears <body>'s list, but since |abs| isn't in the list (it's
in <LayoutFrame>'s list instead), |abs| isn't marked dirty for re-layout.
What we need to do here is that, when |this| is changed from non-CB to CB,
assuming |this| has absolute descendants, calculate the containing block of the
descendants, not of |this|.
The correct pseudo code is like this:
for (auto& descendat : traverseDescendants()) {
if (descendant.styleRef().position() == AbsolutePosition) {
if (auto& cb = descendant.containingBlock())
cb->removePositionedObjects(descendant, NewContainingBlock);
}
}
This pseudo code code can be optimized to the code before the last CL, or new
code in this CL.
> #abs has no "left" or "top" specified, so we have to figure out where it would
> have been had it been statically positioned. All boxes have a m_frameRect,
which
> is relative to the containing block of the box. The containing block of #abs
> used to be the initial containing block. #next has a top margin of 50px (which
> collapses with the default 8px top-margin on BODY). So the vertical distance
> from the top of #abs to its containing block (initial containing block) will
be
> 50px. Now, if we change the containing block of #abs to be #outer, but forget
to
> update its m_frameRect, things will look different. The vertical position of
> #outer (relative to the viewport) is 50px, due to the aforementioned
> margin-collapsing. Since #abspos doesn't get its m_frameRect corrected, it
> remains at 50px too. 50px + 50px = 100px, and it gets mispositioned.
Correct. "forgot to update its m_frameRect" is what is happening here, and what
I tried to explain by "invalidate" but that was a bad use of the terminology,
apologies.
I hope new version of the explanation above now makes sense?
> Since margin collapsing is just a pain, and it's not necessary to trigger the
> bug, here's a simpler test case:
>
> <body style="margin:0;">
> <div id="filler" style="height:100px;"></div>
> <div id="outer">
> <div id="abs" style="position:absolute; width:100px; height:50px;
> background:blue;"></div>
> <div id="dummy"></div>
> </div>
> </div>
Right, I actually removed margin-top once. But the diff look too subtle that I
worried someone who regress in future may consider it's not a failure. It's
there just to make the diff more obvious. I'm fine to remove that if it's
confusing.
> The vertical distance from #abs to its containing block (and to the viewport)
is
> 100px (thanks to #filler). The vertical distance from #outer to its containing
> block (and to the viewport) is also 100px. If you change #outer to become
> position:relative, #abspos will incorrectly think that it's still 100px away
> from its containing block (while the actual distance to #outer (the new
> containing block) is 0), and it will end up at 100px + 100px = 200px, instead
of
> 100px.
>
> Oh, and the #dummy element? It needs to be there to trigger the bug, because
we
> need #outer to have block children, or the bug won't occur. The reason for
this
> can be found in the old version of needsLayoutDueToStaticPosition() in
> LayoutBlock.cpp, where we returned false if we had block children. I fixed
this
> function yesterday in https://codereview.chromium.org/1217833007 - and now
your
> bug is gone too. :) So I'm not sure what to do with this CL. The test you
wrote
> is valuable in any case as a regression test, but we need a new one that fails
> without your code changes.
Oh, wao.
I need time to understand your CL to see if there were any further cases left.
As explained above, by the last CL, Blink no longer clears |abs| from the list
of its old CB unless |outer|'s parent is the old CB. If your CL forces layout of
positioned objects without clearing from the old list, clearing is not necessary
-- this part of the code can even be removed.
I'll look into it, and see if there were any cases, but if you found something
from the explanation above, it's apprecaited to know.
On 2015/07/01 13:39:24, kojii wrote: > I need time to understand your CL to see if there were any further cases left. > > As explained above, by the last CL, Blink no longer clears |abs| from the list > of its old CB unless |outer|'s parent is the old CB. If your CL forces layout of > positioned objects without clearing from the old list, clearing is not necessary > -- this part of the code can even be removed. Why? Being in the wrong list is bad in any case, isn't it? Out-of-flow positioned elements get added to the correct containing block when their parent (NOT necessarily the same as containing block) is laid out, so I'm pretty sure we'll fail to move it into the correct list in many cases (since a non-CB parent of an out-of-flow child doesn't need layout just because the out-of-flow child needs layout). My CL doesn't improve that situation at all, as far as I can tell. > I'll look into it, and see if there were any cases, but if you found something > from the explanation above, it's apprecaited to know. You're still fixing an actual issue with this CL. I have a pretty clear understanding of what's going on and what you're trying to fix. Now all we need to do is come up with a test case that gets fixed by it. :)
On 2015/07/02 13:46:19, mstensho wrote: > On 2015/07/01 13:39:24, kojii wrote: > > I need time to understand your CL to see if there were any further cases left. > > > > As explained above, by the last CL, Blink no longer clears |abs| from the list > > of its old CB unless |outer|'s parent is the old CB. If your CL forces layout > of > > positioned objects without clearing from the old list, clearing is not > necessary > > -- this part of the code can even be removed. > > Why? Being in the wrong list is bad in any case, isn't it? Out-of-flow > positioned elements get added to the correct containing block when their parent > (NOT necessarily the same as containing block) is laid out, so I'm pretty sure > we'll fail to move it into the correct list in many cases (since a non-CB parent > of an out-of-flow child doesn't need layout just because the out-of-flow child > needs layout). My CL doesn't improve that situation at all, as far as I can > tell. > > > I'll look into it, and see if there were any cases, but if you found something > > from the explanation above, it's apprecaited to know. > > You're still fixing an actual issue with this CL. I have a pretty clear > understanding of what's going on and what you're trying to fix. Now all we need > to do is come up with a test case that gets fixed by it. :) Yeah, I understand that, but "come up with a test case" turned out to be harder than thought. Tried several patterns and traced code, but still unable to find one. One symptom I found and I'm wondering is in |LayoutBlock::insertIntoTrackedLayoutBoxMaps|, |abs|'s containerSet->size() becomes 2. This looks strange to me, as an element should not have 2 containing blocks. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I suspect this is a HashMap because it shares the code with |addPercentHeightDescendant|, which could have multiple containerSet, but we could assert the size is always 1 only in |insertPositionedObject|. The test in this CL without fix will hit the AF, and the fix will fix it. I ran all fast tests and no other tests hit this AF. Is this a correct assertion to add? Right now, this is the only way I found so far for this CL to "fix" something visible, other than having the correct list as you said. One more thing; if this is the correct assertion, using a HashMap looks a waste of private memory, though it saves shared memory by sharing the code with |addPercentHeightDescendant|. I can change that to a raw pointer if you agree that it's the right thing to do. Note I thought we may paint twice by having single block pointed from two containers, and dumped the paint rects, but that didn't occur. If, for some reasons the size() could be more than 1, I can keep looking for other cases of failures in lower priority, it's a good way to study the code for me, but I'm not sure if this can cause anything other than having incorrect entry in the list and doing some unnecessary calculation after your CL.
PTAL. WDYT? PS2 added the ASSERT(gPositionedContainerMap->get(o)->size() == 1). I confirmed: 1. This ASSERT does not fail with any existing tests. 2. This ASSERT fails with the test in this CL if without this fix. 3. Unlike gPositionedContainerMap, gPercentHeightContainerMap could have multiple containers. We could further do: a. Remove from container's list if multiple containers are found, since it's likely a bug. b. Make the TrackedContainerMap to a raw pointer only for gPositionedContainerMap. This adds some shared memory (increased code) while reduces private memory (data) and some CPU cycles for positioned objects. I'm not sure if this is worth to pursue.
No, gPositionedContainerMap doesn't seem useful at all to me. It was originally introduced by leviw. Might want to check with him, whether he did it that way just to be able to share machinery with percentage height descendants. But changing anything regarding this is something for a separate CL, in any case. You also need to update the description of this CL. https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.cpp:900: if (!isTextOrSVGChild() && m_style->position() == AbsolutePosition) Kind of silly to call isTextOrSVGChild() all day long, although not expensive. You've just made it possible to move the check to an outer scope. if (!isTextOrSVGChild()) { if (fixedpos) return blah; if (abspos) return fish; } https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.cpp:901: return containerForAbsolutePosition(); containingBlockForAbsolutePosition() is a better name. container() locates a containing block for abspos differently from what containingBlock() does.
Oh, look! I managed to come up with a test case that fails visually without your
fix. The only thing I could think of was that you'd have incorrect LayoutState
when laying out an out-of-flow descendant from the wrong containing block on the
call stack. And since LayoutState is mostly about multicol and pagination,
here's a multicol test:
<!DOCTYPE html>
<p>The word "PASS" should be seen below.</p>
<div id="outer" style="position:relative; height:200px;">
<div style="-webkit-columns:4; -webkit-column-gap:0; width:4em;">
<div id="newContainingBlock">
<div style="position:absolute; top:0; left:0; right:0; bottom:0;">
P<br>
A<br>
S<br>
S<br>
</div>
<br>
<br>
<br>
<br>
</div>
</div>
</div>
<script>
document.body.offsetTop;
document.getElementById("newContainingBlock").style.position = "relative";
document.getElementById("outer").style.height = "201px";
</script>
I'm changing the height of the old containing block (#outer) at the same time as
introducing a new containing block, to force layout of all objects in the
positioned objects list of #outer. I haven't debugged, but it looks like the
multicol code gets sufficiently confused by an incorrect LayoutState
(LayoutState will say that it's not "paginated" when it should have said that it
was).
Thank you mstensho@, this is far beyond my knowledge, but I confirmed this CL fix your test. Removed the assert and included your test, PTAL. https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... File Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.cpp:900: if (!isTextOrSVGChild() && m_style->position() == AbsolutePosition) On 2015/07/06 09:46:24, mstensho wrote: > Kind of silly to call isTextOrSVGChild() all day long, although not expensive. > You've just made it possible to move the check to an outer scope. > > if (!isTextOrSVGChild()) { > if (fixedpos) > return blah; > if (abspos) > return fish; > } Done. https://codereview.chromium.org/1218023004/diff/20001/Source/core/layout/Layo... Source/core/layout/LayoutObject.cpp:901: return containerForAbsolutePosition(); On 2015/07/06 09:46:24, mstensho wrote: > containingBlockForAbsolutePosition() is a better name. container() locates a > containing block for abspos differently from what containingBlock() does. Done.
This sentence in the description doesn't parse: "As part of the fix, the CL replaced a copy of code from containingBlock() for when position:absolute to a call to containingBlock()." Typos here: "descendats are not removed from the list of the old containg block, and thus belong to two containg blocks simultaneously." Should be "descendants" and 2x "containing".
Thank you for your prompt review, along with all the reviews so far. On 2015/07/07 09:02:53, mstensho wrote: > This sentence in the description doesn't parse: "As part of the fix, the CL > replaced a copy of code from containingBlock() for when position:absolute to a > call to containingBlock()." Tried to fix, does this parse? > Typos here: "descendats are not removed from the list of the old > containg block, and thus belong to two containg blocks simultaneously." > > Should be "descendants" and 2x "containing". Fixed, thank you.
Could you say something like this instead: "As part of the fix, the CL replaced a partial duplication of the containingBlock() implementation with a call to containingBlock()." Then, lgtm :)
On 2015/07/07 10:08:08, mstensho wrote: > Could you say something like this instead: > "As part of the fix, the CL replaced a partial duplication of the > containingBlock() implementation with a call to containingBlock()." > > Then, lgtm :) Thank you for better English, not only great supports in code and tests :) Really, I appreciate your deep support to land this CL, thank you so much!!
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218023004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198389 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
