[CSS Regions] Implement region styling for font-size

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Andrei Bucur]

The CSS Regions spec mentions that font properties support region styling:
http://www.w3.org/TR/css3-regions/#the-at-region-style-rule

Add support for the font-size property.

[Andrei Bucur]

Created attachment 161694 [details]
Patch

[Andrei Bucur]

Created attachment 161696 [details]
A test file to play with

A test file to play with.

[Andrei Bucur]

Comment on attachment 161696 [details]
A test file to play with

Move the loupe with the mouse or the buttons.

[Dave Hyatt]

Comment on attachment 161694 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderBlock.cpp:6136
> +    updateRegionStyleForOffset(logicalHeight() + paginationStrut());

Add an if (inRenderFlowThread()) check to avoid the function call for normal layout.

> Source/WebCore/rendering/RenderBlock.cpp:7180
> -    return rootBox->paginatedLineWidth() != availableLogicalWidthForContent(rootBox->lineTopWithLeading() + lineDelta);
> +    return rootBox->paginatedLineWidth() != availableLogicalWidthForContent(rootBox->lineBottomWithLeading() + lineDelta);

This change is questionable. We use the top of lines for floats, and the intent here is to match that heuristic. Not sure why a change is necessary here?

> Source/WebCore/rendering/RenderBlock.cpp:7211
> +    // HACK to skip the regions with height 0.
> +    LayoutUnit pageLogicalHeight = pageLogicalHeightForOffset(blockOffset);
> +    if (!pageLogicalHeight)
> +        return;

Can't we just fix regionAtBlockOffset to never return a zero-height region?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1287
> +    // FIXME: styleToUse can be cached because no region stylable property is used below

