Bug 119135 – [CSS Regions] Anonymous nested regions

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Catalin badea]

This is a prototype for a new approach on how region renderers are created and handled. The idea is to have the block elements with the region property create their normal renderer and add a region renderer as its only child. The nested region does not have a node and takes the size of the container. Also, the parent should be prevented from adding other children, with the exception of pseudo elements like :before and :after.

[Catalin badea]

Created attachment 207512 [details]
wip patch

[Catalin badea]

Created attachment 208304 [details]
WIP

[Darin Adler]

Comment on attachment 208304 [details]
WIP

Looks like a backwards patch.

[Catalin badea]

Created attachment 208319 [details]
WIP

[Catalin badea]

Created attachment 208559 [details]
WIP

[Build Bot]

Comment on attachment 208559 [details]
WIP

Attachment 208559 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1453242

New failing tests:
fast/regions/flow-body-in-html.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/overflow-not-moving-below-floats-in-variable-width-regions.html
fast/regions/flows-dependency-same-flow.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
http/tests/inspector/inspect-element.html
fast/regions/flow-content-basic-vertical.html
fast/regions/shape-inside/shape-inside-with-region-borders.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html
fast/regions/autoheight-regions-mark.html
fast/regions/overflow-in-variable-width-regions.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html
fast/regions/multiple-directionality-changes-in-variable-width-regions.html
fast/regions/offsetLeft-offsetTop-in-multiple-regions.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/overflow-in-uniform-regions-dynamic.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html
fast/regions/overflow-moving-below-floats-in-variable-width-regions.html
fast/regions/flow-content-basic.html
fast/regions/flows-dependency-dynamic-remove.html
fast/repaint/line-flow-with-floats-in-regions.html
fast/repaint/region-painting-invalidation.html
fast/repaint/region-painting-via-layout.html
fast/regions/overflow-rtl-in-variable-width-regions.html
fast/regions/flow-content-basic-vertical-rl.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html
fast/loader/javascript-url-in-object.html
fast/repaint/overflow-flipped-writing-mode-block-in-regions.html

[Build Bot]

Created attachment 208572 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Catalin badea]

Created attachment 209858 [details]
WIP

[Build Bot]

Comment on attachment 209858 [details]
WIP

Attachment 209858 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1628294

New failing tests:
fast/regions/flow-body-in-html.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/flows-dependency-same-flow.html
fast/regions/layers/dynamic-layer-removed-with-no-layout.html
http/tests/inspector/inspect-element.html
fast/regions/flow-content-basic-vertical.html
fast/regions/shape-inside/shape-inside-with-region-borders.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html
fast/regions/autoheight-regions-mark.html
fast/regions/layers/regions-promoted-to-layers-vertical-rl.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html
fast/regions/multiple-directionality-changes-in-variable-width-regions.html
fast/regions/offsetLeft-offsetTop-in-multiple-regions.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/overflow-in-uniform-regions-dynamic.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html
fast/regions/flow-content-basic.html
fast/regions/flows-dependency-dynamic-remove.html
fast/repaint/line-flow-with-floats-in-regions.html
fast/repaint/region-painting-invalidation.html
fast/repaint/region-painting-via-layout.html
fast/regions/layers/regions-promoted-to-layers.html
fast/regions/flow-content-basic-vertical-rl.html
fast/regions/layers/dynamic-layer-added-with-no-layout.html
fast/regions/layers/regions-promoted-to-layers-vertical-lr.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html
fast/repaint/overflow-flipped-writing-mode-block-in-regions.html
fast/regions/layers/regions-promoted-to-layers-horizontal-bt.html
fast/regions/content-flowed-into-pseudo-regions.html

[Build Bot]

Created attachment 209889 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Darin Adler]

Comment on attachment 209858 [details]
WIP

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

> Source/WebCore/rendering/RenderObject.h:645
> +    Node* generatingNode() const
> +    {
> +        if (isRenderRegion() && parent())
> +            return parent()->node();
> +        return isPseudoElement() ? generatingPseudoHostElement() : node();
> +    }

