New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 663942 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Nov 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Block inside float not relaid out properly when fragmentainer height changes

Reported by msten...@opera.com, Nov 9 2016

Issue description

If we need to insert a soft fragmentainer break before a float with a block, the pagination strut before the block is lost in some cases (rather than being propagated to the float), leading to unbreakable content (e.g. a line) being split across fragmentainer boundaries.

This is a recent regression, and it was most likely caused by the fix for  bug 487026 .
 
tc.html
370 bytes View Download
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d108b2435612ff0bf1f2d88e79f798b691889df2

commit d108b2435612ff0bf1f2d88e79f798b691889df2
Author: mstensho <mstensho@opera.com>
Date: Fri Nov 11 00:42:57 2016

Split positionAndLayoutFloat() off positionNewFloats().

Float layout is somewhat broken when it comes to fragmentation (multicol,
printing). We're going to have to make sure that we always position the
float before laying it out, and, after layout, insert a break before it if
needed. This is a preparatory CL for that.

We currently lay out a float e.g. in insertFloatingObject() without
worrying about setting the position first.

No behavior changes intended.

BUG= 663942 

Review-Url: https://codereview.chromium.org/2486413002
Cr-Commit-Position: refs/heads/master@{#431422}

[modify] https://crrev.com/d108b2435612ff0bf1f2d88e79f798b691889df2/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/d108b2435612ff0bf1f2d88e79f798b691889df2/third_party/WebKit/Source/core/layout/LayoutBlockFlow.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7892652d84a2a704b5a6118806886720dc8c7127

commit 7892652d84a2a704b5a6118806886720dc8c7127
Author: mstensho <mstensho@opera.com>
Date: Thu Nov 17 16:48:40 2016

Rename positionNewFloats() to placeNewFloats().

This will distinguish it better from the method named "positionAndLayoutFloat".

Also be explicit about the fact that we use the top margin edge when
positioning floats, as opposed to the top border edge, which is common for all
other object types. So "logicalTop" usually means the logical top of the border
edge. Therefore, use "logicalTopMarginEdge" for floats.

No behavioral changes, just cleanup.

BUG= 663942 

Review-Url: https://codereview.chromium.org/2505943003
Cr-Commit-Position: refs/heads/master@{#432895}

[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/LayoutBlockFlow.h
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/line/LineBreaker.cpp
[modify] https://crrev.com/7892652d84a2a704b5a6118806886720dc8c7127/third_party/WebKit/Source/core/layout/line/LineWidth.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/523855f49df9f2f9ab9701b2488e4708ef908391

commit 523855f49df9f2f9ab9701b2488e4708ef908391
Author: mstensho <mstensho@opera.com>
Date: Fri Nov 18 07:46:11 2016

Introduce adjustFloatLogicalTopForPagination(), to offload positionAndLayoutFloat().

Also renamed a variable from childBox to child in positionAndLayoutFloat().

BUG= 663942 

Review-Url: https://codereview.chromium.org/2513643002
Cr-Commit-Position: refs/heads/master@{#433143}

[modify] https://crrev.com/523855f49df9f2f9ab9701b2488e4708ef908391/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/523855f49df9f2f9ab9701b2488e4708ef908391/third_party/WebKit/Source/core/layout/LayoutBlockFlow.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/596aa530f12f7777ec6336c891930a5c6770fad4

commit 596aa530f12f7777ec6336c891930a5c6770fad4
Author: mstensho <mstensho@opera.com>
Date: Tue Nov 29 13:10:13 2016

Position a float before laying it out.

We'll no longer perform inaccurate layout from insertFloatingObject(), but
defer all layout to positionAndLayoutFloat(). We need to do this correctly
everywhere. One crucial thing is also to pay attention to the resulting
pagination strut before the float, if any. There's only one place where we do
this, and that's in positionAndLayoutFloat().

At most call sites, insertFloatingObject() is followed by a call to
placeNewFloats(), which will call positionAndLayoutFloat(). There are
exceptions to this in line layout, though. In some cases we just insert floats
without laying them out and placing them. This happens when we need to figure
out the height of the current line before we can place floats below it. In
order to figure out if a float fits on the current line, though, we first need
to lay it out without marking it as placed.

We lacked some test coverage, so I added
float-pushed-to-next-fragmentainer-by-floats.html . This also passed prior to
this CL, but I nearly broke it while working on this.

BUG= 663942 

Review-Url: https://codereview.chromium.org/2532573003
Cr-Commit-Position: refs/heads/master@{#434969}

[add] https://crrev.com/596aa530f12f7777ec6336c891930a5c6770fad4/third_party/WebKit/LayoutTests/fragmentation/change-fragmentainer-height-block-float-2.html
[add] https://crrev.com/596aa530f12f7777ec6336c891930a5c6770fad4/third_party/WebKit/LayoutTests/fragmentation/float-pushed-to-next-fragmentainer-by-floats.html
[modify] https://crrev.com/596aa530f12f7777ec6336c891930a5c6770fad4/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/596aa530f12f7777ec6336c891930a5c6770fad4/third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h
[modify] https://crrev.com/596aa530f12f7777ec6336c891930a5c6770fad4/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/88acad37d1ea59638054bc8b23216a67c392ccc3

commit 88acad37d1ea59638054bc8b23216a67c392ccc3
Author: mstensho <mstensho@opera.com>
Date: Wed Nov 30 22:16:28 2016

Avoid rogue line float re-layout.

We cannot just lay out an object without setting its position first. That would
confuse the fragmenation machinery. Fortunately, it's not even necessary to lay
out here. Changed the comment, as an attempt to explain why.

BUG= 663942 

Review-Url: https://codereview.chromium.org/2539813002
Cr-Commit-Position: refs/heads/master@{#435442}

[add] https://crrev.com/88acad37d1ea59638054bc8b23216a67c392ccc3/third_party/WebKit/LayoutTests/fragmentation/change-fragmentainer-height-line-float.html
[modify] https://crrev.com/88acad37d1ea59638054bc8b23216a67c392ccc3/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp

Comment 6 by msten...@opera.com, Nov 30 2016

Status: Fixed (was: Assigned)

Sign in to add a comment