Require spanners to have the multicol container as their nearest block formatting context.

Require spanners to have the multicol container as their nearest block formatting context.

The editor's draft [1] suggests that a column-span:all object needs to be in
the same block formatting context, so disallow column-span for everything else.
MSIE does the same. Gecko doesn't support spanners.

While this simplifies the code and gives us less to worry about (and it
actually fixes bug 529737, although that wasn't my main motivation here),
another reason to make this change is that we'd get assertion failures if we
put a spanner inside a scrollable flex item inside a flexbox inside a multicol
inside another flexbox. The reason why the assertion failures happen is the
LayoutBlock::finishDelayUpdateScrollInfo() mechanism, which may jump to some
arbitrary block in the subtree and lay it out directly. While that is bad on
its own, the multicol implementation should now at least be immune to damage
caused by that.

Removed some old tests that are now invalid, because they expected spanners to
be created inside scrollable containers. This no longer works, because
non-visible overflow implies a new block formatting context.

[1] https://drafts.csswg.org/css-multicol/#column-span

BUG=529737
R=leviw@chromium.org

Committed: https://crrev.com/7b65998a89f974e7c7f5dfce4f4eae7fa69b421d
Cr-Commit-Position: refs/heads/master@{#356680}

[mstensho (USE GERRIT), 2015-10-27 14:43:57]

I decided against fixing the LayoutBlock::finishDelayUpdateScrollInfo() (used by
flexbox) mechanism for now. We obviously need to do something like that somehow
(and I don't know how to clean it up properly). Since what made that mechanism
cause trouble for multicol was spanners, let's just avoid spanners in these
cases. That's what IE does, and also what the spec suggests.

See also https://codereview.chromium.org/1328923002 , which is another fix for
the problems that LayoutBlock::finishDelayUpdateScrollInfo() caused for
multicol.

[leviw_travelin_and_unemployed, 2015-10-27 18:33:56]

leviw@chromium.org changed reviewers:
+ szager@chromium.org

[leviw_travelin_and_unemployed, 2015-10-27 18:35:34]

LGTM.

Good call on working out a reasonable solution to this issue.

[mstensho (USE GERRIT), 2015-10-28 08:57:08]

Description was changed from

==========
Require spanners to have the multicol container as their nearest block
formatting context.

The editor's draft [1] suggests that a column-span:all object needs to be in
the same block formatting context, so disallow column-span for everything else.
MSIE does the same. Gecko doesn't support spanners.

While this simplifies the code and gives us less to worry about, another reason
to make this change is that we'd get assertion failures if we put a spanner
inside a scrollable flex item inside a flexbox inside a multicol inside another
flexbox. The reason why the assertion failures happen is the
LayoutBlock::finishDelayUpdateScrollInfo() mechanism, which may jump to some
arbitrary block in the subtree and lay it out directly. While that is bad on
its own, the multicol implementation should now at least be immune to damage
caused by that.

Removed some old tests that are now invalid, because they expected spanners to
be created inside scrollable containers. This no longer works, because
non-visible overflow implies a new block formatting context.

[1] https://drafts.csswg.org/css-multicol/#column-span

R=leviw@chromium.org
==========

to

==========
Require spanners to have the multicol container as their nearest block
formatting context.

The editor's draft [1] suggests that a column-span:all object needs to be in
the same block formatting context, so disallow column-span for everything else.
MSIE does the same. Gecko doesn't support spanners.

While this simplifies the code and gives us less to worry about (and it
actually fixes bug 529737, although that wasn't my main motivation here),
another reason to make this change is that we'd get assertion failures if we
put a spanner inside a scrollable flex item inside a flexbox inside a multicol
inside another flexbox. The reason why the assertion failures happen is the
LayoutBlock::finishDelayUpdateScrollInfo() mechanism, which may jump to some
arbitrary block in the subtree and lay it out directly. While that is bad on
its own, the multicol implementation should now at least be immune to damage
caused by that.

Removed some old tests that are now invalid, because they expected spanners to
be created inside scrollable containers. This no longer works, because
non-visible overflow implies a new block formatting context.

[1] https://drafts.csswg.org/css-multicol/#column-span

BUG=529737
R=leviw@chromium.org
==========

[mstensho (USE GERRIT), 2015-10-28 09:13:14]

Turns out that this also fixes bug 529737, so I added a test for that as well.

I think leviw wants szager1 to take a look before I land this, though.

[leviw_travelin_and_unemployed, 2015-10-28 22:15:56]

On 2015/10/28 at 09:13:14, mstensho wrote:
> Turns out that this also fixes bug 529737, so I added a test for that as well.
> 
> I think leviw wants szager1 to take a look before I land this, though.

I wanted him to be aware is all.

[leviw_travelin_and_unemployed, 2015-10-28 22:15:58]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-10-28 22:15:58]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1421423003/#ps20001
(title: "Add test for bug 529737")

[commit-bot: I haz the power, 2015-10-28 22:17:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421423003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421423003/20001

[commit-bot: I haz the power, 2015-10-28 22:50:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-28 22:51:28]

Patchset 2 (id:??) landed as
https://crrev.com/7b65998a89f974e7c7f5dfce4f4eae7fa69b421d
Cr-Commit-Position: refs/heads/master@{#356680}