Bug 65627

Created attachment 102797 [details] Patch

Comment on attachment 102797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102797&action=review > Source/WebCore/rendering/RenderFlowThread.cpp:188 > +void RenderFlowThread::computeLogicalWidth() This seems odd to me. Does this mean that if you have one narrow region and one wide region, the content will be flowed to the width of the widest region? Shouldn't the width actually vary from region to region? > Source/WebCore/rendering/RenderFlowThread.cpp:197 > + int regionWidth = region->contentWidth(); > + if (regionWidth > width) > + width = regionWidth; I'd just write this as width = max(width, region->contentWidth()); > Source/WebCore/rendering/RenderFlowThread.cpp:211 > + int regionHeight = region->contentHeight(); > + height += regionHeight; Don't really understand the need for a local here. Just add region->contentHeight() to height directly. > Source/WebCore/rendering/RenderFlowThread.cpp:217 > +void RenderFlowThread::paintRegion(PaintInfo& paintInfo, const LayoutRect& regionRect, const LayoutPoint& paintOffset) It seems like you're propagating a mistake of columns here by clipping to the region bounds. I don't think overflow should be clipped by default, should it? That's actually a mistake columns make. Also, what about RenderLayers? I don't see how you can just do a RenderBlock paint here, since that won't include descendants with layers. > Source/WebCore/rendering/RenderRegion.cpp:90 > + // Delegate painting of content in region to RenderFlowThread. > + adjustedPaintOffset.move(paddingLeft() + borderLeft(), paddingTop() + borderTop()); > + m_flowThread->paintRegion(paintInfo, regionRect(), adjustedPaintOffset); Seems like you just want to paint the flow thread's RenderLayer (it will always have one since you absolutely positioned it) with an offset and clip applied. I'll give you a pass on clipping for now, since columns make the same mistake, and I guess regions can make the same mistake as well.