I don't understand this FIXME.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1389
> +                    // If the block got shifted for pagination; we need to take the strut into consideration.
> +                    if (adjustment || oldPaginationStrut != newPaginationStrut) {

Not quite following this. The strut value changed but the adjustment is 0? When does that occur?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1395
> +                        if (availableLogicalWidthForLine(oldLogicalHeight + adjustment + newPaginationStrut, layoutState.lineInfo().isFirstLine()) != oldLineWidth) {

This seems confusing to me. Why wasn't the newPaginationStrut factored into the adjustment if you're actually adding it in here?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1681
> +                updateRegionStyleForOffset(curr->lineBottomWithLeading());
> +                if (m_lineHeight == -1) {
> +                    layoutState.markForFullLayout();
> +                    break;
> +                }

You're probably going to want updateRegionStyleForOffset to return a hint similar to RenderStyle::diff that helps you know what you have to do, e.g., a repaint, a relayout, etc. Just checking if line height cleared is way too specific to font-size, and you should be thinking more generally here.

I'd pick lineTopWithLeading rather than the bottom, unless you have a compelling reason why bottom should be used.

> Source/WebCore/rendering/RenderText.cpp:203
> +void RenderText::styleInRegionDidChange(const RenderStyle* oldStyle)
> +{
> +    RenderObject::styleInRegionDidChange(oldStyle);
> +    RenderBlock* containingBlock = this->containingBlock();
> +    ASSERT(containingBlock);
> +    // FIXME: Is this accurate? We want to invalidate the preferred width only at layout time.
> +    if (!containingBlock->needsLayout())
> +        return;
> +    if (oldStyle->fontSize() != style()->fontSize())
> +        setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
> +}
> +

I really want to avoid class-specific methods for everything like this. You need to try to implement something that leverages the style diff stuff we've already got to know if something is a repaint vs. a layout etc. and then do the right thing based off that hint.

[Andrei Bucur]

(In reply to comment #4)
> (From update of attachment 161694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161694&action=review
> 
> r-
> 
> > Source/WebCore/rendering/RenderBlock.cpp:6136
> > +    updateRegionStyleForOffset(logicalHeight() + paginationStrut());
> 
> Add an if (inRenderFlowThread()) check to avoid the function call for normal layout.
Will do.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:7180
> > -    return rootBox->paginatedLineWidth() != availableLogicalWidthForContent(rootBox->lineTopWithLeading() + lineDelta);
> > +    return rootBox->paginatedLineWidth() != availableLogicalWidthForContent(rootBox->lineBottomWithLeading() + lineDelta);
> 
> This change is questionable. We use the top of lines for floats, and the intent here is to match that heuristic. Not sure why a change is necessary here?
Because if we look at the top of the lines we don't catch the lines that fall in the middle of the region break (the top is in the good region, the bottom is in the bad region). Actually I think we should look both at the top of the line and at the bottom to be 100% certain a line crosses region boundaries.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:7211
> > +    // HACK to skip the regions with height 0.
> > +    LayoutUnit pageLogicalHeight = pageLogicalHeightForOffset(blockOffset);
> > +    if (!pageLogicalHeight)
> > +        return;
> 
> Can't we just fix regionAtBlockOffset to never return a zero-height region?
I wasn't sure it's ok to do it. If there's no problem, I'll change it.

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1287
> > +    // FIXME: styleToUse can be cached because no region stylable property is used below
> 
> I don't understand this FIXME.
Yes, bad FIXME. Sorry about that. It was informative only.

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1389
> > +                    // If the block got shifted for pagination; we need to take the strut into consideration.
> > +                    if (adjustment || oldPaginationStrut != newPaginationStrut) {
> 
> Not quite following this. The strut value changed but the adjustment is 0? When does that occur?
If the first line is the one that gets adjusted in adjustLinePositionForPagination, the whole block is shifted for pagination, leaving adjustment to 0. adjustment reflects the line shifts. paginationStrut reflects the block shifts.

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1395
> > +                        if (availableLogicalWidthForLine(oldLogicalHeight + adjustment + newPaginationStrut, layoutState.lineInfo().isFirstLine()) != oldLineWidth) {
> 
> This seems confusing to me. Why wasn't the newPaginationStrut factored into the adjustment if you're actually adding it in here?
I didn't want to touch the adjustLinePositionForPagination just yet. As I said before, adjustment is for shifting the lines. There's no point in doing that if the block is already shifted with pagination strut.
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1681
> > +                updateRegionStyleForOffset(curr->lineBottomWithLeading());
> > +                if (m_lineHeight == -1) {
> > +                    layoutState.markForFullLayout();
> > +                    break;
> > +                }
> 
> You're probably going to want updateRegionStyleForOffset to return a hint similar to RenderStyle::diff that helps you know what you have to do, e.g., a repaint, a relayout, etc. Just checking if line height cleared is way too specific to font-size, and you should be thinking more generally here.
I think I understand. My intent was to minimize the impact surface of this change, that is doing the full layout as less often as possible. Fine grained property invalidation seemed like a good option. A new type of Diff is necessary or using the old one is fine?
> 
> I'd pick lineTopWithLeading rather than the bottom, unless you have a compelling reason why bottom should be used.
Again, this is about catching lines that fall in between regions. If the previous line was in a region, it's known (I think? or floats can change that?) that the current line top is also in that region. So checking the bottom seemed like a good option. We can look at the top as well and invalidate starting from the previous line. Is this how you see it?
> 
> > Source/WebCore/rendering/RenderText.cpp:203
> > +void RenderText::styleInRegionDidChange(const RenderStyle* oldStyle)
> > +{
> > +    RenderObject::styleInRegionDidChange(oldStyle);
> > +    RenderBlock* containingBlock = this->containingBlock();
> > +    ASSERT(containingBlock);
> > +    // FIXME: Is this accurate? We want to invalidate the preferred width only at layout time.
> > +    if (!containingBlock->needsLayout())
> > +        return;
> > +    if (oldStyle->fontSize() != style()->fontSize())
> > +        setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
> > +}
> > +
> 
> I really want to avoid class-specific methods for everything like this. You need to try to implement something that leverages the style diff stuff we've already got to know if something is a repaint vs. a layout etc. and then do the right thing based off that hint.
You mean that a class specific styleDidChange is not appropriate or that it needs to know only about the Diff, not the old style?

[Brent Fulgham]

CSS Regions were removed in Bug 174978.