Bug 118526 - [CSS Regions] Propagate overflow from the flow thread to the first and last region
Summary: [CSS Regions] Propagate overflow from the flow thread to the first and last r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrei Bucur
URL:
Keywords:
Depends on:
Blocks: 57312 116295 74737
  Show dependency treegraph
 
Reported: 2013-07-10 02:04 PDT by Andrei Bucur
Modified: 2013-08-08 00:44 PDT (History)
5 users (show)

See Also:


Attachments
WIP (508.09 KB, patch)
2013-07-15 09:37 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (141.93 KB, patch)
2013-07-22 10:16 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (59.26 KB, patch)
2013-07-24 02:24 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (59.92 KB, patch)
2013-08-07 05:37 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff
Patch (61.06 KB, patch)
2013-08-07 09:02 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Bucur 2013-07-10 02:04:35 PDT
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.
Comment 1 Andrei Bucur 2013-07-15 09:37:40 PDT
Created attachment 206670 [details]
WIP
Comment 2 Andrei Bucur 2013-07-22 10:16:22 PDT
Created attachment 207254 [details]
Patch
Comment 3 Andrei Bucur 2013-07-22 10:17:20 PDT
Comment on attachment 207254 [details]
Patch

I need to rewrite the test descriptions. Will do before landing the patch if everything is OK.
Comment 4 Dave Hyatt 2013-07-22 10:25:05 PDT
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.
Comment 5 Andrei Bucur 2013-07-24 02:24:35 PDT
Created attachment 207385 [details]
Patch
Comment 6 Andrei Bucur 2013-07-24 03:58:39 PDT
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.
Comment 7 Dave Hyatt 2013-08-06 11:50:25 PDT
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.
Comment 8 Mihnea Ovidenie 2013-08-06 23:14:28 PDT
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?
Comment 9 Andrei Bucur 2013-08-07 04:25:00 PDT
(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().
Comment 10 Andrei Bucur 2013-08-07 05:37:03 PDT
Created attachment 208257 [details]
Patch
Comment 11 Mihai Maerean 2013-08-07 06:32:50 PDT
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();
Comment 12 Radu Stavila 2013-08-07 06:46:45 PDT
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...".
Comment 13 Andrei Bucur 2013-08-07 09:02:41 PDT
Created attachment 208273 [details]
Patch
Comment 14 Dave Hyatt 2013-08-07 14:06:22 PDT
Comment on attachment 208273 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2013-08-07 23:26:53 PDT
Comment on attachment 208273 [details]
Patch

Clearing flags on attachment: 208273

Committed r153814: <http://trac.webkit.org/changeset/153814>
Comment 16 WebKit Commit Bot 2013-08-07 23:26:56 PDT
All reviewed patches have been landed.  Closing bug.