Bug 122826 - Region based multicol: unresolvable percent height results in 1px tall multicol
Summary: Region based multicol: unresolvable percent height results in 1px tall multicol
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Morten Stenshorne
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-15 04:08 PDT by Morten Stenshorne
Modified: 2014-01-23 23:56 PST (History)
7 users (show)

See Also:


Attachments
Test case (416 bytes, text/html)
2013-10-15 04:08 PDT, Morten Stenshorne
no flags Details
Patch (10.41 KB, patch)
2013-10-15 05:54 PDT, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (12.34 KB, patch)
2013-10-15 09:02 PDT, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2014-01-12 04:22 PST, Morten Stenshorne
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2014-01-21 01:53 PST, Morten Stenshorne
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Morten Stenshorne 2013-10-15 04:08:28 PDT
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.
Comment 1 Morten Stenshorne 2013-10-15 05:54:49 PDT
Created attachment 214253 [details]
Patch
Comment 2 Morten Stenshorne 2013-10-15 05:55:15 PDT
@dhyatt: can you review this, please?
Comment 3 Andrei Bucur 2013-10-15 06:17:18 PDT
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()?
Comment 4 Morten Stenshorne 2013-10-15 06:33:49 PDT
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.
Comment 5 Morten Stenshorne 2013-10-15 06:41:57 PDT
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.
Comment 6 Andrei Bucur 2013-10-15 07:09:11 PDT
(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 WebkitBugTracker@adobe.com
Comment 7 Andrei Bucur 2013-10-15 07:11:37 PDT
(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 WebkitBugTracker@adobe.com

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.
Comment 8 Morten Stenshorne 2013-10-15 08:55:59 PDT
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 WebkitBugTracker@adobe.com
> 
> 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.
Comment 9 Morten Stenshorne 2013-10-15 09:02:33 PDT
Created attachment 214267 [details]
Patch
Comment 10 Morten Stenshorne 2014-01-12 04:22:08 PST
Created attachment 220964 [details]
Patch
Comment 11 Dave Hyatt 2014-01-17 11:42:10 PST
Comment on attachment 220964 [details]
Patch

r=me
Comment 12 Morten Stenshorne 2014-01-21 01:53:49 PST
Created attachment 221728 [details]
Patch
Comment 13 Morten Stenshorne 2014-01-21 01:56:18 PST
Resolved conflicts caused by bug 123993.
Comment 14 Dave Hyatt 2014-01-23 17:13:04 PST
Comment on attachment 221728 [details]
Patch

r=me
Comment 15 WebKit Commit Bot 2014-01-23 23:56:20 PST
Comment on attachment 221728 [details]
Patch

Clearing flags on attachment: 221728

Committed r162694: <http://trac.webkit.org/changeset/162694>
Comment 16 WebKit Commit Bot 2014-01-23 23:56:23 PST
All reviewed patches have been landed.  Closing bug.