Bug 91097 – [CSSRegions]Auto height is not working for regions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

If a region does not have a specified height, it should compute its height based on: http://dev.w3.org/csswg/css3-regions/#regions-visual-formatting-steps. Currently, its height is computed to 0.

[Mihnea Ovidenie]

Created attachment 156074 [details]
Patch

[WebKit Review Bot]

Attachment 156074 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:11:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 156074 [details]
Patch

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

You really have to split this patch. 76k is not something anyone can review properly.

> LayoutTests/fast/regions/region-auto-height-01.html:7
> +            #red { width: 50px; height: 50px; background-color: red; }

maybe factoring the width / height into a div selector would make it more obvious what you are expecting.

> Source/WebCore/ChangeLog:35
> +               fast/regions/region-auto-height-18.html

I hate such naming for tests. Unless you have written the tests, the difference between 01 and 18 is unknown. When you have 2 - 3 tests, it's possible to look at them and find the difference but when you have 18, not so much.

> Source/WebCore/rendering/RenderRegion.h:104
> +            && style()->logicalTop().isSpecified()
> +            && style()->logicalBottom().isSpecified();

Shouldn't you also check that the logical left or right are specified too?

> Source/WebCore/rendering/RenderRegion.h:117
> +    LayoutUnit computedAutoHeight() const { return m_computedAutoHeight; }
> +    void setComputedAutoHeight(LayoutUnit computedAutoHeight)
> +    {
> +        m_computedAutoHeight = computedAutoHeight;
> +        m_hasComputedAutoHeight = true;
> +    }

Couldn't this reuse the overrideLogicalHeight logic instead of re-implementing it?

> Source/WebCore/rendering/RenderView.cpp:170
> +    if (inFirstLayoutPhaseAutoHeightRegions()) {

This is really a poor's man implementation of multi-phase layout. On top of duplicating the code, it removes a lot of the safety that you usually get from doing a full layout (see the ASSERT below for example). I am not supportive of such a hack.

If you want to do multiple-phase layout then you should patch FrameView::layout to actually call RenderView::layout several time as needed for regions. You can also implement a proper state machine in RenderView in this case.

Btw, how does this code deal with subtree layout?

> Source/WebCore/rendering/RenderView.cpp:175
> +        if (hasRenderNamedFlowThreads() && hasAutoHeightRegions())
> +            flowThreadController()->markAutoHeightRegionsForSecondLayoutPhase();

Globally calling setNeedsLayout during layout is a bad idea. Here you can probably get away with it but I would rather that this get moved outside layout.

> Source/WebCore/rendering/RenderView.h:191
> +    enum AutoHeightRegionsLayoutPhase { AutoHeightRegionsFirstLayoutPhase, AutoHeightRegionsSecondLayoutPhase };
> +    bool inFirstLayoutPhaseAutoHeightRegions() const { return m_autoHeightRegionsLayoutPhase == AutoHeightRegionsFirstLayoutPhase; }
> +    bool needsSecondPassLayoutForRegionsAutoHeight() const;

This is poorly named. first vs second layout don't give much information about *what* you are doing. The first layout is unconstrained vs the second one is properly breaking.

> Source/WebCore/rendering/RenderView.h:311
> +    AutoHeightRegionsLayoutPhase m_autoHeightRegionsLayoutPhase;
> +    unsigned m_autoHeightRegionsCount;

Shouldn't those be on the FlowThreadController and not on the RenderView?

[Julien Chaffraix]

Comment on attachment 156074 [details]
Patch

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

>> Source/WebCore/rendering/RenderView.cpp:170
>> +    if (inFirstLayoutPhaseAutoHeightRegions()) {
> 
> This is really a poor's man implementation of multi-phase layout. On top of duplicating the code, it removes a lot of the safety that you usually get from doing a full layout (see the ASSERT below for example). I am not supportive of such a hack.
> 
> If you want to do multiple-phase layout then you should patch FrameView::layout to actually call RenderView::layout several time as needed for regions. You can also implement a proper state machine in RenderView in this case.
> 
> Btw, how does this code deal with subtree layout?

Also a comment about repainting, this will likely cause spurious repaintings due to the LayoutRepainter logic assuming a single phase layout (see for example bug 92800). This is currently an open problem in WebKit but you should be aware of it.

[Dave Hyatt]

Comment on attachment 156074 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.h:102
> +        bool hasAnchoredTopAndBottom = (isOutOfFlowPositioned() || isRelPositioned())

You should remove isRelPositioned from this. You only should be checking isOutOfFlowPositioned, and as Julien says, in vertical writing mode left/right could be the ones giving you height.

[Dave Hyatt]

Comment on attachment 156074 [details]
Patch

I disagree with Julien that this needs to be outside the RenderObjects. This two-pass "auto height regions including breaks" algorithm does need to originate at renderers. Don't think of it as some global multi-pass layout algorithm. Note that pagination already has numerous multi-pass layout systems right in the renderers, so I don't think pulling this outside of the RenderObject tree is what we want to do, especially given it has to work originating at multi-column blocks for the new multi-column layout.

I think it's pretty much fine as is, although I do agree that the two phases should be more descriptive and elaborate on what they are actually doing.

[Mihnea Ovidenie]

Support for auto-height regions has landed in trunk, i have added the relevant bugs as dependencies.