WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
98633
max-height property not respected in case of tables
https://bugs.webkit.org/show_bug.cgi?id=98633
Summary
max-height property not respected in case of tables
Pravin D
Reported
2012-10-08 00:31:02 PDT
The max-height property does not override the height 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
Attachments
TestCase
(1.73 KB, text/html)
2012-10-08 00:31 PDT
,
Pravin D
no flags
Details
Proposed Patch
(20.45 KB, patch)
2012-10-16 14:41 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Proposed patch
(13.69 KB, patch)
2012-10-24 07:02 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(11.37 KB, patch)
2012-10-29 10:01 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(13.18 KB, patch)
2012-11-08 11:28 PST
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(13.48 KB, patch)
2012-11-12 23:02 PST
,
Pravin D
no flags
Details
Formatted Diff
Diff
TestCase with content height > max-height
(1.78 KB, text/html)
2012-11-20 00:29 PST
,
Pravin D
no flags
Details
Patch
(13.55 KB, patch)
2012-11-20 03:19 PST
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(14.45 KB, patch)
2012-11-23 09:53 PST
,
Pravin D
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Pravin D
Comment 1
2012-10-08 00:31:39 PDT
Created
attachment 167515
[details]
TestCase
Pravin D
Comment 2
2012-10-16 14:41:58 PDT
Created
attachment 169033
[details]
Proposed Patch
Build Bot
Comment 3
2012-10-16 17:46:53 PDT
Comment on
attachment 169033
[details]
Proposed Patch
Attachment 169033
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/14389368
New failing tests: fast/replaced/table-percent-height-text-controls.html editing/pasteboard/5780697-2.html http/tests/misc/acid2-pixel.html http/tests/misc/acid2.html fast/css/acid2-pixel.html fast/replaced/table-percent-height.html fast/dynamic/subtree-boundary-percent-height.html css3/flexbox/percentage-sizes.html fast/css/acid2.html
WebKit Review Bot
Comment 4
2012-10-17 02:57:29 PDT
Comment on
attachment 169033
[details]
Proposed Patch
Attachment 169033
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/14387506
New failing tests: editing/pasteboard/5780697-2.html http/tests/misc/acid2-pixel.html http/tests/misc/acid2.html fast/css/acid2-pixel.html fast/replaced/table-percent-height.html fast/dynamic/subtree-boundary-percent-height.html css3/flexbox/percentage-sizes.html fast/css/acid2.html
Pravin D
Comment 5
2012-10-24 07:02:58 PDT
Created
attachment 170387
[details]
Proposed patch
Pravin D
Comment 6
2012-10-24 09:03:13 PDT
Did some testing on the behavior of various browsers with regards to height distribution when max-height,min-height < computed content height. FF and IE do not respect max-height when the content height is greater than the max-height(for tables and css tables). The fix is that we simply take max-height, min-height in account in RenderTable::layout() while computing the height of the table. distributeExtraLogicalHeight() (already present) will take care of distributing extra positive height. If the extra height is negative, it simply ignores it, as other browsers do. Attachment
https://bugs.webkit.org/attachment.cgi?id=170387
does the above. Attachment
https://bugs.webkit.org/attachment.cgi?id=169033
was actually with a little efficient version of
attachment 170387
[details]
in terms of code reuse and container height calculation. However it's a little bug at the moment. @Julin, @Tony Would like ur opinions both the patches...
Tony Chang
Comment 7
2012-10-24 14:19:06 PDT
Comment on
attachment 170387
[details]
Proposed patch The code in 170387 is more what I expect. I'm not sure what 169033 is trying to do. View in context:
https://bugs.webkit.org/attachment.cgi?id=170387&action=review
> Source/WebCore/rendering/RenderTable.cpp:444 > + // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow. > + if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX)
We should cache this expression in a bool and just write the comment once.
> Source/WebCore/rendering/RenderTable.cpp:451 > +
Nit: Extra blank line.
> Source/WebCore/rendering/RenderTable.cpp:468 > +
Nit: Extra blank line.
Julien Chaffraix
Comment 8
2012-10-26 07:25:52 PDT
Comment on
attachment 170387
[details]
Proposed patch View in context:
https://bugs.webkit.org/attachment.cgi?id=170387&action=review
> Source/WebCore/rendering/RenderTable.cpp:463 > + if (logicalMinHeightLength.isFixed()) { > + // HTML tables size as though CSS height includes border/padding, CSS tables do not. > + LayoutUnit borders = ZERO_LAYOUT_UNIT; > + // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow. > + if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX) > + borders = borderAndPaddingBefore + borderAndPaddingAfter; > + computedMinLogicalHeight = logicalMinHeightLength.value() - borders;
I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see
bug 23658
) but that's more a nit.
> LayoutTests/fast/table/css-table-max-height.html:96 > +shouldBe("maxGreatThanMinHeightAutoLayout.getBoundingClientRect().height","202");
You should consider removing the padding on your cell and the border-spacing on your table so that this is 200px!
> LayoutTests/fast/table/css-table-max-height.html:108 > +onlyMaxHeightFixedLayout = document.getElementById("onlyMaxHeightFixedLayout"); > +shouldBe("onlyMaxHeightFixedLayout.getBoundingClientRect().height","202");
This looks like it's failing: max-height is 200px not 202px.
> LayoutTests/fast/table/css-table-max-height.html:112 > +document.body.removeChild(document.getElementById('container'));
This could be a check-layout.js test.
Pravin D
Comment 9
2012-10-29 10:01:50 PDT
Created
attachment 171270
[details]
Patch
Pravin D
Comment 10
2012-10-29 10:38:50 PDT
(In reply to
comment #8
)
> (From update of
attachment 170387
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=170387&action=review
> > > Source/WebCore/rendering/RenderTable.cpp:463 > > + if (logicalMinHeightLength.isFixed()) { > > + // HTML tables size as though CSS height includes border/padding, CSS tables do not. > > + LayoutUnit borders = ZERO_LAYOUT_UNIT; > > + // FIXME: We cannot apply box-sizing: content-box on <table> which other browsers allow. > > + if ((node() && node()->hasTagName(tableTag)) || style()->boxSizing() == BORDER_BOX) > > + borders = borderAndPaddingBefore + borderAndPaddingAfter; > > + computedMinLogicalHeight = logicalMinHeightLength.value() - borders; >
Have moved the logic to a helper function
> I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see
bug 23658
) but that's more a nit. >
I was not sure about this. In tables the padding is ignored if border collapsing set. adjustBorderBoxLogicalHeightForBoxSizing() uses border + padding to adjust the logical height. So using adjustBorderBoxLogicalHeightForBoxSizing() means we have to re-adjust the border values when border collaspse is set(border-collapse:collapse). So currently I have not used adjustBorderBoxLogicalHeightForBoxSizing() to calculate the border values. Had a doubt(Off-topic) adjustBorderBoxLogicalHeightForBoxSizing() add/subtract border+padding if the style()->boxSizing() == CONTENT_BOX and adjustContentBoxLogicalHeightForBoxSizing() add/subtract border+padding if the style()->boxSizing() == BORDER_BOX Function name and logic inside the function seem to different. Is it intentional ? (I don not hv much idea about box sizing :( ).
> > LayoutTests/fast/table/css-table-max-height.html:96 > > +shouldBe("maxGreatThanMinHeightAutoLayout.getBoundingClientRect().height","202"); > > You should consider removing the padding on your cell and the border-spacing on your table so that this is 200px! > > > LayoutTests/fast/table/css-table-max-height.html:108 > > +onlyMaxHeightFixedLayout = document.getElementById("onlyMaxHeightFixedLayout"); > > +shouldBe("onlyMaxHeightFixedLayout.getBoundingClientRect().height","202"); > > This looks like it's failing: max-height is 200px not 202px. >
Done...
> > LayoutTests/fast/table/css-table-max-height.html:112 > > +document.body.removeChild(document.getElementById('container')); > > This could be a check-layout.js test. >
Done. However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.
Julien Chaffraix
Comment 11
2012-10-30 06:46:54 PDT
> > I really think this code should be some helper function as it is not be the only place where we have to do that (copying and pasting some code is bad!). We should also have a call to RenderBox::adjustBorderBoxLogicalHeightForBoxSizing for the CSS case as it would give us support for other box-sizing value for free (e.g. padding-box - see
bug 23658
) but that's more a nit. > > > > I was not sure about this. > In tables the padding is ignored if border collapsing set. adjustBorderBoxLogicalHeightForBoxSizing() uses border + padding to adjust the logical height. So using adjustBorderBoxLogicalHeightForBoxSizing() means we have to re-adjust the border values when border collaspse is set(border-collapse:collapse). > So currently I have not used adjustBorderBoxLogicalHeightForBoxSizing() to calculate the border values.
Good point, we would need some solution to handle collapsing borders unfortunately. You can ignore this comment (which was a nit and orthogonal to your change).
> Had a doubt(Off-topic) > adjustBorderBoxLogicalHeightForBoxSizing() add/subtract border+padding if the > style()->boxSizing() == CONTENT_BOX and > > adjustContentBoxLogicalHeightForBoxSizing() add/subtract border+padding if the > style()->boxSizing() == BORDER_BOX > > Function name and logic inside the function seem to different. Is it intentional ? (I don not hv much idea about box sizing :( ).
Those 2 functions work as expected. The passed in logical height is supposed to be from the style and the functions checks box-sizing to adjust it accordingly. Check CSS 2.1 box model and the 'box-sizing' property if you are not convinced.
> However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be.
You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background.
Julien Chaffraix
Comment 12
2012-10-30 08:47:08 PDT
Comment on
attachment 171270
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=171270&action=review
The test matches FF behavior (Opera has the same bug we have), haven't been able to check against IE. Another round will be needed for the viewport relative inconsistency.
> Source/WebCore/rendering/RenderTable.cpp:329 > +LayoutUnit RenderTable::convertStyleLogicalHeightToComputedHeight(const Length& styleLogicalHeight, bool collapseBorders)
I wouldn't pass collapseBorders in the new method. Querying the value from the style should be cheap enough.
> Source/WebCore/rendering/RenderTable.cpp:467 > + if (logicalMaxHeightLength.isSpecified() && !logicalMaxHeightLength.isNegative()) {
This will not work if max-height or min-height are view-port relative. Actually it will badly break: the code will compute the 2 values to be 0. Using isSpecified() was a good idea as it is consistent with the existing logical width code but this means you have to fix convertStyleLogicalHeightToComputedHeight to properly handle view-port relative units. I don't mind either way but this will need to be tested or tracked.
> LayoutTests/fast/table/css-table-max-height-expected.txt:4 > +Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. > +PASS
This placeholder text isn't needed as you force the table logical height and could be dropped. If you really think it's needed, I would put some explanations instead of a placeholder as it would make the output more readable. For example: This sub-test checks that max-height is applied to a table. PASS
> LayoutTests/fast/table/css-table-max-height.html:9 > + border:1px solid green;
Not sure the border adds much here as we are 100% height.
> LayoutTests/fast/table/css-table-max-height.html:70 > + <div iclass="child fixed-table" style="max-height:200px;" data-expected-height=200>
typo 'iclass', that's why you don't have a PASS in the output.
Tony Chang
Comment 13
2012-10-30 12:14:32 PDT
(In reply to
comment #11
)
> > However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be. > > You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background.
If a check fails, it dumps the details of the failure. For this test, I think either js-test-pre.js or check-layout.js would be OK. check-layout.js is probably most useful if you want to test lots of properties (width, height, offsets, etc for dozens of elements). Maybe we should add an optional second param to checkLayout that emits the test condition and value (as a separate patch).
Pravin D
Comment 14
2012-11-07 09:32:20 PST
(In reply to
comment #13
)
> (In reply to
comment #11
) > > > However IMHO using js-test-pre.js seemed to be informative than check-layout.js. Opening the testcase(js-test-pre.js) in a browser would tell a developer wat to expect. Using check-layout.js, one must see the source code to figure out the expected values should be. > > > > You need to read the source to understand the test at some point so it's not really an issue. However I do agree that it's unfortunate that check-layout.js doesn't dump what is tested nor what are the expected values. I don't know the decision behind that choice though so Tony may have some background. > > If a check fails, it dumps the details of the failure. For this test, I think either js-test-pre.js or check-layout.js would be OK. check-layout.js is probably most useful if you want to test lots of properties (width, height, offsets, etc for dozens of elements). >
Given a choice i'd prefer js-test-pre.js over check-layout.js(atleast in this case). However I'd leave the decision to either one of u(Julien or Tony).
> Maybe we should add an optional second param to checkLayout that emits the test condition and value (as a separate patch). >
There is a drawback in using check-layout.js over js-test-pre.js. Inequality comparisons are not possible when using checkLayout such as ">" or "<" as these have special meaning in html and will lead to bad consequences. For example the case in the test case using data-expected-height=144 Actually what I wanted was data-expected-height > 100 (showing that height of the table can be more than max-height value when the content height is greater). However this is not possible when using checkLayout
Pravin D
Comment 15
2012-11-08 11:28:34 PST
Created
attachment 173081
[details]
Patch
Pravin D
Comment 16
2012-11-08 21:42:34 PST
(In reply to
comment #12
)
> (From update of
attachment 171270
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=171270&action=review
>
> > Source/WebCore/rendering/RenderTable.cpp:329 > > +LayoutUnit RenderTable::convertStyleLogicalHeightToComputedHeight(const Length& styleLogicalHeight, bool collapseBorders) > > I wouldn't pass collapseBorders in the new method. Querying the value from the style should be cheap enough. >
Done.
> > Source/WebCore/rendering/RenderTable.cpp:467 > > + if (logicalMaxHeightLength.isSpecified() && !logicalMaxHeightLength.isNegative()) { > > This will not work if max-height or min-height are view-port relative. Actually it will badly break: the code will compute the 2 values to be 0. > > Using isSpecified() was a good idea as it is consistent with the existing logical width code but this means you have to fix convertStyleLogicalHeightToComputedHeight to properly handle view-port relative units. I don't mind either way but this will need to be tested or tracked. >
Added support for handling viewport percent height values. Also added a set of sub-tests to check the same.
> > LayoutTests/fast/table/css-table-max-height-expected.txt:4 > > +Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. > > +PASS > > This placeholder text isn't needed as you force the table logical height and could be dropped. > > If you really think it's needed, I would put some explanations instead of a placeholder as it would make the output more readable. For example: > > This sub-test checks that max-height is applied to a table. > PASS >
The height of the table is governed by it's content height in some sub tests. To keep the test case more informative and maintain consistency across sub tests have used short description text as placeholders.
> > LayoutTests/fast/table/css-table-max-height.html:9 > > + border:1px solid green; > > Not sure the border adds much here as we are 100% height. >
Done.
> > LayoutTests/fast/table/css-table-max-height.html:70 > > + <div iclass="child fixed-table" style="max-height:200px;" data-expected-height=200> > > typo 'iclass', that's why you don't have a PASS in the output. >
thanks for the catch. Done
Julien Chaffraix
Comment 17
2012-11-09 10:52:34 PST
Comment on
attachment 173081
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=173081&action=review
> Source/WebCore/rendering/RenderTable.cpp:345 > + computedLogicalHeight = minimumValueForLength(styleLogicalHeight, 0, view());
I would add this to your code to ensure that we didn't forget a case (like calculated which should hopefully fall back on one of the two above): else ASSERT_NOT_REACHED();
> LayoutTests/fast/table/css-table-max-height.html:65 > + <div class="child" style="max-height:100px;" data-expected-height=128> > + This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout. > + </div>
I don't really understand the final height. 'max-height' should override 'height' and the computed height should be 100px (not 128px). Also your wording is confusing as it's the max-height who should be applied.
Pravin D
Comment 18
2012-11-12 23:02:00 PST
Created
attachment 173828
[details]
Patch
Pravin D
Comment 19
2012-11-12 23:11:47 PST
(In reply to
comment #17
)
> (From update of
attachment 173081
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=173081&action=review
> > > Source/WebCore/rendering/RenderTable.cpp:345 > > + computedLogicalHeight = minimumValueForLength(styleLogicalHeight, 0, view()); > > I would add this to your code to ensure that we didn't forget a case (like calculated which should hopefully fall back on one of the two above): > > else > ASSERT_NOT_REACHED(); >
Done.
> > LayoutTests/fast/table/css-table-max-height.html:65 > > + <div class="child" style="max-height:100px;" data-expected-height=128> > > + This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout. > > + </div> > > I don't really understand the final height. 'max-height' should override 'height' and the computed height should be 100px (not 128px). > > Also your wording is confusing as it's the max-height who should be applied. >
Sorry for not explaining this case before. The height, max-height when used with tables have a slightly different behavior . The content height always gets the highest priority , i.e Priority wise -- Content Height > max-height > height. This is true across all other browsers(verified the case on FF). This is avoid the negative height distribution I guess. In the testcase we check that the above behavior is not affected by the code added to handle max-height, min-height.
Julien Chaffraix
Comment 20
2012-11-19 11:21:19 PST
Comment on
attachment 173828
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=173828&action=review
I don't need to see the updated change.
> Source/WebCore/ChangeLog:8 > + max-height property is does not overriding the height properties for css tables(display:table) > +
https://bugs.webkit.org/show_bug.cgi?id=98633
> + > + Reviewed by NOBODY (OOPS!). > + > + The max-height property determines the maximum computed height an element can have. In case of css tables(display:table)
Note that it was all tables and not just CSS tables that were impacted. Please update the bug title and this line to reflect that (fixing also the extra 'is' in the title)
> LayoutTests/fast/table/css-table-max-height.html:65 > + <div class="child" style="max-height:100px;" data-expected-height=128> > + This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout. > + </div>
This doesn't match any other browsers (even FF returns 100px here) and the div case so I don't agree with your analysis. As you pointed out this is a bug in our extra logical height spreading algorithm (see
bug 81824
for some context) so it is orthogonal to this change. My take would be to land with the real expected result (ie 100px and failing that) along with a comment in the test instead of accepting our broken behavior in this case.
> LayoutTests/fast/table/css-table-max-height.html:100 > +<div class="container"> > + <div class="child fixed-table" style="max-height:100px;" data-expected-height=128> > + This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout. > + </div>
Same comment here.
Pravin D
Comment 21
2012-11-20 00:29:38 PST
Created
attachment 175167
[details]
TestCase with content height > max-height In the testcase used in the previous patch when rendered on FF did not have enough content to satisfy the condition Content Height > max-height. This testcase can be used to check the behavior on FF and Webkit.
Pravin D
Comment 22
2012-11-20 03:19:16 PST
Created
attachment 175179
[details]
Patch
Pravin D
Comment 23
2012-11-20 06:43:08 PST
(In reply to
comment #20
)
> (From update of
attachment 173828
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=173828&action=review
> > I don't need to see the updated change. > > > Source/WebCore/ChangeLog:8 > > + max-height property is does not overriding the height properties for css tables(display:table) > > +
https://bugs.webkit.org/show_bug.cgi?id=98633
> > + > > + Reviewed by NOBODY (OOPS!). > > + > > + The max-height property determines the maximum computed height an element can have. In case of css tables(display:table) > > Note that it was all tables and not just CSS tables that were impacted. Please update the bug title and this line to reflect that (fixing also the extra 'is' in the title) >
Done.
> > LayoutTests/fast/table/css-table-max-height.html:65 > > + <div class="child" style="max-height:100px;" data-expected-height=128> > > + This sub-test checks that when content height is greater than max-height, content height is applied to the table with auto layout. > > + </div> > > This doesn't match any other browsers (even FF returns 100px here) and the div case so I don't agree with your analysis. As you pointed out this is a bug in our extra logical height spreading algorithm (see
bug 81824
for some context) so it is orthogonal to this change. > > My take would be to land with the real expected result (ie 100px and failing that) along with a comment in the test instead of accepting our broken behavior in this case. > > > LayoutTests/fast/table/css-table-max-height.html:100 > > +<div class="container"> > > + <div class="child fixed-table" style="max-height:100px;" data-expected-height=128> > > + This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout. > > + </div> > > Same comment here. >
Have made some changes to the testcase. The issue was that in the previous testcase the content height did not exceed the max-height on FF. Also the computed height when content height is different on FF, Chrome(or Safari) and DumpRenderTree. Also run-webkit-tests gives a different set of values for computed height in case when content height > max-height from the ones found when testcase is fetched using FF or Chrome. The expected test result is in accordance to values found in dumpRenderTree. Also check attachment
https://bugs.webkit.org/attachment.cgi?id=175167
for comparing the behavior of FF and Webkit based browser. In conclusion have changed the testcase such that all the sub tests pass(no expected failures) Please let me know if your opinion on the same. Maybe if you feel that the height of table should not exceed max-height in any case, I can upload another patch reflecting the same.
Julien Chaffraix
Comment 24
2012-11-22 15:47:55 PST
Comment on
attachment 175179
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=175179&action=review
> Please let me know if your opinion on the same. Maybe if you feel that the height of table should not exceed max-height in any case
OK, FF and IE match this behavior. Opera has a weird behavior.
> LayoutTests/fast/table/css-table-max-height.html:101 > + <div class="child fixed-table" style="max-height:100px;" data-expected-height=192> > + This sub-test checks that when content height is greater than max-height, content height is applied to a table with fixed layout. > + <br><br>FILLER TEXT TO INCREASE CONTENT HEIGHT.
Note that this makes the test dependent on the text size. Hopefully this shouldn't make the test platform-dependent.
WebKit Review Bot
Comment 25
2012-11-22 15:57:34 PST
Comment on
attachment 175179
[details]
Patch Clearing flags on attachment: 175179 Committed
r135549
: <
http://trac.webkit.org/changeset/135549
>
WebKit Review Bot
Comment 26
2012-11-22 15:57:39 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 27
2012-11-22 20:57:36 PST
It made zillions tests crash in debug mode on all bot.
Pravin D
Comment 28
2012-11-22 23:16:46 PST
(In reply to
comment #27
)
> It made zillions tests crash in debug mode on all bot. >
Have created a bug for the same. See
Bug 103100
WebKit Review Bot
Comment 29
2012-11-22 23:51:52 PST
Re-opened since this is blocked by
bug 103102
Pravin D
Comment 30
2012-11-23 09:53:21 PST
Created
attachment 175823
[details]
Patch
Pravin D
Comment 31
2012-11-23 10:14:29 PST
(In reply to
comment #30
)
> Created an attachment (id=175823) [details] > Patch >
The asserts caused by
r135549
where due the fact that the patch did not handle the case when logical height was not specified(default auto). Fixed the same. Also added a sub-test to check the same. Sorry for the trouble.
Julien Chaffraix
Comment 32
2012-11-27 11:33:43 PST
Comment on
attachment 175823
[details]
Patch r=me again, assuming no ASSERT triggering.
WebKit Review Bot
Comment 33
2012-11-27 11:53:32 PST
Comment on
attachment 175823
[details]
Patch Clearing flags on attachment: 175823 Committed
r135891
: <
http://trac.webkit.org/changeset/135891
>
WebKit Review Bot
Comment 34
2012-11-27 11:53:38 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug