Bug 117407

Summary: [CSS Regions] Fix getClientRects() for content nodes
Product: WebKit Reporter: Mihai Balan <mibalan>
Component: CSSAssignee: Andrei Bucur <abucur>
Severity: Normal CC: abucur, buildbot, commit-queue, esprehn+autocc, glenn, kondapallykalyan, mihnea, obzhirov, rniwa, sam, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128165    
Description Flags
Patch for landing
Patch for landing
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 none

Description Mihai Balan 2013-06-10 07:22:54 PDT
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
Comment 1 Mihai Balan 2013-06-11 08:42:33 PDT
Corresponding Blink issue: https://code.google.com/p/chromium/issues/detail?id=248554
Comment 2 Anton Obzhirov 2013-07-08 05:38:12 PDT
I am going to check this bug.
Comment 3 Andrei Bucur 2014-04-28 11:09:15 PDT
Created attachment 230311 [details]
Comment 4 Andrei Bucur 2014-04-28 11:11:37 PDT
Comment on attachment 230311 [details]

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.
Comment 5 Dave Hyatt 2014-04-28 11:16:51 PDT
Comment on attachment 230311 [details]

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


> 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.
Comment 6 Andrei Bucur 2014-04-29 05:56:41 PDT
Created attachment 230370 [details]
Patch for landing
Comment 7 Andrei Bucur 2014-04-29 06:17:09 PDT
Created attachment 230372 [details]
Patch for landing
Comment 8 Build Bot 2014-04-29 07:07:06 PDT
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:
Comment 9 Build Bot 2014-04-29 07:07:10 PDT
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
Comment 10 Andrei Bucur 2014-04-29 07:29:09 PDT
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.
Comment 11 WebKit Commit Bot 2014-04-29 07:55:16 PDT
Comment on attachment 230370 [details]
Patch for landing

Clearing flags on attachment: 230370

Committed r167930: <http://trac.webkit.org/changeset/167930>
Comment 12 WebKit Commit Bot 2014-04-29 07:55:20 PDT
All reviewed patches have been landed.  Closing bug.