RESOLVED FIXED 66143
[CSSRegions] RenderFlowThread layout should use the attached region sizes
https://bugs.webkit.org/show_bug.cgi?id=66143
Summary [CSSRegions] RenderFlowThread layout should use the attached region sizes
Alexandru Chiculita
Reported 2011-08-12 09:41:44 PDT
Currently the RenderFlowThread is taking the maximum size of the attached regions. It should flow the content inside the specified region sizes.
Attachments
Patch V1 (29.78 KB, patch)
2011-08-22 03:40 PDT, Alexandru Chiculita
no flags
Patch V2 (29.79 KB, patch)
2011-08-22 04:08 PDT, Alexandru Chiculita
hyatt: review-
hyatt: commit-queue-
Patch V3 (31.68 KB, patch)
2011-08-22 13:16 PDT, Alexandru Chiculita
no flags
Patch V4 (38.52 KB, patch)
2011-08-22 21:03 PDT, Alexandru Chiculita
no flags
Alexandru Chiculita
Comment 1 2011-08-22 03:40:04 PDT
Created attachment 104659 [details] Patch V1 RenderFlowThread::layout will set the current flow thread on its own RenderView. RenderFlowThread cannot be nested inside a single RenderView, so there's no need to keep it on the LayoutState. The RenderFlowThread uses the width of the maximum region, but when it comes to measuring the width of a specific line, the width is adjusted by the difference between the largest region and the region that contains that line.
Alexandru Chiculita
Comment 2 2011-08-22 04:08:38 PDT
Created attachment 104660 [details] Patch V2 Using fixedOffset instead of right in RenderBlock::logicalRightOffsetForLine, so that heightRemaining still works correctly.
Dave Hyatt
Comment 3 2011-08-22 12:08:39 PDT
Comment on attachment 104660 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=104660&action=review Just need some minor tweaks. > Source/WebCore/rendering/RenderBlock.cpp:1229 > LayoutStateMaintainer statePusher(view(), this, locationOffset(), hasColumns() || hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode(), pageLogicalHeight, pageLogicalHeightChanged, colInfo); > > + bool disableRegionFitting = view()->hasRenderFlowThread() && (hasColumns() || (isPositioned() && !isRenderFlowThread()) || isFloating()); > + RegionFittingDisabler regionFittingDisabler(view()->currentRenderFlowThread(), disableRegionFitting); Pull view() into a local since it's being accessed three times here in quick succession. > Source/WebCore/rendering/RenderBlock.cpp:3576 > + if (view()->hasRenderFlowThread()) { > + RenderFlowThread* flowThread = view()->currentRenderFlowThread(); > + if (flowThread->isRegionFittingEnabled()) { > + LayoutState* layoutState = view()->layoutState(); > + IntSize delta = layoutState->m_layoutOffset - layoutState->m_pageOffset; > + int offset = isHorizontalWritingMode() ? delta.height() : delta.width(); > + LayoutUnit regionWidth = flowThread->regionLogicalWidthForLine(offset + logicalTop); > + right -= flowThread->logicalWidth() - regionWidth; > + } > + } Pull this into a helper function. Even a little static function would be nice. I'm not sure if there's any compelling logic behind picking the right side to shrink or if you just made an arbitrary call. It's cool that this gets lines working, but we know long-term that this solution isn't going to work for block splitting. I guess it's better than nothing though. > Source/WebCore/rendering/RenderFlowThread.cpp:466 > + // FIXME: The regions are always in order, optimize this search. Interval tree might work here right? > Source/WebCore/rendering/RenderFlowThread.h:108 > + RenderRegion* renderRegionForLine(LayoutUnit logicalTop) const; This isn't a "logicalTop" really. It's just a position that is examined. I'd rename "logicalTop" to "position" since that matches the other "ForLine" methods in RenderBlock. > Source/WebCore/rendering/RenderRegion.cpp:89 > + if (m_flowThread && isValid()) > + m_flowThread->invalidateRegions(); Can you go from valid to invalid? If so, then you would need an invalidate for that also. I'm thinking you probably can't, but I figured I'd double-check. I'm assuming going from valid to invalid can only happen across a reconstruction of the RenderObject.
Alexandru Chiculita
Comment 4 2011-08-22 12:29:46 PDT
(In reply to comment #3) > (From update of attachment 104660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104660&action=review > Pull view() into a local since it's being accessed three times here in quick succession. Ok. > Pull this into a helper function. Even a little static function would be nice. Ok. > I'm not sure if there's any compelling logic behind picking the right side to shrink or if you just made an arbitrary call. I was thinking that regions stay on the left, and I would need to adjust just the right edge. > It's cool that this gets lines working, but we know long-term that this solution isn't going to work for block splitting. I guess it's better than nothing though. For blocks, I think two passes will be necessary. One with the region width size + region styling applied, and if the block fits inside a single region we're done, if not do a second pass with the maximum region width. I'm not sure how would that work with incremental layout. > > > Source/WebCore/rendering/RenderFlowThread.cpp:466 > > + // FIXME: The regions are always in order, optimize this search. > > Interval tree might work here right? Can that be in a separate bug? I'm sure a simple binary search will do here, because the regions are always in order and never overlapping. Just that we now have a linked list and it would be slow to search the middle region. > This isn't a "logicalTop" really. It's just a position that is examined. I'd rename "logicalTop" to "position" since that matches the other "ForLine" methods in RenderBlock. Ok. > Can you go from valid to invalid? If so, then you would need an invalidate for that also. I'm thinking you probably can't, but I figured I'd double-check. I'm assuming going from valid to invalid can only happen across a reconstruction of the RenderObject. No, there should be no way to get from valid to invalid. The state is required for regions that are invalid when first added. At a later time other regions may be removed meaning that some of the invalid regions may become valid. That's why we need to keep track of invalid regions. Once valid a region cannot go back. A visible issue of this approach might be that one could remove a valid region and reverse the dependencies. When the same region is added back, this time it becomes invalid.
Alexandru Chiculita
Comment 5 2011-08-22 13:16:38 PDT
Created attachment 104722 [details] Patch V3
Dave Hyatt
Comment 6 2011-08-22 13:23:35 PDT
Comment on attachment 104722 [details] Patch V3 r=me
WebKit Review Bot
Comment 7 2011-08-22 14:04:46 PDT
Comment on attachment 104722 [details] Patch V3 Clearing flags on attachment: 104722 Committed r93538: <http://trac.webkit.org/changeset/93538>
WebKit Review Bot
Comment 8 2011-08-22 14:04:51 PDT
All reviewed patches have been landed. Closing bug.
Peter Kasting
Comment 9 2011-08-22 15:52:41 PDT
This seems to have caused the Leopard and SnowLeopard bots, as well as the Chromium Mac bot, to fail fast/repaint/japanese-rl-selection-repaint-in-regions.html . Below I've pasted a diff from the Leopard bot. This looks like it could just be a test whose results need updating, but I don't know anything about this area, so I'm not really sure. The Chromium Mac bots are also failing downstream for the Chromium folks. Since we run pixel tests, there are also pixel diffs in addition to textual diffs. Because ATM neither achicu or dhyatt are around on #webkit to ask about this stuff, I'm rolling this change back for now. --- /Volumes/Data/slave/leopard-intel-debug-tests/build/layout-test-results/fast/repaint/japanese-rl-selection-repaint-in-regions-expected.txt +++ /Volumes/Data/slave/leopard-intel-debug-tests/build/layout-test-results/fast/repaint/japanese-rl-selection-repaint-in-regions-actual.txt @@ -9,8 +9,8 @@ Thread with flow-name 'thread' layer at (400,0) size 400x400 RenderFlowThread at (0,0) size 400x400 - RenderBlock {DIV} at (0,0) size 480x400 - RenderText {#text} at (5,0) size 469x388 + RenderBlock {DIV} at (0,0) size 1248x400 + RenderText {#text} at (5,0) size 1237x388 text run at (5,0) width 381: "\x{305B}\x{3063}\x{304B}\x{304F}\x{898B}\x{3064}\x{3051}\x{305F}\x{3059}\x{3070}\x{3089}\x{3057}\x{3044}\x{8A18}\x{4E8B}\x{304C}\x{3069}\x{3053}" text run at (37,0) width 381: "\x{306B}\x{3042}\x{3063}\x{305F}\x{304B}\x{5FD8}\x{308C}\x{3066}\x{3057}\x{307E}\x{3063}\x{305F}\x{7D4C}\x{9A13}\x{306F}\x{3042}\x{308A}\x{307E}" text run at (69,0) width 360: "\x{3059}\x{304B}\x{306A}\x{3089}\x{30BF}\x{30A4}\x{30C8}\x{30EB}\x{3068}\x{30A2}\x{30C9}\x{30EC}\x{30B9}\x{3060}\x{3051}\x{3067}\x{306A}" @@ -24,8 +24,32 @@ text run at (325,0) width 381: "\x{30DA}\x{30FC}\x{30B8}\x{306E}\x{30B3}\x{30F3}\x{30C6}\x{30F3}\x{30C4}\x{304B}\x{3089}\x{3082}\x{691C}\x{7D22}\x{3059}\x{308B}\x{3053}\x{3068}" text run at (357,0) width 381: "\x{304C}\x{3067}\x{304D}\x{307E}\x{3059}\x{3002}\x{305B}\x{3063}\x{304B}\x{304F}\x{898B}\x{3064}\x{3051}\x{305F}\x{3059}\x{3070}\x{3089}\x{3057}" text run at (389,0) width 381: "\x{3044}\x{8A18}\x{4E8B}\x{304C}\x{3069}\x{3053}\x{306B}\x{3042}\x{3063}\x{305F}\x{304B}\x{5FD8}\x{308C}\x{3066}\x{3057}\x{307E}\x{3063}\x{305F}" - text run at (421,0) width 388: "\x{7D4C}\x{9A13}\x{306F}\x{3042}\x{308A}\x{307E}\x{3059}\x{304B} \x{306A}\x{3089}\x{30BF}\x{30A4}\x{30C8}\x{30EB}\x{3068}\x{30A2}\x{30C9}\x{30EC}" - text run at (453,0) width 191: "\x{30B9}\x{3060}\x{3051}\x{3067}\x{306A}\x{304F}\x{3001}\x{8A2A}\x{554F}" + text run at (421,0) width 22: "\x{7D4C}" + text run at (453,0) width 22: "\x{9A13}" + text run at (485,0) width 22: "\x{306F}" + text run at (517,0) width 22: "\x{3042}" + text run at (549,0) width 22: "\x{308A}" + text run at (581,0) width 22: "\x{307E}" + text run at (613,0) width 22: "\x{3059}" + text run at (645,0) width 22: "\x{304B}" + text run at (677,0) width 22: "\x{306A}" + text run at (709,0) width 22: "\x{3089}" + text run at (741,0) width 22: "\x{30BF}" + text run at (773,0) width 22: "\x{30A4}" + text run at (805,0) width 22: "\x{30C8}" + text run at (837,0) width 22: "\x{30EB}" + text run at (869,0) width 22: "\x{3068}" + text run at (901,0) width 22: "\x{30A2}" + text run at (933,0) width 22: "\x{30C9}" + text run at (965,0) width 22: "\x{30EC}" + text run at (997,0) width 22: "\x{30B9}" + text run at (1029,0) width 22: "\x{3060}" + text run at (1061,0) width 22: "\x{3051}" + text run at (1093,0) width 22: "\x{3067}" + text run at (1125,0) width 22: "\x{306A}" + text run at (1157,0) width 43: "\x{304F}\x{3001}" + text run at (1189,0) width 22: "\x{8A2A}" + text run at (1221,0) width 22: "\x{554F}" Regions for flow 'thread' RenderRegion {DIV} with index 0 selection start: position 28 of child 0 {#text} of child 1 {DIV} of body
Peter Kasting
Comment 10 2011-08-22 16:15:29 PDT
Reverted r93538 for reason: Broke Leopard, SnowLeopard, and Chromium Mac bots Committed r93556: <http://trac.webkit.org/changeset/93556>
Alexandru Chiculita
Comment 11 2011-08-22 20:22:18 PDT
(In reply to comment #10) > Reverted r93538 for reason: > > Broke Leopard, SnowLeopard, and Chromium Mac bots > > Committed r93556: <http://trac.webkit.org/changeset/93556> Sorry about that, that test would need to be updated, but I'm not sure why it didn't show up when I run the tests or the chromium-bot did. For example, it isn't here http://queues.webkit.org/results/9467898 .
Alexandru Chiculita
Comment 12 2011-08-22 21:03:08 PDT
Created attachment 104786 [details] Patch V4 Extended the last region width to all the content that doesn't fit into the regions and that will also fix the test.
Dave Hyatt
Comment 13 2011-08-23 12:21:38 PDT
Comment on attachment 104786 [details] Patch V4 r=me
WebKit Review Bot
Comment 14 2011-08-23 12:44:56 PDT
Comment on attachment 104786 [details] Patch V4 Clearing flags on attachment: 104786 Committed r93627: <http://trac.webkit.org/changeset/93627>
WebKit Review Bot
Comment 15 2011-08-23 12:45:01 PDT
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.