Bug 106143 – min-width/max-width of min-content/max-content don't work correctly if the width set

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

[Ojan Vafai]

Created attachment 181400 [details]
test case

See test case. Mozilla nightly gets this right. This is the same root cause as bug 97747.

I think we might need to add a concept of minContent/maxContent in addition to min/maxPreferredLogicalWidth. The former is not be affected by the width being set, the latter is.

[Ojan Vafai]

Created attachment 181529 [details]
screenshot of different browsers

Browsers disagree about how to render this.

[Ojan Vafai]

That was IE9, FF17 and Chrome at WebKit r136968.

[Ojan Vafai]

Created attachment 181530 [details]
better test case

[Ojan Vafai]

Thread on www-style: http://lists.w3.org/Archives/Public/www-style/2013Jan/0046.html

[Ojan Vafai]

Created attachment 184072 [details]
another test case

fixed width applies when done to a flexbox that is a flex-item. Should be the same root cause.

[Ojan Vafai]

*** Bug 108134 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

Created attachment 193970 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193970 [details]
Patch

Attachment 193970 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17230223

New failing tests:
css3/flexbox/box-sizing.html

[Build Bot]

Comment on attachment 193970 [details]
Patch

Attachment 193970 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17137524

New failing tests:
css3/flexbox/box-sizing.html

[Ojan Vafai]

Created attachment 193975 [details]
Patch

[Elliott Sprehn]

Comment on attachment 193975 [details]
Patch

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

What's the benefit of passing all these values around as ref params?

> Source/WebCore/rendering/RenderBlock.cpp:5750
> +void RenderBlock::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const

It's pretty weird to have this method be "const" but take ref params to instance members so it's not const at all :/

> Source/WebCore/rendering/RenderBlock.cpp:5768
> +        Length w = toRenderTableCell(this)->styleOrColLogicalWidth();

Would be nice to ditch the single letter variables.

> Source/WebCore/rendering/RenderBlock.h:904
> +    void computeBlockPreferredLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const;

Block is const but inline isnt?

[Ojan Vafai]

(In reply to comment #11)
> (From update of attachment 193975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193975&action=review
> 
> What's the benefit of passing all these values around as ref params?

Tried to explain this in the ChangeLog. computeInstrinsicLogicalWidths needs to pass these values in because it's not always setting the preferred width member variable.

> > Source/WebCore/rendering/RenderBlock.cpp:5750
> > +void RenderBlock::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
> 
> It's pretty weird to have this method be "const" but take ref params to instance members so it's not const at all :/

The method is const. Sometimes you pass in member variables as the out params and sometimes you pass stack variables.

> > Source/WebCore/rendering/RenderBlock.cpp:5768
> > +        Length w = toRenderTableCell(this)->styleOrColLogicalWidth();
> 
> Would be nice to ditch the single letter variables.

I was just moving the code, but I may as well fix the name while I'm changing the code.

> > Source/WebCore/rendering/RenderBlock.h:904
> > +    void computeBlockPreferredLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const;
> 
> Block is const but inline isnt?

Yeah, I started down the rabbit-hole of trying to make computeInlinePreferredLogicalWidths const and it was a tangled web that I didn't want to dive into right now. For now, I have a FIXME in the calling location. I suppose the FIXME should be here as well.

[Tony Chang]

Comment on attachment 193975 [details]
Patch

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

This looks OK to me, but I would address Elliott's feedback as well (single letter variable and FIXME in RenderBlock.h.

> LayoutTests/ChangeLog:15
> +        * platform/chromium-win/fast/table/overflowHidden-expected.txt:
> +        The new result correctly adds the scrollbar width to the table cell intrinsic width.

Why is there no updated pixel result for this?

> LayoutTests/ChangeLog:25
> +        * scrollbars/custom-scrollbar-table-cell-expected.png:
> +        The new result correctly adds the scrollbar width to the table cell intrinsic width.

Is this test still testing what it's supposed to be testing?  Certainly the comment in the test about there being 2 grey scrollbars with a black thumb needs to be updated.

[Ojan Vafai]

Comment on attachment 193975 [details]
Patch

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

>> LayoutTests/ChangeLog:15
>> +        The new result correctly adds the scrollbar width to the table cell intrinsic width.
> 
> Why is there no updated pixel result for this?

The elements that changed are below the fold, so they don't show up in the pixel dump.

>> LayoutTests/ChangeLog:25
>> +        The new result correctly adds the scrollbar width to the table cell intrinsic width.
> 
> Is this test still testing what it's supposed to be testing?  Certainly the comment in the test about there being 2 grey scrollbars with a black thumb needs to be updated.

Good catch. I'm pretty sure the new result is correct and the comment is wrong. Updated the comment.

[Ojan Vafai]

Created attachment 195777 [details]
Patch

[Tony Chang]

Comment on attachment 195777 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        min-width/max-width of min-content/max-content don't work correctly if the width set

"if the width set" is awkward.  Maybe "if width is specified"

> Source/WebCore/rendering/RenderBlock.cpp:5756
> +        // FIXME: Remove this const_casts.

Nit: const_casts -> const_cast

> LayoutTests/ChangeLog:15
> +        * platform/chromium-win/fast/table/overflowHidden-expected.txt:
> +        The new result correctly adds the scrollbar width to the table cell intrinsic width.

I would make a note why the pixel result isn't updated.

[Ojan Vafai]

Committed r147245: <http://trac.webkit.org/changeset/147245>

[Benjamin Poulain]

Committed r147488: <http://trac.webkit.org/changeset/147488>