Can this return Element*, or could it actually be a non-Element node in some case?

This function is getting kind of long. Does it really need to be inlined? If it does, can we at least put it outside the class definition?

[Build Bot]

Comment on attachment 209858 [details]
WIP

Attachment 209858 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1621325

New failing tests:
fast/regions/flow-body-in-html.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/flows-dependency-same-flow.html
fast/regions/layers/dynamic-layer-removed-with-no-layout.html
http/tests/inspector/inspect-element.html
fast/regions/flow-content-basic-vertical.html
fast/regions/shape-inside/shape-inside-with-region-borders.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-overflow-multiple-shapes.html
fast/regions/autoheight-regions-mark.html
fast/regions/layers/regions-promoted-to-layers-vertical-rl.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-multiple-shapes.html
fast/regions/multiple-directionality-changes-in-variable-width-regions.html
fast/regions/offsetLeft-offsetTop-in-multiple-regions.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-overflow-bottom-positioned-multiple-shapes.html
fast/regions/overflow-in-uniform-regions-dynamic.html
fast/regions/shape-inside/shape-inside-on-regions-block-content-basic-overflow-shape-top-offset.html
fast/regions/flow-content-basic.html
fast/regions/flows-dependency-dynamic-remove.html
fast/repaint/line-flow-with-floats-in-regions.html
fast/repaint/region-painting-invalidation.html
fast/repaint/region-painting-via-layout.html
fast/regions/layers/regions-promoted-to-layers.html
fast/regions/flow-content-basic-vertical-rl.html
fast/regions/layers/dynamic-layer-added-with-no-layout.html
fast/regions/layers/regions-promoted-to-layers-vertical-lr.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
fast/regions/shape-inside/shape-inside-on-regions-inline-content-basic-overflow-shape-top-offset.html
fast/repaint/overflow-flipped-writing-mode-block-in-regions.html
fast/regions/layers/regions-promoted-to-layers-horizontal-bt.html
fast/regions/content-flowed-into-pseudo-regions.html

[Build Bot]

Created attachment 209911 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209858 [details]
WIP

Attachment 209858 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1592795

[Mihnea Ovidenie]

Created attachment 212464 [details]
WIP2 patch

WIP2 patch

[Mihnea Ovidenie]

Created attachment 213588 [details]
WIP3

[Mihnea Ovidenie]

Created attachment 213697 [details]
WIP4

[WebKit Commit Bot]

Attachment 213697 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/table-caption-as-region-expected.html', u'LayoutTests/fast/regions/table-caption-as-region.html', u'LayoutTests/fast/regions/table-cell-as-region-expected.html', u'LayoutTests/fast/regions/table-cell-as-region.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/WebKitNamedFlow.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h', u'Source/WebCore/rendering/RenderElement.cpp', u'Source/WebCore/rendering/RenderElement.h', u'Source/WebCore/rendering/RenderFlowThread.cpp', u'Source/WebCore/rendering/RenderListItem.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.cpp', u'Source/WebCore/rendering/RenderMultiColumnSet.h', u'Source/WebCore/rendering/RenderNamedFlowFragment.cpp', u'Source/WebCore/rendering/RenderNamedFlowFragment.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderRegion.cpp', u'Source/WebCore/rendering/RenderRegion.h', u'Source/WebCore/rendering/RenderTableCaption.cpp', u'Source/WebCore/rendering/RenderTreeAsText.cpp', u'Source/WebCore/rendering/shapes/ShapeInfo.h', u'Source/WebCore/style/StyleResolveTree.cpp']" exit_code: 1
Source/WebCore/inspector/InspectorOverlay.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/rendering/RenderElement.h:256:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 213697 [details]
WIP4

Attachment 213697 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3750100

[Mihnea Ovidenie]

Created attachment 213761 [details]
Patch

[Build Bot]

Comment on attachment 213761 [details]
Patch

