Created attachment 167185 [details] TestCase The max-width property does not override the width property in case of a container having display:table. Have attached a reduced test case for the same(from bug 25016). courtesy: jasneet@chromium.org
Created attachment 167187 [details] TestCase
Created attachment 167194 [details] Patch
Did you check to see what IE and Firefox do? In bug 2516, Julien said that this behavior is not specified, in which case, we should see what other browsers do and try to get this spec'ed.
(In reply to comment #3) > Did you check to see what IE and Firefox do? In bug 2516, Julien said that this behavior is not specified, in which case, we should see what other browsers do and try to get this spec'ed. > I did check with IE and FF. They respect max-width value. We will have similar behavior with this patch applied. Also we have algorithms(Auto table layout and Fixed table layout) in place to distribute width among the cells when either the width of the table is greater or lesser than the sum of widths of its columns. However there is no such algorithm(defined by CSS) for distributing height among rows of the table. I think Julien was addressing this issue in his Comment #11 on bug 25016. IMHO we should not have much of a problem in addressing the max-width case as most of the required stuff's in place.
Comment on attachment 167194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167194&action=review Julien may have additional comments. > Source/WebCore/rendering/RenderTable.cpp:266 > + if (styleMaxLogicalWidth.isSpecified() && styleMaxLogicalWidth.isPositive()) 0 should be a valid value for max-width. I think you want !styleMaxLogicalWidth.isNegative(). Maybe add a test case for this? > Source/WebCore/rendering/RenderTable.cpp:269 > + setLogicalWidth(max<int>(computedMinLogicalWidth, min(logicalWidth(), computedMaxLogicalWidth))); The mixing of max (template) and min (no-template) styles is weird, but maybe it's intentional to do the conversion to int at the last moment?
Created attachment 167598 [details] Patch
(In reply to comment #5) > (From update of attachment 167194 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167194&action=review > Have upload a new patch with following changes: > > Source/WebCore/rendering/RenderTable.cpp:266 > > + if (styleMaxLogicalWidth.isSpecified() && styleMaxLogicalWidth.isPositive()) > > 0 should be a valid value for max-width. I think you want !styleMaxLogicalWidth.isNegative(). Maybe add a test case for this? > Changed styleMaxLogicalWidth.isPositive() to !styleMaxLogicalWidth.isNegative(). Also added a new case for the max-width = 0 case. > > Source/WebCore/rendering/RenderTable.cpp:269 > > + setLogicalWidth(max<int>(computedMinLogicalWidth, min(logicalWidth(), computedMaxLogicalWidth))); > > The mixing of max (template) and min (no-template) styles is weird, but maybe it's intentional to do the conversion to int at the last moment? > As per the discussion with Tony(on IRC) removal of max(tempate) will be handled in a follow-up bug as it might affect sub-pixel related test cases.
Comment on attachment 167598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167598&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Nit: extra blank line.
Comment on attachment 167598 [details] Patch This is causing a bunch of tests to fail.
Created attachment 167633 [details] Patch
Comment on attachment 167633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review > Source/WebCore/rendering/RenderTable.cpp:259 > + LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth); I think this could just be logicalWidth(). Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing. Another way to write the code would be: if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth))); if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive()) setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth)));
(In reply to comment #11) > (From update of attachment 167633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review > > > Source/WebCore/rendering/RenderTable.cpp:259 > > + LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth); > > I think this could just be logicalWidth(). Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing. > Think so... > Another way to write the code would be: > if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) > setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth))); > > if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive()) > setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth))); > Seems like with this approach we can do away with the local variables and their initializations. Will upload another patch with above approach...
Comment on attachment 167633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review The change looks globally good. As a side note, we should probably think about adding a function similar to RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth that works on non-replaced elements. >>> Source/WebCore/rendering/RenderTable.cpp:259 >>> + LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth); >> >> I think this could just be logicalWidth(). Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing. >> >> Another way to write the code would be: >> if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) >> setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth))); >> >> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive()) >> setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth))); > > Think so... I agree with Tony, the variables are unneeded outside the if statements (I don't mind keeping them for clarity but they don't need to be at the function scope). Also variables should be defined them when you need them not before as you are doing now. > Source/WebCore/rendering/RenderTable.cpp:266 > + if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) Per CSS 2.1, "Negative values for 'min-width' and 'max-width' are illegal.". Both property behave the same with respect to negative values so I would rather that we write both if statements in the same way (0 for any of the value should be rare anyway). > LayoutTests/fast/table/css-table-max-width.html:52 > + </div> I would throw a table with fixed layout in your test as you fix both auto and fixed table layout.
Created attachment 167714 [details] Patch
(In reply to comment #13) > (From update of attachment 167633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167633&action=review > > The change looks globally good. > > As a side note, we should probably think about adding a function similar to > RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth that works on non-replaced elements. > I guess so... I'll raise a bug for the same... > >>> Source/WebCore/rendering/RenderTable.cpp:259 > >>> + LayoutUnit computedMaxLogicalWidth = max(logicalWidth(), availableLogicalWidth); > >> > >> I think this could just be logicalWidth(). Which is to say, if max-width is not specified, the min(logicalWidth(), computedMaxLogicalWidth) line does nothing. > >> > >> Another way to write the code would be: > >> if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) > >> setLogicalWidth(min<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMaxLogicalWidth, availableLogicalWidth))); > >> > >> if (styleMinLogicalWidth.isSpecified() && styleMinLogicalWidth.isPositive()) > >> setLogicalWidth(max<int>(logicalWidth(), convertStyleLogicalWidthToComputedWidth(styleMinLogicalWidth, availableLogicalWidth))); > > > > Think so... > > I agree with Tony, the variables are unneeded outside the if statements (I don't mind keeping them for clarity but they don't need to be at the function scope). > > Also variables should be defined them when you need them not before as you are doing now. > Have uploaded with these suggested changes. > > Source/WebCore/rendering/RenderTable.cpp:266 > > + if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) > > Per CSS 2.1, "Negative values for 'min-width' and 'max-width' are illegal.". Both property behave the same with respect to negative values so I would rather that we write both if statements in the same way (0 for any of the value should be rare anyway). > In the patch handled the case where max-width and min-width can have non-negative values. The reasoning is that as 0 is a rare value for the above properties if (styleMinLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) will not be called often for 0 case(not much of a perf hit) and if (styleMaxLogicalWidth.isSpecified() && !styleMaxLogicalWidth.isNegative()) will also take care of the case when max-width:0, potential bug which need not be addressed later. I think you meant otherwise(i.e zero value case can be safely ignored). If you want I can upload another patch with styleMaxLogicalWidth.isPositive() and styleMinLogicalWidth.isPositive() in place of non-negative check. > > LayoutTests/fast/table/css-table-max-width.html:52 > > + </div> > > I would throw a table with fixed layout in your test as you fix both auto and fixed table layout. > Done
Comment on attachment 167714 [details] Patch Clearing flags on attachment: 167714 Committed r130774: <http://trac.webkit.org/changeset/130774>
All reviewed patches have been landed. Closing bug.