Bug 74144 – [CSS Regions] Elements in a region should be assignable to a named flow

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Alan Stearns]

Created attachment 118497 [details]
reftest

It should be possible to redirect the children of an element that becomes a region to a different named flow. Currently this does not work - all of the region children are not displayed, even if there is flow-into and flow-from styling that should cause them to be displayed.

[Vincent Hardy]

Submitted test to W3C:

https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants.xht

and 

https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants-ref.xht

[Mihai Balan]

*** Bug 103685 has been marked as a duplicate of this bug. ***

[Mihai Maerean]

Created attachment 196131 [details]
patch

[Stephen Chenney]

Comment on attachment 196131 [details]
patch

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

SVG bits seem fine. Can't comment on the rest.

> Source/WebCore/dom/Text.cpp:-305
> -    if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) {

Why did you change this? It was cleaner before and it seems functionally equivalent.

[Mihai Maerean]

Comment on attachment 196131 [details]
patch

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

>> Source/WebCore/dom/Text.cpp:-305
>> -    if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) {
> 
> Why did you change this? It was cleaner before and it seems functionally equivalent.

The previous code here threw a compile time error saying that "a non-const l-value NodeRenderingContext cannot be passed on a temporary object on the stack".
I have thought about how to keep the old code as it clearly more elegant, but the compiler disagreed.

[Andrei Bucur]

Comment on attachment 196131 [details]
patch

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

> LayoutTests/ChangeLog:8
> +        * fast/regions/flow-body-in-html-expected.txt: Added.

Let's try to make use of check-layout.js in the tests. If not possible, ref tests are also a possibility. Layout text dumps are not portable between platforms and should be avoided.

> Source/WebCore/ChangeLog:8
> +        DOM children of a region must not be rendered as children of the region, but can be collected in flow threads.

An overview of the approach taken to solve the bug would be helpful.

> Source/WebCore/dom/Text.cpp:305
> +    if (!textRenderer) {

What changed here? Why the refactoring?

> Source/WebCore/rendering/RenderRegion.h:148
> +    virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; }

This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well?

[Mihai Maerean]

Comment on attachment 196131 [details]
patch

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

>> Source/WebCore/rendering/RenderRegion.h:148
>> +    virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; }
> 
> This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well?

This method ensures that this bug is fixed for good and other features in webkit can avoid it just by overriding the method.

Using a virtual functions ensures that other features (newmulticol/overflow fragments) can decide their behavior without having to find the line of code in NodeRenderingContext.cpp where they would otherwise need to add code for themselves.

[Mihnea Ovidenie]

Comment on attachment 196131 [details]
patch

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

> LayoutTests/ChangeLog:14
> +

For all the tests, you can use the dumpAsText() output.
if (window.testRunner)
    testRunner.dumpAsText()

In general, i would make all the tests for 74144, even those for the 103685. Specifying links to both bugs seems confusing to me.

> Source/WebCore/ChangeLog:9
> +

I would like to see a description of the general approach taken to solve this bug.

> Source/WebCore/ChangeLog:16
> +        (WebCore):

I was told to remove the lines like the above from the Changelogs.

> Source/WebCore/dom/NodeRenderingContext.cpp:181
> +    return 0;

This should follow the same pattern like: return m_renderingParent ? m_renderingParent->renderer() : 0;

[WebKit Review Bot]

Comment on attachment 196131 [details]
patch

Attachment 196131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17394078

New failing tests:
fast/regions/flow-body-in-html.html
fast/regions/universal-selector-children-to-the-same-region.html
fast/regions/region-content-flown-into-region.html

[WebKit Review Bot]

Created attachment 196148 [details]
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Mihnea Ovidenie]

