Bug 89759 – [CSSRegions]Change display values that allow regions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andrei Onea]

Current CSSRegions draft permits non-replaced elements with display values of block, inline-block, table-cell, table-caption, and list-item to become regions. See: http://dev.w3.org/csswg/css3-regions (Note 5).

[Andrei Onea]

Created attachment 149034 [details]
patch

[Tony Chang]

Comment on attachment 149034 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149034&action=review

Are there existing tests for changing the display type dynamically in javascript? Do we properly re-create the RenderObject in that case?

> Source/WebCore/ChangeLog:9
> +        Allow only elements with display values of block, inline-block,
> +        table-cell, table-caption and list-item to become regions.

Can you include a link to the spec here?

> Source/WebCore/rendering/style/RenderStyle.h:1511
> +    { // Only non-replaced block elements can become a region.
> +        return display() == BLOCK || display() == INLINE_BLOCK
> +            || display() == TABLE_CELL || display() == TABLE_CAPTION
> +            || display() == LIST_ITEM;

What about table, box, flexbox, grid and their inline versions?

[Andrei Onea]

> Are there existing tests for changing the display type dynamically in javascript? Do we properly re-create the RenderObject in that case?
Not yet. I will add a test in the next iteration of the patch. The RenderObject, however, is always recreated when changing the display property value. (Node::diff checks for different display values, and forces the node to detach when they are different).

> > Source/WebCore/ChangeLog:9
> > +        Allow only elements with display values of block, inline-block,
> > +        table-cell, table-caption and list-item to become regions.
> 
> Can you include a link to the spec here?

Will do.
 
> > Source/WebCore/rendering/style/RenderStyle.h:1511
> > +    { // Only non-replaced block elements can become a region.
> > +        return display() == BLOCK || display() == INLINE_BLOCK
> > +            || display() == TABLE_CELL || display() == TABLE_CAPTION
> > +            || display() == LIST_ITEM;
> 
> What about table, box, flexbox, grid and their inline versions?

I'm not sure about that yet, I will come back on it.

[Alan Stearns]

(In reply to comment #2)
> 
> What about table, box, flexbox, grid and their inline versions?

The current spec says that flow-from applies to non-replaced block containers.

A table is a block-level box but it is not a block-level container (according to CSS 2.1 section 9.2.1) so it is not a valid flow-from target.

I am not familiar with display:box.

Grid might work. The "Grid Layout" version of the specification is not specific about whether display:grid creates a block container, but the "Grid Template Layout" version does say that a grid is a block container. I'll add that value to the note.

The flexbox specification says that it is definitely not a block container. So for now it's not a valid flow-from target.

Inline versions of anything are not valid flow-from targets.

[Tony Chang]

(In reply to comment #4)
> I am not familiar with display:box.

It's the old flexbox spec. If we can't flow-from a display:flex, then we probably shouldn't flow from a display: box.

> Grid might work. The "Grid Layout" version of the specification is not specific about whether display:grid creates a block container, but the "Grid Template Layout" version does say that a grid is a block container. I'll add that value to the note.

This might be an oversight. It probably doesn't make sense to flow from a display:grid for the same reasons you can't flow from a flexbox (the grid itself doesn't have content, it just controls placement of its children).

[Alan Stearns]

(In reply to comment #5)
>It probably doesn't make sense to flow from a display:grid

After thinking about how this might work for a few minutes, I think I agree. Let's not do display:grid for now. We're leaving open the possibility of allowing flow-from to apply to more things in the future.

[Andrei Onea]

Created attachment 149492 [details]
patch

[Tony Chang]

Comment on attachment 149492 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149492&action=review

> LayoutTests/fast/regions/script-tests/region-element-display-change.js:26
> +shouldBeFalse('testElement(element, "inline-table")');
> +shouldBeTrue('testElement(element, "table-cell")');
> +shouldBeTrue('testElement(element, "table-caption")');
> +shouldBeTrue('testElement(element, "list-item")');

Nit: I would go ahead and add all the other display types here.

> Source/WebCore/rendering/RenderObject.cpp:154
> +    if (doc->cssRegionsEnabled() && style->isDisplayRegionType() && !style->regionThread().isEmpty() && doc->renderView())
> +        return new (arena) RenderRegion(node, doc->renderView()->flowThreadController()->ensureRenderFlowThreadWithName(style->regionThread()));

Please watch the perf results at http://webkit-perf.appspot.com/ after this lands. I don't expect there to be a regression, but it's good to be ready.

[Tony Chang]

Comment on attachment 149492 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149492&action=review

>> LayoutTests/fast/regions/script-tests/region-element-display-change.js:26
>> +shouldBeFalse('testElement(element, "inline-table")');
>> +shouldBeTrue('testElement(element, "table-cell")');
>> +shouldBeTrue('testElement(element, "table-caption")');
>> +shouldBeTrue('testElement(element, "list-item")');
> 
> Nit: I would go ahead and add all the other display types here.

Nit: I would go ahead and add all the other display types here.

>> Source/WebCore/rendering/RenderObject.cpp:154
>> +    if (doc->cssRegionsEnabled() && style->isDisplayRegionType() && !style->regionThread().isEmpty() && doc->renderView())
>> +        return new (arena) RenderRegion(node, doc->renderView()->flowThreadController()->ensureRenderFlowThreadWithName(style->regionThread()));
> 
> Please watch the perf results at http://webkit-perf.appspot.com/ after this lands. I don't expect there to be a regression, but it's good to be ready.

Please watch the perf results at http://webkit-perf.appspot.com/ after this lands. I don't expect there to be a regression, but it's good to be ready.

> Source/WebCore/rendering/style/RenderStyle.h:1510
> +    { // Only non-replaced block elements can become a region.

Nit: I would probably remove this comment since it doesn't really explain 'why'.  The ChangeLog explains the why.

[Andrei Onea]

Created attachment 149756 [details]
patch

[Tony Chang]

After you get an r+, you can use 'webkit-patch land-safely' to upload a patch that any committer can cq+.

[WebKit Review Bot]

Comment on attachment 149756 [details]
patch

Clearing flags on attachment: 149756

Committed r121352: <http://trac.webkit.org/changeset/121352>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.