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).
Created attachment 149034 [details] patch
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?
> 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.
(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.
(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).
(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.
Created attachment 149492 [details] patch
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.
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.
Created attachment 149756 [details] patch
After you get an r+, you can use 'webkit-patch land-safely' to upload a patch that any committer can cq+.
Comment on attachment 149756 [details] patch Clearing flags on attachment: 149756 Committed r121352: <http://trac.webkit.org/changeset/121352>
All reviewed patches have been landed. Closing bug.