Bug 74132 – [CSS Regions] RenderRegion should inherit from RenderBlock

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alan Stearns]

There are some Regions behaviors in the specification that depend on this change. I'll make separate dependent bug reports for the ones I know about.

[Mihnea Ovidenie]

Created attachment 186361 [details]
Patch 1

[WebKit Review Bot]

Comment on attachment 186361 [details]
Patch 1

Attachment 186361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373229

New failing tests:
fast/regions/selecting-text-through-different-region-flows.html

[WebKit Review Bot]

Comment on attachment 186361 [details]
Patch 1

Attachment 186361 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373244

New failing tests:
fast/regions/selecting-text-through-different-region-flows.html

[Julien Chaffraix]

Comment on attachment 186361 [details]
Patch 1

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

> LayoutTests/ChangeLog:12
> +        * fast/regions/flows-dependency-dynamic-remove.html: As a block, an empty region can self collapse,
> +        which was not possible for a replaced element. I used '-webkit-margin-collapse: separate' to prevent
> +        self collapsing for regions and avoid recreating the expectations.

Depending on the expected default behavior, you may want to add some code in StyleResolver::adjustRenderStyle.

> Source/WebCore/rendering/RenderBox.cpp:2921
> +        computePositionedLogicalWidthReplaced(computedValues); // FIXME: Patch for regions when we add replaced element support.

If you can improve the FIXME, it would be nice. Currently it's difficult to assess whether it is fixed or not.

> Source/WebCore/rendering/RenderRegion.cpp:251
>      StackStats::LayoutCheckPoint layoutCheckPoint;
> -    RenderReplaced::layout();
> +    ASSERT(needsLayout());
> +
> +    RenderBlock::layout();

You will do 2 layout checks in the new code (one above and one inside RenderBlock::layout()).

I think you should override layoutBlock, not layout anymore.

> Source/WebCore/rendering/RenderRegion.h:140
> +    virtual bool canHaveChildren() const { return false; }

OVERRIDE

> Source/WebCore/rendering/RenderRegion.h:153
> +    virtual void layout();
> +    virtual void paintObject(PaintInfo&, const LayoutPoint&);

OVERRIDE

[Mihnea Ovidenie]

Created attachment 186801 [details]
Patch 2

[Mihnea Ovidenie]

(In reply to comment #4)
> (From update of attachment 186361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186361&action=review
> 
> > LayoutTests/ChangeLog:12
> > +        * fast/regions/flows-dependency-dynamic-remove.html: As a block, an empty region can self collapse,
> > +        which was not possible for a replaced element. I used '-webkit-margin-collapse: separate' to prevent
> > +        self collapsing for regions and avoid recreating the expectations.
> 
> Depending on the expected default behavior, you may want to add some code in StyleResolver::adjustRenderStyle.
> 

I want regions to self-collapse just as other blocks.

> > Source/WebCore/rendering/RenderBox.cpp:2921
> > +        computePositionedLogicalWidthReplaced(computedValues); // FIXME: Patch for regions when we add replaced element support.
> 
> If you can improve the FIXME, it would be nice. Currently it's difficult to assess whether it is fixed or not.
> 

Done.

> > Source/WebCore/rendering/RenderRegion.cpp:251
> >      StackStats::LayoutCheckPoint layoutCheckPoint;
> > -    RenderReplaced::layout();
> > +    ASSERT(needsLayout());
> > +
> > +    RenderBlock::layout();
> 
> You will do 2 layout checks in the new code (one above and one inside RenderBlock::layout()).
> 
> I think you should override layoutBlock, not layout anymore.
> 

Done

> > Source/WebCore/rendering/RenderRegion.h:140
> > +    virtual bool canHaveChildren() const { return false; }
> 
> OVERRIDE
> 

Done

> > Source/WebCore/rendering/RenderRegion.h:153
> > +    virtual void layout();
> > +    virtual void paintObject(PaintInfo&, const LayoutPoint&);
> 
> OVERRIDE

Done

[Julien Chaffraix]

Comment on attachment 186801 [details]
Patch 2

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

r- about the paintObject issue that needs to be investigated and justified / documented (and tested!).

> LayoutTests/ChangeLog:18
> +        * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> +        * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> +        * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> +        * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt:

These updates are just magic to me. How come an <h1> disappears?

> Source/WebCore/rendering/RenderBox.cpp:2927
> +        // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 )

Missing final sentence dot.

> Source/WebCore/rendering/RenderRegion.cpp:147
> -    if (!m_flowThread || !isValid())
> +    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground))

By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's  content).

paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.

[Mihnea Ovidenie]

