Bug 117407

Summary: [CSS Regions] Fix getClientRects() for content nodes
Product: WebKit Reporter: Mihai Balan <mibalan>
Component: CSSAssignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
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    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 none

Mihai Balan
Reported 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
Attachments
Patch (45.72 KB, patch)
2014-04-28 11:09 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Patch for landing (46.98 KB, patch)
2014-04-29 05:56 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
Patch for landing (45.96 KB, patch)
2014-04-29 06:17 PDT, Andrei Bucur 		no flags
DetailsFormatted DiffDiff
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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mihai Balan
Comment 1 2013-06-11 08:42:33 PDT
Corresponding Blink issue: https://code.google.com/p/chromium/issues/detail?id=248554
Anton Obzhirov
Comment 2 2013-07-08 05:38:12 PDT
I am going to check this bug.
Andrei Bucur
Comment 3 2014-04-28 11:09:15 PDT
Created attachment 230311 [details] Patch
Andrei Bucur
Comment 4 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.
Dave Hyatt
Comment 5 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.
Andrei Bucur
Comment 6 2014-04-29 05:56:41 PDT
Created attachment 230370 [details] Patch for landing
Andrei Bucur
Comment 7 2014-04-29 06:17:09 PDT
Created attachment 230372 [details] Patch for landing
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Andrei Bucur
Comment 10 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.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2014-04-29 07:55:20 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.