WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
74132
[CSS Regions] RenderRegion should inherit from RenderBlock
https://bugs.webkit.org/show_bug.cgi?id=74132
Summary
[CSS Regions] RenderRegion should inherit from RenderBlock
Alan Stearns
Reported
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.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mihnea Ovidenie
Comment 1
2013-02-04 05:55:48 PST
Created
attachment 186361
[details]
Patch 1
WebKit Review Bot
Comment 2
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
WebKit Review Bot
Comment 3
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
Julien Chaffraix
Comment 4
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
Mihnea Ovidenie
Comment 5
2013-02-06 02:23:04 PST
Created
attachment 186801
[details]
Patch 2
Mihnea Ovidenie
Comment 6
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
Julien Chaffraix
Comment 7
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.
Mihnea Ovidenie
Comment 8
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.
Mihnea Ovidenie
Comment 9
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.
Julien Chaffraix
Comment 10
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.
Mihnea Ovidenie
Comment 11
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.
Mihnea Ovidenie
Comment 12
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
Julien Chaffraix
Comment 13
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.
Mihnea Ovidenie
Comment 14
2013-02-15 04:14:51 PST
Created
attachment 188534
[details]
Patch for landing
WebKit Review Bot
Comment 15
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
>
WebKit Review Bot
Comment 16
2013-02-15 05:01:54 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug