Bug 74132

Summary: [CSS Regions] RenderRegion should inherit from RenderBlock
Product: WebKit Reporter: Alan Stearns <stearns>
Component: Layout and RenderingAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, donggwan.kim, eric, gyuyoung.kim, jchaffraix, mibalan, mihnea, ojan.autocc, rakuco, simon.fraser, WebkitBugTracker, webkit.review.bot
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 109228    
Bug Blocks: 57312, 74144, 107880    
Attachments:
Description Flags
Patch 1
webkit.review.bot: commit-queue-
Patch 2
none
Patch for landing none

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-
Patch 2 (28.67 KB, patch)
2013-02-06 02:23 PST, Mihnea Ovidenie
no flags
Patch for landing (21.10 KB, patch)
2013-02-15 04:14 PST, Mihnea Ovidenie
no flags
Mihnea Ovidenie
Comment 1 2013-02-04 05:55:48 PST
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
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.