Bug 117407 – [CSS Regions] Fix getClientRects() for content nodes

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

[Mihai Balan]

getClientRects() should work as per the spec, returning multiple rects for fragmented content nodes. See http://www.w3.org/TR/css3-regions/#cssomview-getclientrects-and-getboundingclientrect

[Mihai Balan]

Corresponding Blink issue: https://code.google.com/p/chromium/issues/detail?id=248554

[Anton Obzhirov]

I am going to check this bug.

[Andrei Bucur]

Created attachment 230311 [details]
Patch

[Andrei Bucur]

Comment on attachment 230311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230311&action=review

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:846
> +    // If the region range is empty or consists of a single region, fallback to the default behavior. The box is not fragmented.

I'll fix this comment before landing. It's just about the box not having a range at all.

[Dave Hyatt]

Comment on attachment 230311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230311&action=review

r=me

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:861
> +        LayoutRect layoutLocalRect(0, localTop, renderer->borderBoxRectInRegion(region).width(), localBottom - localTop);
> +        LayoutRect fragmentRect = region->rectFlowPortionForBox(renderer, layoutLocalRect);
> +
> +        // We want to skip the 0px height fragments for non-empty boxes that may appear in case the bottom of the box
> +        // overlaps the bottom of a region.
> +        if (localBottom == localTop || fragmentRect.height()) {
> +            CurrentRenderRegionMaintainer regionMaintainer(*region);
> +            quads.append(renderer->localToAbsoluteQuad(FloatRect(fragmentRect), 0 /* mode */, wasFixed));
> +        }

In my opinion, all of this could move to a virtual function on RenderRegion that takes the quads as a reference argument. Then you could keep your code exactly the same, but for region sets I could just add directly to the quads list myself.

[Andrei Bucur]

Created attachment 230370 [details]
Patch for landing

[Andrei Bucur]

Created attachment 230372 [details]
Patch for landing

[Build Bot]

Comment on attachment 230372 [details]
Patch for landing

Attachment 230372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6345695145492480

New failing tests:
fast/multicol/newmulticol/client-rects.html
fast/multicol/client-rects-spanners.html
fast/multicol/client-rects-spanners-complex.html

[Build Bot]

Created attachment 230374 [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.5

[Andrei Bucur]

The patch that devirtualizes RenderFlowThread::absoluteQuadsForBox fails for multi-col (obviously, I forgot to guard for multicol flow threads and return false before doing anything with them).
For now it's more elegant to virtualize both the absoluteQuadsForBox and absoluteQuadsForBoxInRegion. If it happens absoluteQuadsForBox implementations for both multicol and regions match (e.g. spans won't be a problem) then we can merge them in one non-virtual function on RenderFlowThread.

[WebKit Commit Bot]

Comment on attachment 230370 [details]
Patch for landing

Clearing flags on attachment: 230370

Committed r167930: <http://trac.webkit.org/changeset/167930>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.