Bug 74132 - [CSS Regions] RenderRegion should inherit from RenderBlock
Summary: [CSS Regions] RenderRegion should inherit from RenderBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: AdobeTracked
Depends on: 109228
Blocks: 57312 74144 107880
  Show dependency treegraph
 
Reported: 2011-12-08 14:53 PST by Alan Stearns
Modified: 2013-02-15 05:01 PST (History)
12 users (show)

See Also:


Attachments
Patch 1 (21.29 KB, patch)
2013-02-04 05:55 PST, Mihnea Ovidenie
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch 2 (28.67 KB, patch)
2013-02-06 02:23 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (21.10 KB, patch)
2013-02-15 04:14 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Stearns 2011-12-08 14:53:11 PST
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.
Comment 1 Mihnea Ovidenie 2013-02-04 05:55:48 PST
Created attachment 186361 [details]
Patch 1
Comment 2 WebKit Review Bot 2013-02-04 08:15:59 PST
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
Comment 3 WebKit Review Bot 2013-02-04 09:10:51 PST
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
Comment 4 Julien Chaffraix 2013-02-04 10:57:18 PST
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
Comment 5 Mihnea Ovidenie 2013-02-06 02:23:04 PST
Created attachment 186801 [details]
Patch 2
Comment 6 Mihnea Ovidenie 2013-02-06 02:24:46 PST
(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
Comment 7 Julien Chaffraix 2013-02-06 11:21:25 PST
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.
Comment 8 Mihnea Ovidenie 2013-02-07 10:36:19 PST
(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.
Comment 9 Mihnea Ovidenie 2013-02-08 05:50:53 PST
(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.
Comment 10 Julien Chaffraix 2013-02-11 09:31:50 PST
> > > 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.
Comment 11 Mihnea Ovidenie 2013-02-13 12:17:49 PST
(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.
Comment 12 Mihnea Ovidenie 2013-02-13 13:28:46 PST
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
Comment 13 Julien Chaffraix 2013-02-14 09:51:40 PST
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.
Comment 14 Mihnea Ovidenie 2013-02-15 04:14:51 PST
Created attachment 188534 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-02-15 05:01:44 PST
Comment on attachment 188534 [details]
Patch for landing

Clearing flags on attachment: 188534

Committed r142984: <http://trac.webkit.org/changeset/142984>
Comment 16 WebKit Review Bot 2013-02-15 05:01:54 PST
All reviewed patches have been landed.  Closing bug.