Bug 118526 – [CSS Regions] Propagate overflow from the flow thread to the first and last region

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

The overflow of the content inside regions is not propagated to the regions. This creates two problems:
- the overflow is not scrollable
- the content inside the flow thread layer is not correctly painted/repainted/selectable

Use this bug to implement just propagation. Correct overflow computation for all the regions will follow in a different patch.

[Andrei Bucur]

Created attachment 206670 [details]
WIP

[Andrei Bucur]

Created attachment 207254 [details]
Patch

[Andrei Bucur]

Comment on attachment 207254 [details]
Patch

I need to rewrite the test descriptions. Will do before landing the patch if everything is OK.

[Dave Hyatt]

Comment on attachment 207254 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.cpp:137
> +// FIXME: This is wrong for horizontal overflow because the overflow rectangle doesn't take the region size into account.
> +LayoutRect RenderRegion::layoutOverflowRectForFlowThreadPortion(const LayoutRect& flowThreadPortionRect, bool isFirstPortion, bool isLastPortion) const

Is there no way to share more code with the other overflow function? There's a lot of similar logic here.

> Source/WebCore/rendering/RenderRegion.cpp:375
> +    if (style()->writingMode() != m_flowThread->style()->writingMode()) {
> +        if (m_flowThread->style()->writingMode() == RightToLeftWritingMode || style()->writingMode() == RightToLeftWritingMode)
> +            layoutRect.setX(width() - layoutRect.maxX());
> +        else if (m_flowThread->style()->writingMode() == BottomToTopWritingMode || style()->writingMode() == BottomToTopWritingMode)
> +            layoutRect.setY(height() - layoutRect.maxY());
> +    }

This does not seem right to me.

[Andrei Bucur]

Created attachment 207385 [details]
Patch

[Andrei Bucur]

Comment on attachment 207385 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.cpp:116
> +    // Only clip along the flow thread axis if we want the visual overflow.

I'll correct the comment before committing. It was supposed to say we are not interested about the outline size when computing the layout overflow because the outline is just visual.

[Dave Hyatt]

Comment on attachment 207385 [details]
Patch

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

Looks good. Just fix the new argument to be an enum and it's good to go.

> Source/WebCore/rendering/RenderRegion.h:148
> +    LayoutRect overflowRectForFlowThreadPortion(const LayoutRect& flowThreadPortionRect, bool isFirstPortion, bool isLastPortion, bool isVisual) const;

Use an enum for the new argument please. The general rule is to use enums if you're going to pass raw true/false at call sites so that the call sites aren't confusing.

[Mihnea Ovidenie]

Comment on attachment 207385 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2770
> +        // FIXME: Find a way to invalidate the knowToHaveNoOverflow flah on the InlineBoxes.

flah -> flag

> Source/WebCore/rendering/RenderFlowThread.cpp:62
> +    , m_layoutPhase(0)

0 -> LayoutPhaseNormal

> Source/WebCore/rendering/RenderFlowThread.h:162
> +    enum LayoutPhase {

It would be awesome if you can add a comment above clarifying what each phase is about. Something like the description for StyleDifference.

> Source/WebCore/rendering/RenderRegion.cpp:333
> +    if (!m_flowThread)

Is this check needed? Should this be an ASSERT instead?

[Andrei Bucur]

(In reply to comment #8)
> (From update of attachment 207385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207385&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2770
> > +        // FIXME: Find a way to invalidate the knowToHaveNoOverflow flah on the InlineBoxes.
> 
> flah -> flag
Will do.

> 
> > Source/WebCore/rendering/RenderFlowThread.cpp:62
> > +    , m_layoutPhase(0)
> 
> 0 -> LayoutPhaseNormal
Will do.

> 
> > Source/WebCore/rendering/RenderFlowThread.h:162
> > +    enum LayoutPhase {
> 
> It would be awesome if you can add a comment above clarifying what each phase is about. Something like the description for StyleDifference.
Will do.

> 
> > Source/WebCore/rendering/RenderRegion.cpp:333
> > +    if (!m_flowThread)
> 
> Is this check needed? Should this be an ASSERT instead?
This is needed in case the region is invalid and has no flow thread attached. I can replace it with !isValid().

[Andrei Bucur]

Created attachment 208257 [details]
Patch

[Mihai Maerean]

Comment on attachment 208257 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.h:164
> +        LayoutPhaseNormal = 0, // The initial phase, used to measure content for the auto-height regions.

Calling this LayoutPhaseMeasureContent or even LayoutPhaseMeasureContentForAutoHeightRegions makes the code readable:
an assert like ASSERT( inNormalLayoutPhase() && hasAutoLogicalHeightRegions() ) in RenderFlowThread::updateRegionsFlowThreadPortionRect
will sound like this:
ASSERT( inMeasureContentForAutoHeightRegionsLayoutPhase() && hasAutoLogicalHeightRegions() )
It now makes sense why it's needed to test for hasAutoLogicalHeightRegions();

[Radu Stavila]

Comment on attachment 208257 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        The overflow phase is set after the flow threads are laid out and it marks the regions they

I think "and it marks the regions as needing to extract" would be more clear. I kept reading it as "(it marks the regions they need) to extract...".

[Andrei Bucur]

Created attachment 208273 [details]
Patch

[Dave Hyatt]

Comment on attachment 208273 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 208273 [details]
Patch

Clearing flags on attachment: 208273

Committed r153814: <http://trac.webkit.org/changeset/153814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.