Bug 118665 – [CSS Regions] Implement visual overflow for first & last regions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Radu Stavila]

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

[Radu Stavila]

Created attachment 207082 [details]
Patch

[Mihai Maerean]

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.

[Radu Stavila]

Right, missed that, thanks.

[Radu Stavila]

Created attachment 207085 [details]
Patch

[Mihai Maerean]

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.

[Radu Stavila]

Created attachment 207087 [details]
Patch integrating feedback

[Dave Hyatt]

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.

[Dave Hyatt]

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).

[Dave Hyatt]

Also, you should be propagating all overflow and not just visual overflow. Layout overflow needs to propagate too.

[Radu Stavila]

Layout overflow is being implemented as we speak in a different patch.

[Radu Stavila]

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?

[Dave Hyatt]

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.

[Radu Stavila]

Created attachment 210374 [details]
New approach - WIP

[Radu Stavila]

There are still some failing tests, this is just a WIP. Please review the approach.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Radu Stavila]

Created attachment 210620 [details]
WIP - source files only (tests still failing, ignore)

[Radu Stavila]

Created attachment 210621 [details]
WIP - source files only (tests still failing, ignore)

[Dave Hyatt]

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.

[Radu Stavila]

Created attachment 216380 [details]
Patch (src only, will add tests in a separate attachment, for easier review).

[Radu Stavila]

Created attachment 216383 [details]
Complete patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

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.

[Radu Stavila]

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.

[Radu Stavila]

Created attachment 216816 [details]
Patch integrating reviewer feedback

[Radu Stavila]

Created attachment 216931 [details]
Added automation for testing selection in regions overflow

[Dave Hyatt]

Comment on attachment 216931 [details]
Added automation for testing selection in regions overflow

r=me

[Radu Stavila]

Created attachment 216934 [details]
Patch recreated with --binary and updated new repaint test

[Build Bot]

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

[Build Bot]

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

[Radu Stavila]

Created attachment 216960 [details]
Fixed automated selection test for both WK1 and WK2

[Radu Stavila]

Created attachment 217028 [details]
Added "reviewed by" in CL

[WebKit Commit Bot]

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

[Radu Stavila]

Created attachment 217047 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 217047 [details]
Patch for landing

Clearing flags on attachment: 217047

Committed r159337: <http://trac.webkit.org/changeset/159337>

[Philippe Normand]

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.

[Javier Fernandez]

(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.

[Radu Stavila]

Created attachment 217426 [details]
Patch with fix for GTK

[Radu Stavila]

Created attachment 217430 [details]
Recreated with --binary

[Radu Stavila]

Created attachment 217432 [details]
Rebased CL

[Radu Stavila]

Created attachment 217440 [details]
Updated TestExpectations for GTK/EFL, patch for landing

[WebKit Commit Bot]

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>

[Jon Lee]

*** Bug 134302 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

*** Bug 134457 has been marked as a duplicate of this bug. ***