Attachment 213761 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3766032

[Dave Hyatt]

Comment on attachment 213761 [details]
Patch

Is RenderBLockFlow the correct place? You don't intend to allow flexboxes to be regions?

[Mihnea Ovidenie]

(In reply to comment #22)
> (From update of attachment 213761 [details])
> Is RenderBLockFlow the correct place? You don't intend to allow flexboxes to be regions?

No, the flexbox element and the grid element will not become regions, as described in the spec:
"The flex and grid display values do not result in block containers (they are defined as flex containers and grid elements, respectively). So ‘flow-from’ combined with those display values does not result in a CSS Region."

http://dev.w3.org/csswg/css-regions/#the-flow-from-property.

[Mihnea Ovidenie]

Created attachment 214275 [details]
Patch 2

New patch with just the basic functionality. Region styling code still in RenderRegion and can be moved in a follow up patch, same for autoheight.

[Build Bot]

Comment on attachment 214275 [details]
Patch 2

Attachment 214275 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3482050

[Mihnea Ovidenie]

Created attachment 214344 [details]
Patch 3

Same patch with Win build fix as shapes are not enabled by default on Win.

[Dave Hyatt]

Comment on attachment 214344 [details]
Patch 3

r=me

[Mihnea Ovidenie]

Created attachment 214433 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 214433 [details]
Patch for landing

Clearing flags on attachment: 214433

Committed r157567: <http://trac.webkit.org/changeset/157567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Maerean]

Comment on attachment 214433 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:9
> +               fast/regions/table-cell-as-region.html

if list items can now be CSS Regions, you can add tests for that too.

> Source/WebCore/GNUmakefile.list.am:4394
> +    Source/WebCore/rendering/RenderNamedFlowFragment.cpp \

nit: the indentation for these lines should be the same as for the rest of the lines in the file.

> Source/WebCore/inspector/InspectorOverlay.cpp:394
> +    RenderBlockFlow* regionContainer = toRenderBlockFlow(region->parent());

maybe we could have a RenderBlockFlow operator in RenderRegion that returns parent().

> Source/WebCore/rendering/RenderListItem.cpp:84
> +    RenderBlockFlow::insertedIntoTree();

if list items can now be CSS Regions, you can add tests for that too.

> Source/WebCore/rendering/RenderNamedFlowFragment.h:40
> +class RenderNamedFlowFragment : public RenderRegion {

1. it would really help if you add a short comment explaining what use cases this class enables and how the render tree looks after this class was introduced.

2. in my opinion, the RenderAnonymousRegion name does a better job of explaining what it does than RenderNamedFlowFragment. The CSS Break specification defines a fragment as "the portion of a box that belongs to exactly one fragmentainer". This class is not about that concept.

> Source/WebCore/rendering/RenderTreeAsText.cpp:578
> +        if (!o.isRenderNamedFlowFragmentContainer()) {

This hides the renderers of the anonymous regions from the output, which doesn't happen for the other types of anonymous renderers in the tree.

[Mihnea Ovidenie]

(In reply to comment #31)
> (From update of attachment 214433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214433&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +               fast/regions/table-cell-as-region.html
> 
> if list items can now be CSS Regions, you can add tests for that too.
> 

Filed https://bugs.webkit.org/show_bug.cgi?id=122961

> > Source/WebCore/GNUmakefile.list.am:4394
> > +    Source/WebCore/rendering/RenderNamedFlowFragment.cpp \
> 
> nit: the indentation for these lines should be the same as for the rest of the lines in the file.
> 
> > Source/WebCore/inspector/InspectorOverlay.cpp:394
> > +    RenderBlockFlow* regionContainer = toRenderBlockFlow(region->parent());
> 
> maybe we could have a RenderBlockFlow operator in RenderRegion that returns parent().
> 
> > Source/WebCore/rendering/RenderListItem.cpp:84
> > +    RenderBlockFlow::insertedIntoTree();
> 
> if list items can now be CSS Regions, you can add tests for that too.
> 
> > Source/WebCore/rendering/RenderNamedFlowFragment.h:40
> > +class RenderNamedFlowFragment : public RenderRegion {
> 
> 1. it would really help if you add a short comment explaining what use cases this class enables and how the render tree looks after this class was introduced.
> 
> 2. in my opinion, the RenderAnonymousRegion name does a better job of explaining what it does than RenderNamedFlowFragment. The CSS Break specification defines a fragment as "the portion of a box that belongs to exactly one fragmentainer". This class is not about that concept.
> 
> > Source/WebCore/rendering/RenderTreeAsText.cpp:578
> > +        if (!o.isRenderNamedFlowFragmentContainer()) {
> 
> This hides the renderers of the anonymous regions from the output, which doesn't happen for the other types of anonymous renderers in the tree.

Filed https://bugs.webkit.org/show_bug.cgi?id=122963, as it would mean changing a lot of tests.

[Andreas Kling]

I think this may have regressed Parser/html5-full-render by ~1.1% and Parser/html-parser by ~2%:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%2C%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml-parser%3ATime%22%5D%5D

[Andreas Kling]

Comment on attachment 214433 [details]
Patch for landing

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

> Source/WebCore/dom/Element.cpp:2764
>      document().updateLayoutIgnorePendingStylesheets();

Unrelated, but it's weird that this function forces a layout even if the CSS regions are disabled.

> Source/WebCore/rendering/RenderBlockFlow.cpp:280
> +    if (namedFlowFragmentNeedsUpdate())
> +        relayoutChildren = true;

Could we avoid calling namedFlowFragmentNeedsUpdate() if relayoutChildren is already true?
I feel like this block of code could be structured differently to avoid unnecessary work.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2371
> +    if (document().cssRegionsEnabled() && style()->isDisplayRegionType() && !style()->regionThread().isEmpty()) {

I think we should put the cssRegionsEnabled() check at the head of this function.
There's no need to check anything else if the feature is disabled.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2408
> +    if (!m_rareData)
> +        m_rareData = adoptPtr(new RenderBlockFlowRareData(this));

This pattern seems to repeat a lot. It should be done in a dedicated function, e.g:
RenderBlockFlowRareData& RenderBlockFlow::ensureRareData()

> Source/WebCore/rendering/RenderBlockFlow.h:98
> +            , m_renderNamedFlowFragment(0)

0 -> nullptr

> Source/WebCore/rendering/RenderElement.h:263
> +    if (isRenderNamedFlowFragment() && parent())

We should swap the order of these checks to avoid the virtual call when possible.

> Source/WebCore/rendering/RenderNamedFlowFragment.h:40
> +class RenderNamedFlowFragment : public RenderRegion {

Unless you are going to inherit from this class, it should be marked FINAL.

> Source/WebCore/rendering/RenderNamedFlowFragment.h:48
> +    virtual bool isRenderNamedFlowFragment() const OVERRIDE FINAL { return true; }
> +    virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);

These should be private.
styleDidChange() needs OVERRIDE

> Source/WebCore/rendering/RenderNamedFlowFragment.h:55
> +protected:
> +    virtual bool shouldHaveAutoLogicalHeight() const;

When you mark the class FINAL, this should be marked with private.
Also it needs OVERRIDE.

> Source/WebCore/rendering/RenderNamedFlowFragment.h:58
> +    virtual const char* renderName() const { return "RenderNamedFlowFragment"; }

Needs OVERRIDE.

> Source/WebCore/rendering/RenderObject.cpp:2567
> +bool RenderObject::isRenderNamedFlowFragmentContainer() const
> +{
> +    if (!isRenderBlockFlow())
> +        return false;
> +
> +    return toRenderBlockFlow(this)->renderNamedFlowFragment();
> +}

This function belongs in RenderElement. We should move it there to avoid an extra branch in isRenderBlockFlow().

[Andrei Bucur]

Ok, I'm going to look at this now. Stay tuned.