Bug 78493 – [CSSRegions][CSSOM] Implement Element.getRegionFlowRanges

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Raul Hudea]

Implement the Element.getRegionsFlowRanges extension from http://dev.w3.org/csswg/css3-regions/

[Raul Hudea]

Created attachment 156424 [details]
Current work (still lots of fprintf's)

This is the current status for this:
- the patch is still in its infancy (no tests in this one)
- the idea behind is to do walk the RenderFlowThread in order to find the RenderObjects that starts and ends this region (it uses an identical AnchorType as the Position, in order to be able have things like BeforeAnchor, BeforeChildren, InAnchor (for text)..)
- after this RenderObject boundary is found, then it tries to map it over the DOM tree and create extra Ranges when a hole is found (for example a node inside a content node is also a content node => those nodes are siblings in the RenderTree).

[Raul Hudea]

Created attachment 161495 [details]
patch

[Dave Hyatt]

Comment on attachment 161495 [details]
patch

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().

[Raul Hudea]

Created attachment 162493 [details]
Address reviewer comments

[Raul Hudea]

Created attachment 162499 [details]
Removed duplicate changelog entry (from the patch above)

[Early Warning System Bot]

Comment on attachment 162499 [details]
Removed duplicate changelog entry (from the patch above)

Attachment 162499 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13761983

[Raul Hudea]

Created attachment 162511 [details]
Make the qt bot happy

[Dave Hyatt]

Comment on attachment 162511 [details]
Make the qt bot happy

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

Looks good. Just minor nits. I would possibly think about that writing mode section, since it feels like that could be simplified to me, but if you fix the other issues I pointed out that's good enough for a +.

> Source/WebCore/rendering/RenderBlock.cpp:7220
> +        if (containerBlock->style()->writingMode() != currentBlock->style()->writingMode()) {

You can use the method isWritingModeRoot() here. That's cleaner than having to compare the writing mode styles.

> Source/WebCore/rendering/RenderBlock.cpp:7231
> +            if (containerBlock->style()->isFlippedBlocksWritingMode()) {
> +                if (containerBlock->isHorizontalWritingMode())
> +                    blockRect.setY(currentBlock->height() - blockRect.maxY());
> +                else
> +                    blockRect.setX(currentBlock->width() - blockRect.maxX());
> +            }
> +            currentBlock->flipForWritingMode(blockRect);
> +        }
> +        blockRect.moveBy(currentBlockLocation);

I think this is correct, but it feels kind of convoluted to me. I think it could be simplified, but I won't hold up review for it.

> Source/WebCore/rendering/RenderBlock.cpp:7234
> +    return currentBlock->isHorizontalWritingMode() ? blockRect.y().toInt() : blockRect.x().toInt();

You should not be returning toInt() here, should you? Your function is returning a LayoutUnit, which means you don't need to be converting to int.

[Adam Barth]

Comment on attachment 162511 [details]
Make the qt bot happy

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

> Source/WebCore/dom/Element.idl:147
>          // CSS Regions API
>          readonly attribute DOMString webkitRegionOverset;
> +        sequence<Range> webkitGetRegionFlowRanges();

Is there no ENABLE macro for regions?  These DOM APIs shouldn't be enabled by default.

[Raul Hudea]

Created attachment 163176 [details]
Remove the copy-pasted toInt()

- As discussed on IRC, the isWritingModeRoot is using the parent style writing mode, while here is needed the container's one
- For now, I would like to leave the writing mode code as is. If there were a function like flipForWritingModeForBlock(RenderBlock*, LayoutRect&) the code would be just:

   container->flipForWritingModeForBlock(currentBlock, rect);
   currentBlock->flipForWritingMode(rect);

  In the worst case, you need to do two flips.

[Raul Hudea]

Created attachment 163179 [details]
Addresses Adam's comment about visibility on IDL

[Raul Hudea]

(In reply to comment #9)
> (From update of attachment 162511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162511&action=review
> 
> > Source/WebCore/dom/Element.idl:147
> >          // CSS Regions API
> >          readonly attribute DOMString webkitRegionOverset;
> > +        sequence<Range> webkitGetRegionFlowRanges();
> 
> Is there no ENABLE macro for regions?  These DOM APIs shouldn't be enabled by default.

I made the webkitGetRegionFlowRanges not visible if CSS_REGIONS is not enable, but for the others I've added a new bug to make sure every Regions API is validated: https://bugs.webkit.org/show_bug.cgi?id=96300

Thanks for catching that.

[Adam Barth]

Comment on attachment 163179 [details]
Addresses Adam's comment about visibility on IDL

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

> Source/WebCore/dom/Element.idl:147
> +        [Conditional=CSS_REGIONS] sequence<Range> webkitGetRegionFlowRanges();

Thanks!

[Dave Hyatt]

Comment on attachment 163179 [details]
Addresses Adam's comment about visibility on IDL

r=me

[WebKit Review Bot]

Comment on attachment 163179 [details]
Addresses Adam's comment about visibility on IDL

Rejecting attachment 163179 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ile Source/WebCore/rendering/RenderNamedFlowThread.h
patching file Source/WebCore/rendering/RenderRegion.cpp
Hunk #3 succeeded at 542 with fuzz 1 (offset 40 lines).
patching file Source/WebCore/rendering/RenderRegion.h
Hunk #2 FAILED at 98.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderRegion.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Hyatt']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13825755

[Raul Hudea]

Created attachment 163777 [details]
Rebased patch

[Raul Hudea]

Created attachment 163785 [details]
Fix the reviewer name in rebased patch

[WebKit Review Bot]

Comment on attachment 163785 [details]
Fix the reviewer name in rebased patch

Rejecting attachment 163785 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rBox.cpp
patching file Source/WebCore/rendering/RenderNamedFlowThread.cpp
patching file Source/WebCore/rendering/RenderNamedFlowThread.h
patching file Source/WebCore/rendering/RenderRegion.cpp
Hunk #3 FAILED at 542.
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderRegion.cpp.rej
patching file Source/WebCore/rendering/RenderRegion.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13841222

[Raul Hudea]

Created attachment 163794 [details]
Some extraspace manages to enter somehow in the previous patch

[Mihnea Ovidenie]

Comment on attachment 163794 [details]
Some extraspace manages to enter somehow in the previous patch

One more round :)

[WebKit Review Bot]

Comment on attachment 163794 [details]
Some extraspace manages to enter somehow in the previous patch

Clearing flags on attachment: 163794

Committed r128416: <http://trac.webkit.org/changeset/128416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.