[Webkit-unassigned] [Bug 78493] [CSSRegions][CSSOM] Implement Element.getRegionFlowRanges

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78493


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #161495|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Dave Hyatt <hyatt at apple.com>  2012-08-30 10:11:12 PST ---
(From update of attachment 161495)
View in context: https://bugs.webkit.org/attachment.cgi?id=161495&action=review

r-. Comments below.

> Source/WebCore/rendering/RenderBlock.cpp:7189
> +    // slower path without layout state
> +    LayoutUnit offsetTop;
> +    const RenderBlock* currBlock = this;

Can you put in a FIXME above this code that it doesn't work for columns or pages? Right now nothing works outside of layout, but with your change, only regions will work outside of layout. That's actually fine, since the new columns and pages are written using regions and will work with this new code you've put in, but I'd at least like a FIXME for now that it doesn't work with the current columns and pages code.

Something like;

// FIXME: This is a slower path that doesn't use layout state and relies on getting your logical top inside the enclosing flow thread. It doesn't
// work with columns or pages currently, but it should once they have been switched over to using flow threads.

Also, I'd like to see an ASSERT added to prevent anyone from calling this outside of layout for anything but regions. You can do this by adding:

ASSERT(currBlock->inRenderFlowThread());

before your loop. You could also just go ahead and bail with a return value of ZERO_LAYOUT_UNIT if currBlock isn't inside a render flow thread (following the assert).

> Source/WebCore/rendering/RenderBlock.cpp:7192
> +        offsetTop += currBlock->logicalTop();

This isn't correct. You have to account for changes in writing mode as you crawl up containing blocks. You're going to need to add isWritingModeRoot checks and do flipping as needed in order to get the correct offset.

You're going to need some additional tests here. Embed a few orthogonal writing mode objects inside your regions so that you are testing this, e.g., put a vertical-rl object inside a horizontal-tb flow thread etc. They don't paginate and just move atomically, but they should still work correctly as far as knowing what region they're in.

> Source/WebCore/rendering/RenderFlowThread.cpp:-792
> -

Random whitespace change. Please revert this.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:378
> +    LayoutRect regionRect(region->flowThreadPortionRect());

I would prefer you call the local flowThreadPortionRect and not regionRect.

Ultimately we're probably going to move to adopt CSS3 Fragmentation's terminology and start calling these fragmentRects, but for now let's keep using flowThreadPortion.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:387
> +        logicalTopForRegion = logicalTopForBox(isHorizontalWritingMode(), regionRect);

I think this would be a useful method to put on RenderRegion itself. I can see this being used by others, and I think there are even existing places in the code that grab this value that would benefit from such a helper.

Something like:

LayoutUnit RenderRegion::logicalTopForFlowThreadContent() const { return flowThread()->isHorizontalWritingMode() ? flowThreadPortionRect().y() : flowThreadPortionRect().x(); }

If you look around you'll see we do the above in other places in the code too.

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:393
> +        logicalBottomForRegion = logicalBottomForBox(isHorizontalWritingMode(), regionRect);

LayoutUnit RenderRegion::logicalBottomForFlowThreadContent() const { return flowThread()->isHorizontalWritingMode() ? flowThreadPortionRect().maxY() : flowThreadPortionRect().maxX(); }

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:439
> +            LayoutUnit logicalTopForRenderer = logicalTopForBox(isHorizontalWritingMode(), boundingBox);
> +            LayoutUnit logicalBottomForRenderer = logicalBottomForBox(isHorizontalWritingMode(), boundingBox);

RenderRegion::logicalTopOfFlowThreadContentRect(const LayoutRect& rect) const;
RenderRegion::logicalBottomOfFlowThreadContentRect(const LayoutRect& rect) const;

Could even implement the previous helpers I suggested and have them call these, passing in the flowThreadPortionRect().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.