Bug 106143

Created attachment 181529 [details] screenshot of different browsers Browsers disagree about how to render this.

That was IE9, FF17 and Chrome at WebKit r136968.

Created attachment 181530 [details] better test case

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

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.

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

Created attachment 193970 [details] Patch

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

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

Created attachment 193975 [details] Patch

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?

(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.

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.

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.

Created attachment 195777 [details] Patch

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.

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

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