(In reply to comment #7)
> (From update of attachment 186801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review
> 
> r- about the paintObject issue that needs to be investigated and justified / documented (and tested!).
> 
> > LayoutTests/ChangeLog:18
> > +        * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> 
> These updates are just magic to me. How come an <h1> disappears?
> 

After the RenderRegion is derived from RenderBlock, when we simulate a click at coordinates (1,1) at the end of the test, the first RenderRegion object (div) is a valid candidate in function VisiblePosition::canonicalPosition(..).

When the region was based on RenderReplaced, the first valid candidate was the H1 element from the flow thread. It seemed to me that the easiest way to change the expected results for this test.
Another option was to convert the test to a ref test, but the previous option seemed faster.

> > Source/WebCore/rendering/RenderBox.cpp:2927
> > +        // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 )
> 
> Missing final sentence dot.
> 
> > Source/WebCore/rendering/RenderRegion.cpp:147
> > -    if (!m_flowThread || !isValid())
> > +    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground))
> 
> By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's  content).
> 
> paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.

RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call:
layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects)

and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly.

My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times.

[Mihnea Ovidenie]

(In reply to comment #7)
> (From update of attachment 186801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186801&action=review
> 
> r- about the paintObject issue that needs to be investigated and justified / documented (and tested!).
> 
> > LayoutTests/ChangeLog:18
> > +        * platform/chromium-win/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/efl/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/gtk/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> > +        * platform/qt/fast/regions/selecting-text-through-different-region-flows-expected.txt:
> 
> These updates are just magic to me. How come an <h1> disappears?
> 
> > Source/WebCore/rendering/RenderBox.cpp:2927
> > +        // with variable width regions (see https://bugs.webkit.org/show_bug.cgi?id=69896 )
> 
> Missing final sentence dot.
> 
> > Source/WebCore/rendering/RenderRegion.cpp:147
> > -    if (!m_flowThread || !isValid())
> > +    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground))
> 
> By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's  content).
> 
> paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.

We have converted LayoutTest/fast/regions/selecting-text-through-different-region-flows.html into a ref test in https://bugs.webkit.org/show_bug.cgi?id=109228.

I also filed a bug https://bugs.webkit.org/show_bug.cgi?id=109274 about the selection highlighting in regions.

[Julien Chaffraix]

> > > Source/WebCore/rendering/RenderRegion.cpp:147
> > > -    if (!m_flowThread || !isValid())
> > > +    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground))
> > 
> > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's  content).
> > 
> > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.
> 
> RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call:
> layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects)
> 
> and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly.
> 
> My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times.

Posting my comment here for archive's sake, RenderLayer paints its associated renderer only if it's self-painting. You are making the assumption that the flow thread's layer is self-painting but I couldn't understand what guarantees it. Apart from that, your logic makes sense and is comment-worthy.

[Mihnea Ovidenie]

(In reply to comment #10)
> > > > Source/WebCore/rendering/RenderRegion.cpp:147
> > > > -    if (!m_flowThread || !isValid())
> > > > +    if (!m_flowThread || !isValid() || (paintInfo.phase != PaintPhaseForeground))
> > > 
> > > By limiting yourself to the foreground phase, you are breaking a lot of cases! (e.g. outline and selection on your flow thread's  content).
> > > 
> > > paintReplaced was also called for PaintPhaseSelection which the new code doesn't handle, nor does it explain why it's OK to do so. If you are temporarily breaking some paint phase, this should at least have a FIXME.
> > 
> > RenderFlowThread requires a layer and is using the layer mechanism to paint its collected children. When RenderRegion has the paint function called, it also calls RenderFlowThread::paintFlowThreadPortionInRegion which will further call:
> > layer()->paint(context, info.rect, 0, 0, region, RenderLayer::PaintLayerTemporaryClipRects)
> > 
> > and so on. The RenderFlowThread's layer will further call paintLayer -> paintLayerContentsAndReflection -> paintLayerContents that will go through all the paint phases for the flow thread collected content. Therefore, the outline for the render flow thread content will be painted properly.
> > 
> > My intention with (paintInfo.phase != PaintPhaseForeground) was to prevent painting of the flow thread content multiple times.
> 
> Posting my comment here for archive's sake, RenderLayer paints its associated renderer only if it's self-painting. You are making the assumption that the flow thread's layer is self-painting but I couldn't understand what guarantees it. Apart from that, your logic makes sense and is comment-worthy.

RenderFlowThread is a self-painting layer. When we create the RFT object, we associate a style for it and, as can be seen in RenderFlowThread::createFlowThreadStyle, the RFT object is an absolutely positioned one. Because of that, RenderLayer::shouldBeNormalFlowOnly() returns false, and RenderLayer::shouldBeSelfPaintingLayer() returns true.

[Mihnea Ovidenie]

Regarding the outlines for the content that is flowed into regions, there are some tests available:
LayoutTests/fast/regions/region-styling-mediaquery.html
LayoutTests/fast/regions/outline-sides-in-region.html
LayoutTests/fast/regions/hit-test-region.html

[Julien Chaffraix]

Comment on attachment 186801 [details]
Patch 2

Great explanations, thanks for digging this up. Please amend the ChangeLog and paintObject with the information from this bug as it explains why the whole change (and your optimization) works and is properly covered.

[Mihnea Ovidenie]

Created attachment 188534 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188534 [details]
Patch for landing

Clearing flags on attachment: 188534

Committed r142984: <http://trac.webkit.org/changeset/142984>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.