Comment on attachment 196131 [details]
patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:173
> +    if (m_node->isElementNode() && toElement(m_node)->moveToFlowThreadIsNeeded()) {

Is there a way to cache the result of calling styleForRenderer() for m_node and pass it to moveToFlowThreadIsNeeded() and also initialize m_style member in this class? You are calling styleForRenderer() a lot and i think you can reduce the necessary calls.

[WebKit Review Bot]

Comment on attachment 196131 [details]
patch

Attachment 196131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17240831

New failing tests:
fast/regions/flow-body-in-html.html
fast/regions/universal-selector-children-to-the-same-region.html
fast/regions/region-content-flown-into-region.html

[WebKit Review Bot]

Created attachment 196167 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Mihai Maerean]

Comment on attachment 196131 [details]
patch

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

> Source/WebCore/dom/Element.cpp:2573
> +    // FIXME: Do not collect elements if they are in shadow tree.
>      if (isInShadowTree())
>          return false;

I have carried this FIXME text along but it seems the code below fixes the problem.
I will remove this FIXME comment.
Please comment if you think otherwise.

[Mihai Maerean]

Created attachment 196601 [details]
patch that incorporates the feedback.

[Mihnea Ovidenie]

(In reply to comment #15)
> Created an attachment (id=196601) [details]
> patch that incorporates the feedback.

Ok, we need David Hyatt's input on this.

[Dave Hyatt]

Comment on attachment 196601 [details]
patch that incorporates the feedback.

r=me

[WebKit Commit Bot]

Comment on attachment 196601 [details]
patch that incorporates the feedback.

Clearing flags on attachment: 196601

Committed r147756: <http://trac.webkit.org/changeset/147756>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

This patch is causing the performance regression we can see today on the performance bots. On my tests, I could measure a slowdown of 9.5% on the Parser/html5-full-render.html test.

These are my runs (ms, lower is better):

r147911
4642.6008261
4641.89847605
4643.60051905

r147911 (with this patch reverted)
4239.35488146
4234.28493692
4251.07173051


For the record:

This is the shortest range of "suspects" by crossing the information from the performance bots.

http://trac.webkit.org/log/?rev=147758&stop_rev=147752&verbose=on

[Mihai Maerean]

I'm rolling out the patch as part of https://bugs.webkit.org/show_bug.cgi?id=114176

[Mihai Maerean]

Created attachment 198890 [details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5%

Running the performance tests on 2 machines gets only 0.7-1.0% bigger times instead of the 9.5% of the previous patch.

The big performance impact of the previous patch was caused by always setting m_style = element->styleForRenderer() in NodeRenderingContext::createRendererForElementIfNeeded() even if the style was already computed and stored in m_style.

[Dave Hyatt]

Comment on attachment 198890 [details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5%

r=me

[WebKit Commit Bot]

Comment on attachment 198890 [details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5%

Clearing flags on attachment: 198890

Committed r148865: <http://trac.webkit.org/changeset/148865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions.

[Simon Fraser (smfr)]

Maybe RenderView::flowThreadController() should return nil until a named flow is actually encountered, and that can be used as the trigger to hit slow code.

[Mihai Maerean]

(In reply to comment #26)
> The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions.

When parsing a page without regions you need to compute the style to read the "flow" properties to know whether the page has regions.
So this is a logical loop.
In short, how do we detect that the page doesn't use regions without computing the style (note that NodeRenderingContext is used when parsing the page)?

[Simon Fraser (smfr)]

Why does moveToFlowThreadIsNeeded() need to every compute style itself? When does the renderer not have a style?

[Mihai Maerean]

I've been actively discussing this problem at https://codereview.chromium.org/15027005/ with Julien Chaffraix. When the discussion comes to a conclusion I will post the conclussions here too (and a patch).

[Mihai Maerean]

As Simon Fraser suggested that there could be less calls to Element::styleForRenderer(), I'm working on it at https://bugs.webkit.org/show_bug.cgi?id=117637

[Mihai Maerean]

Closing the bug, as Simon's concerns are not applicable on the latest code.

[Mihnea Ovidenie]

Reopened after http://trac.webkit.org/changeset/166353 temporarily removed support for this. Changeset http://trac.webkit.org/changeset/166632 added a test for that.

[Brent Fulgham]

CSS Regions were removed in Bug 174978.