(In reply to comment #2) > (From update of attachment 102797 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102797&action=review > > > Source/WebCore/rendering/RenderFlowThread.cpp:188 > > +void RenderFlowThread::computeLogicalWidth() > > This seems odd to me. Does this mean that if you have one narrow region and one wide region, the content will be flowed to the width of the widest region? Shouldn't the width actually vary from region to region? The width will indeed vary from region to region, we thought we will fix that in the following patches. Should we add a test for this issue with this patch? > > > Source/WebCore/rendering/RenderFlowThread.cpp:197 > > + int regionWidth = region->contentWidth(); > > + if (regionWidth > width) > > + width = regionWidth; > > I'd just write this as width = max(width, region->contentWidth()); Sure, i missed that. > > > Source/WebCore/rendering/RenderFlowThread.cpp:211 > > + int regionHeight = region->contentHeight(); > > + height += regionHeight; > > Don't really understand the need for a local here. Just add region->contentHeight() to height directly. Right. > > > Source/WebCore/rendering/RenderFlowThread.cpp:217 > > +void RenderFlowThread::paintRegion(PaintInfo& paintInfo, const LayoutRect& regionRect, const LayoutPoint& paintOffset) > > It seems like you're propagating a mistake of columns here by clipping to the region bounds. I don't think overflow should be clipped by default, should it? That's actually a mistake columns make. > > Also, what about RenderLayers? I don't see how you can just do a RenderBlock paint here, since that won't include descendants with layers. I thought that adding support for RenderLayers should be another patch, what do you think? > > > Source/WebCore/rendering/RenderRegion.cpp:90 > > + // Delegate painting of content in region to RenderFlowThread. > > + adjustedPaintOffset.move(paddingLeft() + borderLeft(), paddingTop() + borderTop()); > > + m_flowThread->paintRegion(paintInfo, regionRect(), adjustedPaintOffset); > > Seems like you just want to paint the flow thread's RenderLayer (it will always have one since you absolutely positioned it) with an offset and clip applied. > > I'll give you a pass on clipping for now, since columns make the same mistake, and I guess regions can make the same mistake as well. Should i add a bug for this issue? Maybe fill a bug for columns too?

We had a lot of discussions today related to how to render the layers. Similar to columns we wanted to layout with the RenderFlowThread, but this time paint needs to happen inside the RenderRegions. Currently the RenderFlowThread is not collecting its layers, because there was a need to hide them initially. The idea was that we could somehow collect all the layers to inside the RenderRegion that actually needed to paint them, so that painting goes as normal. It seems like the code is pretty tightly integrated with the parent() and the renderer()->parent(), so it would need a serious amount of change in order to support that. It would also mean that a simple layer position change would need to recalculate the parent region. On the other hand we can revert the RenderFlowThread to collect its layers and do the painting only if the RenderRegion is requesting it. One big issue that is still valid is about the GPU composited layers. Those are automatically created and painted in a different software stack. On some platforms we would need to create multiple GPU layers for the same GraphicLayer. That's because it needs to render in multiple place at the same time. About clipping. I'm not sure which is the best way to do this. My current idea on that is to render by only clipping between regions. For example if a box is positioned between two regions, a part of it will render in the first region and the other part in the second region. Nothing will be painted outside the regions. However, if it is placed with "left:-40px" for example, it will not be clipped on the left margin of the region and will also paint outside of it. I'm sure the spec should specify how positioned objects are placed inside the flow and how are transforms going to affect them. More interesting would be to know what happens with positioned elements that are bigger then a region.

You could force all RenderFlowThreads to be stacking contexts (give them a z-index of 0 along with absolute positioning), and then let them collect layers. Those layers will all be held cleanly by the RenderFlowThread itself and won't propagate out of the RenderFlowThread. You'll have to then hack painting in RenderLayer to not paint the RenderFlowThread layers from the document's root layer. Once you've done that you'll have a complete layer tree in each RenderFlowThread, and then the problem of painting them in some other place is a bit like the multi-column problem. RenderRegion would then always paint the entire RenderFlowThread layer tree but with the appropriate transform and clip applied. For now I would just clip content to region bounds like you're already doing. It's what we do for columns as well. It's not strictly correct, but let's just do that for now to get things up and running. We can address overflow out of regions later.

Comment on attachment 102797 [details] Patch The other issue I see here is that you're not dealing with writing modes and directionality and are assuming a simple vertical pagination model. I posted to the CSS WG asking how the initial writing mode and direction for regions are determined, i.e., whether you honor the writing mode and directionality per-region or if there is a single master writing-mode/direction.

Created attachment 103680 [details] Patch (work in progress) This patch improves upon the original posted one. Things that I changed include: (1) Fixed a broken test case that said content:from instead of the right syntax. (2) Added test cases for other writing modes. (3) Fixed RenderFlowThread's style to inherit from RenderView and to properly update when RenderView's style changed, so that things like the zoom factor and writing mode would be honored in the flow thread. (4) Rewrote all of the layout functions in RenderFlowThread and painting functions to be writing-mode-aware. (5) Added new test cases for vertical writing modes. (6) Fixed some asserts in RenderFlowThread by making sure RenderRegion claims to be replaced. There's still a problem that I can see with RenderRegions in that you can construct a situation where a RenderFlowThread gets a layout before a RenderRegion does if that RenderRegion goes into the RenderView's positioned objects list after the RenderFlowThread. We will eventually need to customize RenderView's layout I think to make sure that RenderFlowThreads get a layout after the regions.

> There's still a problem that I can see with RenderRegions in that you can construct a situation where a RenderFlowThread gets a layout before a RenderRegion does if that RenderRegion goes into the RenderView's positioned objects list after the RenderFlowThread. We will eventually need to customize RenderView's layout I think to make sure that RenderFlowThreads get a layout after the regions. We had that fixed in our labs prototype. I will post a different bug for it. About writing mode, can we just use what the flowed element specifies? For example: <div style="flow:into(flow1); writing-mode: vertical-rl"></div> would actually make the RenderFlowThread use that direction. I think we don't need to layout the regions positions from the beginning. We can just stack them and "initialize" their position, when needed based on the current writing mode of the elements. For example if two divs were to be flowed into the same RenderFlowThread with two different writing modes: the first region will be used as "width*height" while the second one will use the "height*width".

Created attachment 103720 [details] Picture for vertical text I've illustrated the two regions I was writing about in Comment #8.

(In reply to comment #9) > Created an attachment (id=103720) [details] > Picture for vertical text > > I've illustrated the two regions I was writing about in Comment #8. That isn't how pagination works for printing when you mix writing modes or for multi-column. In both of those cases there is a single dominant writing mode that dictates how all pages (or columns) are paginated. It doesn't matter what the elements you're paginating do... if content changes writing modes it has no effect on the direction or orientation of the pagination. I raised the writing-mode issue with the CSS WG and Vincent Hardy agreed that there should be a single pagination direction/orientation for regions. I suggested a couple of ideas... use the initial containing block or use the the first region. The implementation in the patch above uses the initial containing block. I think it is a good idea to actually track the region rectangles, since otherwise at paint time, you would have to walk the entire list to compute your region's position. You also have to know the logical height of the entire RenderFlowThread in order to flip for vertical-rl writing modes. For example, in a worst case scenario if you don't cache region rectangles, and the paint order of the regions is the exact opposite of the layout order, then you could have an O(n^2) crawl to compute each region's rectangle for painting. I think overall the initial patch is solid once the writing mode issue is fixed. We can handle using RenderLayer painting in a followup.

Created attachment 103764 [details] Patch -> Updated changelogs. -> Updated the test results. Also some tests had no regions attached to flows. That means that the RenderFlowThread now would have got a size of 0 by 0, so no output was generated in the render tree. Fixed that by adding regions to the old tests. Added bugs for the other issues: Layers: https://bugs.webkit.org/show_bug.cgi?id=66130 Layout regions first: https://bugs.webkit.org/show_bug.cgi?id=66129 Hit testing: https://bugs.webkit.org/show_bug.cgi?id=66133

Comment on attachment 103764 [details] Patch Looks good. r=me.

Comment on attachment 103764 [details] Patch Clearing flags on attachment: 103764 Committed r92969: <http://trac.webkit.org/changeset/92969>

All reviewed patches have been landed. Closing bug.