Bug 91097 - [CSSRegions]Auto height is not working for regions
Summary: [CSSRegions]Auto height is not working for regions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords:
Depends on: 96267 97533 99952 100749
Blocks: 66645
  Show dependency treegraph
 
Reported: 2012-07-12 08:26 PDT by Mihnea Ovidenie
Modified: 2021-04-02 00:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (76.61 KB, patch)
2012-08-02 07:09 PDT, Mihnea Ovidenie
jchaffraix: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2012-07-12 08:26:49 PDT
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.
Comment 1 Mihnea Ovidenie 2012-08-02 07:09:47 PDT
Created attachment 156074 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-02 07:13:03 PDT
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.
Comment 3 Julien Chaffraix 2012-08-09 09:51:23 PDT
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?
Comment 4 Julien Chaffraix 2012-08-09 10:15:11 PDT
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.
Comment 5 Dave Hyatt 2012-08-20 09:04:56 PDT
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.
Comment 6 Dave Hyatt 2012-08-21 09:20:49 PDT
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.
Comment 7 Mihnea Ovidenie 2012-11-05 01:40:28 PST
Support for auto-height regions has landed in trunk, i have added the relevant bugs as dependencies.