Bug 117407 - [CSS Regions] Fix getClientRects() for content nodes
Summary: [CSS Regions] Fix getClientRects() for content nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords:
Depends on:
Blocks: 128165
  Show dependency treegraph
 
Reported: 2013-06-10 07:22 PDT by Mihai Balan
Modified: 2014-04-29 07:55 PDT (History)
11 users (show)

See Also:


Attachments
Patch (45.72 KB, patch)
2014-04-28 11:09 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch for landing (46.98 KB, patch)
2014-04-29 05:56 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch for landing (45.96 KB, patch)
2014-04-29 06:17 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (580.10 KB, application/zip)
2014-04-29 07:07 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 4 Andrei Bucur 2014-04-28 11:11:37 PDT
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.
Comment 5 Dave Hyatt 2014-04-28 11:16:51 PDT
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.
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:
fast/multicol/newmulticol/client-rects.html
fast/multicol/client-rects-spanners.html
fast/multicol/client-rects-spanners-complex.html
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.