Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737253003:
Remove floating objects from descendants of subtree roots (Closed)
DescriptionRemove floating objects from descendants of subtree roots
The pre-layout for orthogonal flow clears floats from its containing
block in [1]. However, this can cause trouble for
markAllDescendantsWithFloatsForLayout() to mark descendants for layout.
When preceding floats overhanging to following blocks are gone, such
floats are referred from multiple following blocks including
descendants. Failure to mark descendants for layout may leave preceding
floats.
This patch ensures such floats are removed.
[1] https://codereview.chromium.org/2025543002
BUG=698455
Review-Url: https://codereview.chromium.org/2737253003
Cr-Commit-Position: refs/heads/master@{#456300}
Committed: https://chromium.googlesource.com/chromium/src/+/c020f6a22577978ce1fe89fc1a397f2a651c48a8
Patch Set 1 #Patch Set 2 : Test from mstensho #
Messages
Total messages: 29 (20 generated)
The CQ bit was checked by kojii@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 ========== ffloats BUG= ========== to ========== Remove floating objects from descendants of subtree roots BUG=698455 ==========
kojii@chromium.org changed reviewers: + mstensho@opera.com, robhogan@chromium.org
Hey, could the two best float experts on the planet advise? By tracing the case and reading more float-related code, I now understand floats are copied to multiple LayoutBlockFlow, and in this case, addOverhangingFloats copies a destroyed object from one of its children. So either: 1. remove floating objects from descendants, or 2. mark them for layout and ensure they're laid out before addOverhangingFloats copies objects from its children seems to fix this issue. However, I don't have good idea which is better, or even other fix is more desirable. Advice appreciated in advance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/09 at 15:13:28, kojii wrote: > Hey, could the two best float experts on the planet advise? > > By tracing the case and reading more float-related code, I now understand floats are copied to multiple LayoutBlockFlow, and in this case, addOverhangingFloats copies a destroyed object from one of its children. > > So either: > 1. remove floating objects from descendants, or > 2. mark them for layout and ensure they're laid out before addOverhangingFloats copies objects from its children > seems to fix this issue. > > However, I don't have good idea which is better, or even other fix is more desirable. > > Advice appreciated in advance. In an ideal world you would only need to do this when a float has been removed from the orthogonal subtree (which should happen automatically from removeFloatingObject()) or when the subtree first becomes orthogonal. I believe this issue is the due to the former case and is the subject of your TODO in removeFloatingObjectsForSubtreeRoot() - the floating object moved to another tree doesn't always get removed from the subtree's float lists. So yes, it looks like we've finally found a case where this happens and need to clear down the floatlists in the subtree completely. Is there a way of ensuring removeChildNode() always succeeds in clearing down float lists? Probably not without making the common case much slower, it looks like its the orthogonal case that has to suffer.
On 2017/03/09 at 19:15:28, robhogan wrote: > On 2017/03/09 at 15:13:28, kojii wrote: > > Hey, could the two best float experts on the planet advise? > > > > By tracing the case and reading more float-related code, I now understand floats are copied to multiple LayoutBlockFlow, and in this case, addOverhangingFloats copies a destroyed object from one of its children. > > > > So either: > > 1. remove floating objects from descendants, or > > 2. mark them for layout and ensure they're laid out before addOverhangingFloats copies objects from its children > > seems to fix this issue. > > > > However, I don't have good idea which is better, or even other fix is more desirable. > > > > Advice appreciated in advance. > > In an ideal world you would only need to do this when a float has been removed from the orthogonal subtree (which should happen automatically from removeFloatingObject()) or when the subtree first becomes orthogonal. Yeah, I first thought we can check WritingMode in LayoutBoxModelObject::moveChildTo(), but it didn't hit. I don't have a complete picture yet, but this could happen without floats moving across writing-mode boundary. > I believe this issue is the due to the former case and is the subject of your TODO in removeFloatingObjectsForSubtreeRoot() - the floating object moved to another tree doesn't always get removed from the subtree's float lists. So yes, it looks like we've finally found a case where this happens and need to clear down the floatlists in the subtree completely. > > Is there a way of ensuring removeChildNode() always succeeds in clearing down float lists? I failed to understand this, sorry. Did you mean removeFloatingObjectsFromDescendants()? Could it fail in any circumstances? > Probably not without making the common case much slower, it looks like its the orthogonal case that has to suffer. Agree, orthogonal flows are not common, we should prioritize other cases even if it makes orthogonal flows slower.
On 2017/03/10 at 03:19:01, kojii wrote: > On 2017/03/09 at 19:15:28, robhogan wrote: > > Is there a way of ensuring removeChildNode() always succeeds in clearing down float lists? > > I failed to understand this, sorry. Did you mean removeFloatingObjectsFromDescendants()? Could it fail in any circumstances? No I don't think it can fail to do what you want here. What I meant is, if removeChildNode() is removing a float from the tree it should succeed in removing that float from every other element's float lists. It currently doesn't, and you allude to that in your TODO. That's the part we really need to fix but I think it might be too expensive in all cases (I don't remember the reason we can't/don't offhand) - so for now we're probably better off just nuking all floats in an orthogonal subtree like you do in your CL.
The CQ bit was checked by kojii@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 CQ bit was checked by kojii@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 ========== Remove floating objects from descendants of subtree roots BUG=698455 ========== to ========== Remove floating objects from descendants of subtree roots The pre-layout for orthogonal flow clears floats from its containing block in [1]. However, this can cause trouble for markAllDescendantsWithFloatsForLayout() to mark descendants for layout. When preceding floats overhanging to following blocks are gone, such floats are referred from multiple following blocks including descendants. Failure to mark descendants for layout may leave preceding floats. This patch ensures such floats are removed. [1] https://codereview.chromium.org/2025543002 BUG=698455 ==========
Patchset #2 (id:20001) has been deleted
Thank you, I understand you correctly now. |removeChildNode() fails only when |moveChildTo()| with |fullRemoveInsert| is false. Changing it to clean down float list of all descendants is likely to hit performance on normal cases, IIUC the purpose of |fullRemoveInsert|, though I'm not really familiar with these operations. Probably mstensho@ is the best expert on this. I hoped to check writing-mode in |moveChildTo()|, but it doesn't help since we're not moving floats across writing modes boundary. So, as you said, since extending the |removeFloatingObjectsForSubtreeRoot()| to clear descendants only affects when there are orthogonal flows, I think this is still better way to go. NG should fix this completely. Until then, if either of you, or someone else, wants to fix when |moveChildTo()| calls |removeChildNode()| with |fullRemoveInsert| is false, I'd be happy to help whatever I can, but I don't have good confidence to fix that by myself.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL.
robhogan@gmail.com changed reviewers: + robhogan@gmail.com
The CQ bit was checked by robhogan@gmail.com
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": 1489313788206920,
"parent_rev": "4d64dfea1e991801d7c4fc0438439f702ea7f206", "commit_rev":
"c020f6a22577978ce1fe89fc1a397f2a651c48a8"}
Message was sent while issue was closed.
Description was changed from ========== Remove floating objects from descendants of subtree roots The pre-layout for orthogonal flow clears floats from its containing block in [1]. However, this can cause trouble for markAllDescendantsWithFloatsForLayout() to mark descendants for layout. When preceding floats overhanging to following blocks are gone, such floats are referred from multiple following blocks including descendants. Failure to mark descendants for layout may leave preceding floats. This patch ensures such floats are removed. [1] https://codereview.chromium.org/2025543002 BUG=698455 ========== to ========== Remove floating objects from descendants of subtree roots The pre-layout for orthogonal flow clears floats from its containing block in [1]. However, this can cause trouble for markAllDescendantsWithFloatsForLayout() to mark descendants for layout. When preceding floats overhanging to following blocks are gone, such floats are referred from multiple following blocks including descendants. Failure to mark descendants for layout may leave preceding floats. This patch ensures such floats are removed. [1] https://codereview.chromium.org/2025543002 BUG=698455 Review-Url: https://codereview.chromium.org/2737253003 Cr-Commit-Position: refs/heads/master@{#456300} Committed: https://chromium.googlesource.com/chromium/src/+/c020f6a22577978ce1fe89fc1a39... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c020f6a22577978ce1fe89fc1a39... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
