Bug 122826 – Region based multicol: unresolvable percent height results in 1px tall multicol

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Morten Stenshorne]

Created attachment 214248 [details]
Test case

When a multicol container specifies a percentage height, but its containing block has auto height (and therefore the multicol's computed height should become auto as well, according to the spec), the multicol's height becomes 1px.

[Morten Stenshorne]

Created attachment 214253 [details]
Patch

[Morten Stenshorne]

@dhyatt: can you review this, please?

[Andrei Bucur]

Comment on attachment 214253 [details]
Patch

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

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
> +                m_maxColumnHeight = LayoutUnit::max();

There's a static getter on RenderFlowThread:
    // Used to estimate the maximum height of the flow thread.
    static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }

Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?

[Morten Stenshorne]

Comment on attachment 214253 [details]
Patch

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

>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
>> +                m_maxColumnHeight = LayoutUnit::max();
> 
> There's a static getter on RenderFlowThread:
>     // Used to estimate the maximum height of the flow thread.
>     static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }
> 
> Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?

Sure, I can do that. Then I'd better replace all the other instances of LayoutUnit()::max() in RenderMultiColumnSet.cpp as well, since they're all about "infinite" heights.

[Morten Stenshorne]

Comment on attachment 214253 [details]
Patch

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

>>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
>>> +                m_maxColumnHeight = LayoutUnit::max();
>> 
>> There's a static getter on RenderFlowThread:
>>     // Used to estimate the maximum height of the flow thread.
>>     static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }
>> 
>> Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?
> 
> Sure, I can do that. Then I'd better replace all the other instances of LayoutUnit()::max() in RenderMultiColumnSet.cpp as well, since they're all about "infinite" heights.

Wait a minute! RenderFlowThread::maxLogicalHeight() doesn't exist in Blink? I'd like to apply my patch there as well, so unless it's going to be added, I think I'd like to keep the LayoutUnit::max() stuff.

[Andrei Bucur]

(In reply to comment #5)
> (From update of attachment 214253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214253&action=review
> 
> >>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
> >>> +                m_maxColumnHeight = LayoutUnit::max();
> >> 
> >> There's a static getter on RenderFlowThread:
> >>     // Used to estimate the maximum height of the flow thread.
> >>     static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }
> >> 
> >> Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?
> > 
> > Sure, I can do that. Then I'd better replace all the other instances of LayoutUnit()::max() in RenderMultiColumnSet.cpp as well, since they're all about "infinite" heights.
> 
> Wait a minute! RenderFlowThread::maxLogicalHeight() doesn't exist in Blink? I'd like to apply my patch there as well, so unless it's going to be added, I think I'd like to keep the LayoutUnit::max() stuff.

The function was added a while back but it wasn't ported yet: http://trac.webkit.org/changeset/152572 . Basically Blink doesn't have the issue (sub-pixel layout is enabled) and I didn't prioritise the porting. It will be there soon.
CC-ing [e-mail]

[Andrei Bucur]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 214253 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214253&action=review
> > 
> > >>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
> > >>> +                m_maxColumnHeight = LayoutUnit::max();
> > >> 
> > >> There's a static getter on RenderFlowThread:
> > >>     // Used to estimate the maximum height of the flow thread.
> > >>     static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }
> > >> 
> > >> Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?
> > > 
> > > Sure, I can do that. Then I'd better replace all the other instances of LayoutUnit()::max() in RenderMultiColumnSet.cpp as well, since they're all about "infinite" heights.
> > 
> > Wait a minute! RenderFlowThread::maxLogicalHeight() doesn't exist in Blink? I'd like to apply my patch there as well, so unless it's going to be added, I think I'd like to keep the LayoutUnit::max() stuff.
> 
> The function was added a while back but it wasn't ported yet: http://trac.webkit.org/changeset/152572 . Basically Blink doesn't have the issue (sub-pixel layout is enabled) and I didn't prioritise the porting. It will be there soon.
> CC-ing [e-mail]

Also, you can add the function and I'll make sure to stick the rest of the code to it in Blink when I port the before mentioned patch.

[Morten Stenshorne]

Comment on attachment 214253 [details]
Patch

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

>>>>>> Source/WebCore/rendering/RenderMultiColumnSet.cpp:169
>>>>>> +                m_maxColumnHeight = LayoutUnit::max();
>>>>> 
>>>>> There's a static getter on RenderFlowThread:
>>>>>     // Used to estimate the maximum height of the flow thread.
>>>>>     static LayoutUnit maxLogicalHeight() { return LayoutUnit::max() / 2; }
>>>>> 
>>>>> Do you think it makes more sense to use maxLogicalHeight() instead of LayoutUnit()::max()?
>>>> 
>>>> Sure, I can do that. Then I'd better replace all the other instances of LayoutUnit()::max() in RenderMultiColumnSet.cpp as well, since they're all about "infinite" heights.
>>> 
>>> Wait a minute! RenderFlowThread::maxLogicalHeight() doesn't exist in Blink? I'd like to apply my patch there as well, so unless it's going to be added, I think I'd like to keep the LayoutUnit::max() stuff.
>> 
>> The function was added a while back but it wasn't ported yet: http://trac.webkit.org/changeset/152572 . Basically Blink doesn't have the issue (sub-pixel layout is enabled) and I didn't prioritise the porting. It will be there soon.
>> CC-ing [e-mail]
> 
> Also, you can add the function and I'll make sure to stick the rest of the code to it in Blink when I port the before mentioned patch.

OK, sounds good.

[Morten Stenshorne]

Created attachment 214267 [details]
Patch

[Morten Stenshorne]

Created attachment 220964 [details]
Patch

[Dave Hyatt]

Comment on attachment 220964 [details]
Patch

r=me

[Morten Stenshorne]

Created attachment 221728 [details]
Patch

[Morten Stenshorne]

Resolved conflicts caused by bug 123993.

[Dave Hyatt]

Comment on attachment 221728 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 221728 [details]
Patch

Clearing flags on attachment: 221728

Committed r162694: <http://trac.webkit.org/changeset/162694>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.