Created attachment 207082 [details]
Patch
Comment on attachment 207082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207082&action=review > Source/WebCore/rendering/InlineFlowBox.h:108 > + void clearKnownToHaveNoOverflowForSelfAndChildren() OVERRIDE; Please add "virtual" at the beginning of the line. Right, missed that, thanks. Created attachment 207085 [details]
Patch
Comment on attachment 207085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207085&action=review > Source/WebCore/rendering/RenderFlowThread.cpp:380 > + if (!RenderBox::addVisualOverflow(rect)) RenderFlowThread derives from RenderBlock. If RenderBlock will override addVisualOverflow, this call will skip that code and will only call the method from RenderBox. Please change this to RenderBlock::addVisualOverflow. > Source/WebCore/rendering/RenderRegion.cpp:107 > + if (!RenderBox::addVisualOverflow(rect)) RenderRegion derives from RenderBlock. If RenderBlock will override addVisualOverflow, this call will skip that code and will only call the method from RenderBox. Please change this to RenderBlock::addVisualOverflow. Created attachment 207087 [details]
Patch integrating feedback
Comment on attachment 207087 [details] Patch integrating feedback View in context: https://bugs.webkit.org/attachment.cgi?id=207087&action=review r- for a number of writing-mode issues and the weird parent walk that could just use containing blocks. I think you might find that you could use a simpler approach here though. Instead of trying to write your own propagation code, I think instead you should just mark RenderRegions for a simplified layout pass if you detect that the RenderFlow has overflow that needs to propagate. If you do that, then the layout code can just handle it. You can have RenderRegion when it computes its overflow ask the flow thread if it's in a good state to hand the overflow back and if so, get it from the flow thread. I think this will be much less code, and you won't have to add all this code to try to propagate outside of layout etc. It also won't work anyway, since the layer code has cached some overflow stuff that you're not fixing up. > Source/WebCore/rendering/RenderLayer.cpp:4434 > + LayoutRect hitTestArea; > + if (isOutOfFlowRenderFlowThread()) { > + RenderFlowThread* renderFlowThread = toRenderFlowThread(renderer()); > + hitTestArea = renderFlowThread->hasVisualOverflow() ? renderFlowThread->visualOverflowRect() : renderFlowThread->borderBoxRect(); > + } else > + hitTestArea = renderer()->view()->documentRect(); > + This is wrong. Overflow is not physical. It is in the coordinate space of the block, so you can't use visualOverflowRect without flipping it for writing mode. > Source/WebCore/rendering/RenderRegion.cpp:103 > +void RenderRegion::addVisualOverflowFromFlowThreadOverflow() > +{ > + LayoutRect flowThreadRect = flowThreadPortionOverflowRect(); > + flowThreadRect.expand(borderLeft(), borderTop()); > + flowThreadRect.expand(paddingLeft(), paddingTop()); > + addVisualOverflow(flowThreadRect); > +} This is not correct either. In the case of flipped blocks, the border and padding can be on the bottom and right. Also, if we're going to support the writing mode of the region differing from the writing mode of the flow thread (do we support that now?), then this code isn't going to be correct either. I don't think you necessarily have to get that right, but you should at least switch to using the correct border sides for flipped block writing modes. > Source/WebCore/rendering/RenderRegion.cpp:120 > + RenderObject* objParent = renderChild->parent(); > + ASSERT(objParent); > + > + // Skip non-box parents (for instance a region sitting inside a span). > + while (!objParent->isBox()) > + objParent = objParent->parent(); Just use containingBlock(). Overflow follows the containing block hierarchy. This will simplify all of this code. To elaborate, look into simplifiedNormalFlowLayout and the corresponding bits. All simplifiedNormalFlowLayout does in the most basic cases is recompute overflow, so I think you could leverage it, and then let the normal simplifiedlayout process handle the overflow propagation up the RenderRegion containing block chain. Then all you have to do in RenderRegion is make sure its compute overflow method grabs the overflow it needs from the flow thread if the flow thread is in a good state (which it will be by the time you do the simplified layout). Also, you should be propagating all overflow and not just visual overflow. Layout overflow needs to propagate too. Layout overflow is being implemented as we speak in a different patch. So, would it be ok for now to implement the changes you requested (the writing direction issues and the parent walk) or just scrap it all and do it the other way, using simplified layout? I would like to see the simplified layout approach used instead. I think it's fragile to have a separate code path for overflow propagation, since that's just another piece of code we'd have to maintain and patch. Created attachment 210374 [details]
New approach - WIP
There are still some failing tests, this is just a WIP. Please review the approach. Comment on attachment 210374 [details] New approach - WIP Attachment 210374 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1693340 New failing tests: compositing/contents-opaque/overflow-hidden-child-layers.html fast/multicol/transform-inside-opacity.html compositing/geometry/foreground-layer.html fast/multicol/newmulticol/float-multicol.html fast/clip/009.html fast/regions/overflow-last-region-vert-lr.html fast/multicol/newmulticol/float-paginate.html fast/multicol/newmulticol/cell-shrinkback.html http/tests/inspector/inspect-element.html fast/block/colspan-under-button-crash.html fast/multicol/newmulticol/hide-box-horizontal-bt.html fast/regions/overflow-last-region-horiz-bt.html fast/multicol/newmulticol/float-avoidance.html fast/flexbox/clear-overflow-before-scroll-update.html fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html fast/multicol/newmulticol/float-paginate-complex.html fast/clip/010.html fast/clip/011.html fast/clip/012.html fast/multicol/newmulticol/float-paginate-empty-lines.html fast/regions/overflow-last-region-vert-rl.html fast/multicol/newmulticol/layers-in-multicol.html fast/forms/input-placeholder-text-indent.html fast/multicol/newmulticol/direct-child-column-span-all.html css3/flexbox/button.html fast/multicol/newmulticol/columns-shorthand-parsing.html compositing/geometry/clipping-foreground.html fast/forms/input-placeholder-paint-order-2.html fast/regions/bottom-overflow-out-of-first-region-absolute.html fast/multicol/newmulticol/column-rules-fixed-height.html Created attachment 210404 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Comment on attachment 210374 [details] New approach - WIP Attachment 210374 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1683012 New failing tests: fast/multicol/newmulticol/columns-shorthand-parsing.html compositing/geometry/clipping-foreground.html compositing/contents-opaque/overflow-hidden-child-layers.html compositing/geometry/foreground-layer.html fast/multicol/newmulticol/column-rules-fixed-height.html fast/multicol/newmulticol/direct-child-column-span-all.html fast/clip/009.html fast/multicol/newmulticol/float-avoidance.html fast/flexbox/clear-overflow-before-scroll-update.html fast/multicol/newmulticol/float-multicol.html http/tests/misc/submit-post-keygen.html http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html fast/multicol/newmulticol/cell-shrinkback.html fast/clip/010.html css3/flexbox/button.html http/tests/inspector/inspect-element.html fast/clip/011.html Created attachment 210438 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Created attachment 210620 [details]
WIP - source files only (tests still failing, ignore)
Created attachment 210621 [details]
WIP - source files only (tests still failing, ignore)
Comment on attachment 210621 [details] WIP - source files only (tests still failing, ignore) View in context: https://bugs.webkit.org/attachment.cgi?id=210621&action=review > Source/WebCore/rendering/RenderBlock.cpp:3351 > + // Check our region range to make sure we need to be painting in this region. > + if (paintInfo.renderRegion && !paintInfo.renderRegion->flowThread()->objectInFlowRegion(this, paintInfo.renderRegion)) > + return; Can't you catch this earlier? > Source/WebCore/rendering/RenderBox.cpp:2084 > + if (o->isRenderFlowThread()) { > + RenderRegion* firstRegion = 0; > + RenderRegion* lastRegion = 0; > + toRenderFlowThread(o)->getBoxRegionRangeForOverflow(this, firstRegion, lastRegion); > + if (firstRegion) > + rect.moveBy(firstRegion->flowThreadPortionRect().location()); > + } Probably other places you will need to patch. > Source/WebCore/rendering/RenderLayer.cpp:482 > if (flags & CheckForRepaint) { > if (!renderer().view().printing()) { > + bool repaintRegion = false; > if (m_repaintStatus & NeedsFullRepaint) { > renderer().repaintUsingContainer(repaintContainer, pixelSnappedIntRect(oldRepaintRect)); > - if (m_repaintRect != oldRepaintRect) > + if (m_repaintRect != oldRepaintRect) { > renderer().repaintUsingContainer(repaintContainer, pixelSnappedIntRect(m_repaintRect)); > - } else if (shouldRepaintAfterLayout()) > + > + if (renderer().isRenderRegion()) > + repaintRegion = true; > + } > + } else if (shouldRepaintAfterLayout()) { > renderer().repaintAfterLayoutIfNeeded(repaintContainer, oldRepaintRect, oldOutlineBox, &m_repaintRect, &m_outlineBox); > + > + if (renderer().isRenderRegion()) > + repaintRegion = true; > + } > + > + // If we just repainted a region, we must also repaint the flow thread since it is the one > + // doing the actual painting of the flowed content. > + if (repaintRegion) { > + RenderRegion* region = toRenderRegion(&renderer()); > + if (region->flowThread()) > + region->flowThread()->layer()->repaintIncludingDescendants(); > + } Maybe think about doing the region work in a helper function, even if it duplicates a couple of lines of code. > Source/WebCore/rendering/RenderLayer.cpp:1658 > + // If this is a region, then the painting is actually done by its flow thread's layer. > + if (layer->renderer().isRenderRegion()) { > + RenderRegion* region = toRenderRegion(&layer->renderer()); > + RenderLayer* flowThreadLayer = region->flowThread()->layer(); > + if (!layer->reflection() || layer->reflectionLayer() != flowThreadLayer) { > + LayoutRect flowThreadClipRect = transparencyClipBox(flowThreadLayer, rootLayer, transparencyBehavior, DescendantsOfTransparencyClipBox, paintBehavior); > + > + LayoutPoint offsetFromRoot; > + layer->convertToLayerCoords(flowThreadLayer, offsetFromRoot); > + > + LayoutSize moveOffset = (offsetFromRoot + region->contentBoxRect().location()) - region->flowThreadPortionRect().location(); > + flowThreadClipRect.move(moveOffset); > + > + clipRect.unite(flowThreadClipRect); > + } > + } Big enough to warrant a helper function I think. > Source/WebCore/rendering/RenderLayer.cpp:4002 > + ClipRect regionClipRect; > + if (localPaintingInfo.rootLayer != this && parent()) { > + ClipRectsContext clipRectsContext(localPaintingInfo.rootLayer, toRenderRegion(&renderer()), > + (localPaintFlags & PaintLayerTemporaryClipRects) ? TemporaryClipRects : PaintingClipRects, > + IgnoreOverlayScrollbarSize, (isPaintingOverflowContents) ? IgnoreOverflowClip : RespectOverflowClip); > + regionClipRect = backgroundClipRect(clipRectsContext); > + } else > + regionClipRect = paintDirtyRect; > + > + paintFlowThreadForRegion(context, localPaintingInfo, localPaintFlags, offsetFromRoot, regionClipRect); > + } Helper function. Created attachment 216380 [details]
Patch (src only, will add tests in a separate attachment, for easier review).
Created attachment 216383 [details]
Complete patch
Comment on attachment 216380 [details] Patch (src only, will add tests in a separate attachment, for easier review). Attachment 216380 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22738477 New failing tests: fast/regions/element-in-named-flow-fixed-from-absolute.html fast/regions/top-overflow-out-of-second-region.html fast/regions/float-pushed-width-change-2.html fast/regions/webkit-flow-float-unable-to-push.html fast/regions/bottom-overflow-out-of-first-region.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html fast/regions/float-pushed-width-change.html fast/regions/element-in-named-flow-absolute-from-fixed.html fast/regions/overflow-scrollable-rotated-fragment.html Created attachment 216390 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 216380 [details] Patch (src only, will add tests in a separate attachment, for easier review). Attachment 216380 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22508538 New failing tests: fast/regions/element-in-named-flow-fixed-from-absolute.html fast/regions/top-overflow-out-of-second-region.html fast/regions/float-pushed-width-change-2.html fast/regions/webkit-flow-float-unable-to-push.html fast/regions/bottom-overflow-out-of-first-region.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/regions/counters/extract-ordered-lists-in-regions-explicit-counters-005.html fast/regions/float-pushed-width-change.html fast/regions/element-in-named-flow-absolute-from-fixed.html fast/regions/overflow-scrollable-rotated-fragment.html Created attachment 216397 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216380 [details]
Patch (src only, will add tests in a separate attachment, for easier review).
This patch looks like it is breaking overflow for multicolumn, so that's the reason for the minus. Your options are basically to try to preserve the original behavior, or to just make the change to actually do overflow for columns. Columns were basically using the flow thread overflow and intersecting that with a rect that extended halfway into the column gap for interior columns. The repaintFlowThreadContentRectangle handled this, since you could pass in an overflow rect for repainting and have the right thing happen.
If you decide you really want to try to do visual overflow for columns, the tricky bit is that each column needs a RenderOverflow, and not just the single RenderMultiColumnSet as a whole. The overflow portion rect in the flow thread is the same as per a region with the addition of the "clip halfway into the column gap" rule for interior edges (so first and last column don't have their exterior edges clipped).
I'm fine with overflow not being implemented for columns as long as you don't break it though.
The repaintRect parameter passed to the repaintFlowThreadContentRectangle already contains the flow thread's overflow in this situation. I tested the repainting of region-based multicol by setting negative top-margin on the content in the multicol and moving the multicol around on :hover. Everything works the same in both versions (with and without the patch). Is there a particular scenario or test-case that you have in mind on which to test this issue? Thanks. Created attachment 216816 [details]
Patch integrating reviewer feedback
Created attachment 216931 [details]
Added automation for testing selection in regions overflow
Comment on attachment 216931 [details]
Added automation for testing selection in regions overflow
r=me
Created attachment 216934 [details]
Patch recreated with --binary and updated new repaint test
Comment on attachment 216934 [details] Patch recreated with --binary and updated new repaint test Attachment 216934 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/23618009 New failing tests: fast/regions/overflow-nested-regions.html Created attachment 216955 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 216960 [details]
Fixed automated selection test for both WK1 and WK2
Created attachment 217028 [details]
Added "reviewed by" in CL
Comment on attachment 217028 [details] Added "reviewed by" in CL Rejecting attachment 217028 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 217028, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Dave Hyatt found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/23958080 Created attachment 217047 [details]
Patch for landing
Comment on attachment 217047 [details] Patch for landing Clearing flags on attachment: 217047 Committed r159337: <http://trac.webkit.org/changeset/159337> Sorry but I had to roll this patch out as it broke tests on EFL and GTK. I am CCing Rego and Javi Fernandez, they might be able to help fix the issue on GTK at least. (In reply to comment #41) > Sorry but I had to roll this patch out as it broke tests on EFL and GTK. I am CCing Rego and Javi Fernandez, they might be able to help fix the issue on GTK at least. I'll take care of it. Created attachment 217426 [details]
Patch with fix for GTK
Created attachment 217430 [details]
Recreated with --binary
Created attachment 217432 [details]
Rebased CL
Created attachment 217440 [details]
Updated TestExpectations for GTK/EFL, patch for landing
Comment on attachment 217440 [details] Updated TestExpectations for GTK/EFL, patch for landing Clearing flags on attachment: 217440 Committed r159609: <http://trac.webkit.org/changeset/159609> *** Bug 134302 has been marked as a duplicate of this bug. *** *** Bug 134457 has been marked as a duplicate of this bug. *** |
Created attachment 206653 [details] Test-case There are 3 problems in the attached test: - the overflow in the 1st named flow is cut at the tile border - the overflow in the 2nd named flow is not visible due to the region having position:relative and as such, a self-painting layer - the overflow is not selectable