Summary: | [CSS Regions] Fix getClientRects() for content nodes | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Balan <mibalan> | ||||||||||
Component: | CSS | Assignee: | 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
Mihai Balan
2013-06-10 07:22:54 PDT
Corresponding Blink issue: https://code.google.com/p/chromium/issues/detail?id=248554 I am going to check this bug. Created attachment 230311 [details]
Patch
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 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. Created attachment 230370 [details]
Patch for landing
Created attachment 230372 [details]
Patch for landing
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 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
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 on attachment 230370 [details] Patch for landing Clearing flags on attachment: 230370 Committed r167930: <http://trac.webkit.org/changeset/167930> All reviewed patches have been landed. Closing bug. |