Bug 43319

Summary: CSS 2.1 failure: max-height doesn't work for elements within table cell
Product: WebKit Reporter: Nikita Vasilyev <me>
Component: CSSAssignee: Robert Hogan <robert>
Status: NEW    
Severity: Major CC: buildbot, centralmenu10, dglazkov, eric, info.thegreatsaver, jchaffraix, kyounga.ra, mikelawther, ojan.autocc, rniwa, robert, webkit.review.bot
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://elv1s.ru/files/html+css/max-height-within-table.html
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-02
none
Patch
none
Archive of layout-test-results from gce-cr-linux-06
none
Patch
none
Patch
none
Patch
none
Patch buildbot: commit-queue-

Nikita Vasilyev
Reported 2010-08-01 15:35:07 PDT
It does work in IE 7+ and Opera. It also doesn't work in Firefox. http://elv1s.ru/files/html+css/max-height-within-table.png
Attachments
Patch (48.64 KB, patch)
2012-08-09 13:35 PDT, Robert Hogan
no flags
Archive of layout-test-results from gce-cr-linux-02 (376.29 KB, application/zip)
2012-08-09 15:25 PDT, WebKit Review Bot
no flags
Patch (55.18 KB, patch)
2012-08-11 05:40 PDT, Robert Hogan
no flags
Archive of layout-test-results from gce-cr-linux-06 (370.71 KB, application/zip)
2012-08-11 06:17 PDT, WebKit Review Bot
no flags
Patch (55.19 KB, patch)
2012-08-11 07:30 PDT, Robert Hogan
no flags
Patch (56.98 KB, patch)
2012-12-22 03:34 PST, Robert Hogan
no flags
Patch (57.13 KB, patch)
2012-12-22 07:51 PST, Robert Hogan
no flags
Patch (57.26 KB, patch)
2013-02-09 05:14 PST, Robert Hogan
buildbot: commit-queue-
kyounga
Comment 1 2010-10-07 02:36:35 PDT
It works with FireFox 3.6.1. I think it should be fixed. According to CSS2.1 (http://www.w3.org/TR/CSS2/visudet.html#propdef-height), the percentage height of html box (in this case, height:100%) is relative to the initial containing block.
Robert Hogan
Comment 2 2012-08-09 13:35:50 PDT
WebKit Review Bot
Comment 3 2012-08-09 15:25:03 PDT
Comment on attachment 157531 [details] Patch Attachment 157531 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13455998 New failing tests: fast/replaced/table-percent-height.html
WebKit Review Bot
Comment 4 2012-08-09 15:25:06 PDT
Created attachment 157562 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Robert Hogan
Comment 5 2012-08-11 01:32:30 PDT
It turns out the behaviour in the reduction is undefined by CSS. From http://lists.w3.org/Archives/Public/public-css-testsuite/2012Aug/0029.html: " http://roberthogan.net/css/max-height-percentage-replaced-006.htm html, body, table, tbody, td { width: 100%; height: 100%; padding: 0; margin: 0; vertical-align: top; } table-row-groups can not be set a height; that's given by the spec or http://wiki.csswg.org/spec/css2.1#issue-133 and table-row-groups can not be set to a width 10.2 Content width: the 'width' property 'width' Applies to: all elements but non-replaced inline elements, table rows, and row groups http://www.w3.org/TR/CSS21/visudet.html#the-width-property So, we have again a situation where any size is possible, permissible."
Robert Hogan
Comment 6 2012-08-11 05:40:57 PDT
WebKit Review Bot
Comment 7 2012-08-11 06:17:00 PDT
Comment on attachment 157869 [details] Patch Attachment 157869 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13477569 New failing tests: fast/css/max-height-percentage-replaced-006.htm
WebKit Review Bot
Comment 8 2012-08-11 06:17:04 PDT
Created attachment 157871 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Robert Hogan
Comment 9 2012-08-11 07:30:04 PDT
Julien Chaffraix
Comment 10 2012-09-04 15:19:34 PDT
Comment on attachment 157873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157873&action=review > Source/WebCore/ChangeLog:63 > + The containing block for a table cell is undefined by CSS, as is the height of table row groups, so in reality there is no Are you sure the containing block of a table cell is undefined? Here is my interpretation: Section 10.1 (definition of containing block) "For other elements [non-root elements], if the element’s position is ’relative’ or ’static’, the containing block is formed by the content edge of the nearest block container ancestor box." Section 17.4 Tables in the visual formatting model "The table wrapper box is a ’block’ box if the table is block-level, and an ’inline-block’ box if the table is inline-level. The table wrapper box establishes a block formatting context." As none of the other table parts are blocks, the cell's containing block is the table wrapper box. > Source/WebCore/rendering/RenderBox.cpp:2253 > // FIXME: This needs to be made block-flow-aware. If the cell and image are perpendicular block-flows, this isn't right. You should probably rephrase this. block-flows was renamed to writing-modes since this was written. > Source/WebCore/rendering/RenderBox.cpp:-2260 > - // <http://bugs.webkit.org/show_bug.cgi?id=15359> Please mention somewhere in the ChangeLog that bug 15359 is solved so this is a rotten comment. Btw, bug 15359 mentions bug 81084. Shouldn't this change also solve bug 81084? (if it does, it should be mentioned in the ChangeLog). > Source/WebCore/rendering/RenderBox.cpp:2265 > - return valueForLength(logicalHeight, availableHeight - borderAndPaddingLogicalHeight()); > + return availableHeight - borderAndPaddingLogicalHeight(); This will break the case of -webkit-calc when there is no circular dependency (ie -webkit-calc(50px + 25px) for example). Not sure how much we care about this case but it's worth mentioning it.
Julien Chaffraix
Comment 11 2012-09-04 15:20:22 PDT
CC'ing Mike in case he has a comment about the -webkit-calc change.
Robert Hogan
Comment 12 2012-09-05 10:49:10 PDT
(In reply to comment #10) > (From update of attachment 157873 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157873&action=review > > > Source/WebCore/ChangeLog:63 > > + The containing block for a table cell is undefined by CSS, as is the height of table row groups, so in reality there is no > > Are you sure the containing block of a table cell is undefined? > > Here is my interpretation: > > Section 10.1 (definition of containing block) > > "For other elements [non-root elements], if the element’s position is ’relative’ or ’static’, the containing > block is formed by the content edge of the nearest block container ancestor box." > > Section 17.4 Tables in the visual formatting model > > "The table wrapper box is a ’block’ box if the table is block-level, and an > ’inline-block’ box if the table is inline-level. The table wrapper box establishes a block > formatting context." > > As none of the other table parts are blocks, the cell's containing block is the table wrapper box. I took this to the css-test-suite list when I was looking at the issue: http://lists.w3.org/Archives/Public/public-css-testsuite/2012Aug/0023.html The consensus there at the time was that it probably does not. They agreed with you that the only possible intepretation was the table-wrapper box - but no one treats it that way so it isn't a sane interpreation. :/ > > > Source/WebCore/rendering/RenderBox.cpp:2253 > > // FIXME: This needs to be made block-flow-aware. If the cell and image are perpendicular block-flows, this isn't right. > > You should probably rephrase this. block-flows was renamed to writing-modes since this was written. > > > Source/WebCore/rendering/RenderBox.cpp:-2260 > > - // <http://bugs.webkit.org/show_bug.cgi?id=15359> > > Please mention somewhere in the ChangeLog that bug 15359 is solved so this is a rotten comment. > > Btw, bug 15359 mentions bug 81084. Shouldn't this change also solve bug 81084? (if it does, it should be mentioned in the ChangeLog). I'll take a look. > > > Source/WebCore/rendering/RenderBox.cpp:2265 > > - return valueForLength(logicalHeight, availableHeight - borderAndPaddingLogicalHeight()); > > + return availableHeight - borderAndPaddingLogicalHeight(); > > This will break the case of -webkit-calc when there is no circular dependency (ie -webkit-calc(50px + 25px) for example). Not sure how much we care about this case but it's worth mentioning it.
Mike Lawther
Comment 13 2012-09-05 20:49:32 PDT
> > Source/WebCore/rendering/RenderBox.cpp:2265 > > - return valueForLength(logicalHeight, availableHeight - borderAndPaddingLogicalHeight()); > > + return availableHeight - borderAndPaddingLogicalHeight(); > > This will break the case of -webkit-calc when there is no circular dependency (ie -webkit-calc(50px + 25px) for example). Not sure how much we care about this case but it's worth mentioning it. I would of course like to have calc() work for tables. However, there is a get-out-of-jail-free clause in the calc spec that allows ignoring calc for tables.
Julien Chaffraix
Comment 14 2012-10-22 20:02:31 PDT
Comment on attachment 157873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157873&action=review As mentioned on IRC, the test cases should really be added to the official test suite as we are breaking some compatibility here. Also if you don't intent to upload them to the test suite, they could probably be written in a better way (like HTML 5 doctype, maybe some would be check-layout.js, ...) >>> Source/WebCore/ChangeLog:63 >>> + The containing block for a table cell is undefined by CSS, as is the height of table row groups, so in reality there is no >> >> Are you sure the containing block of a table cell is undefined? >> >> Here is my interpretation: >> >> Section 10.1 (definition of containing block) >> >> "For other elements [non-root elements], if the element’s position is ’relative’ or ’static’, the containing >> block is formed by the content edge of the nearest block container ancestor box." >> >> Section 17.4 Tables in the visual formatting model >> >> "The table wrapper box is a ’block’ box if the table is block-level, and an >> ’inline-block’ box if the table is inline-level. The table wrapper box establishes a block >> formatting context." >> >> As none of the other table parts are blocks, the cell's containing block is the table wrapper box. > > I took this to the css-test-suite list when I was looking at the issue: http://lists.w3.org/Archives/Public/public-css-testsuite/2012Aug/0023.html > > The consensus there at the time was that it probably does not. They agreed with you that the only possible intepretation was the table-wrapper box - but no one treats it that way so it isn't a sane interpreation. :/ OK, I was just picking you on the fact that you said 'there is no defined containing block', which there is even if it doesn't make sense. I think you should update the prose to reflect that there *is* a definition in the spec but all browsers ignore it. >>> Source/WebCore/rendering/RenderBox.cpp:2265 >>> + return availableHeight - borderAndPaddingLogicalHeight(); >> >> This will break the case of -webkit-calc when there is no circular dependency (ie -webkit-calc(50px + 25px) for example). Not sure how much we care about this case but it's worth mentioning it. > > As Mike pointed out, this is still an issue but something we can move to a follow-up bug.
Robert Hogan
Comment 15 2012-12-22 03:34:04 PST
WebKit Review Bot
Comment 16 2012-12-22 05:35:17 PST
Comment on attachment 180601 [details] Patch Attachment 180601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15446832 New failing tests: tables/mozilla/bugs/bug131020.html fast/replaced/percent-height-in-anonymous-block-in-table.html fast/table/padding-height-and-override-height.html
Robert Hogan
Comment 17 2012-12-22 07:51:42 PST
Build Bot
Comment 18 2012-12-22 08:17:20 PST
Comment on attachment 180604 [details] Patch Attachment 180604 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15458759 New failing tests: fast/css/max-height-percentage-replaced-003.htm fast/css/max-height-percentage-replaced-001.htm fast/css/max-height-percentage-replaced-002.htm fast/replaced/table-percent-height.html fast/css/min-height-percentage-replaced-001.htm fast/css/min-height-percentage-replaced-002.htm fast/css/max-height-percentage-replaced-004.htm
Build Bot
Comment 19 2013-01-16 04:35:17 PST
Comment on attachment 180604 [details] Patch Attachment 180604 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15884975 New failing tests: fast/css/max-height-percentage-replaced-003.htm fast/css/max-height-percentage-replaced-001.htm fast/css/max-height-percentage-replaced-002.htm fast/replaced/table-percent-height.html fast/css/min-height-percentage-replaced-001.htm fast/css/min-height-percentage-replaced-002.htm fast/css/max-height-percentage-replaced-004.htm
Robert Hogan
Comment 20 2013-02-09 05:14:52 PST
Build Bot
Comment 21 2013-02-09 08:06:05 PST
Comment on attachment 187424 [details] Patch Attachment 187424 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16474354 New failing tests: fast/replaced/table-percent-height.html
Build Bot
Comment 22 2013-02-09 08:35:15 PST
Comment on attachment 187424 [details] Patch Attachment 187424 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16465562 New failing tests: fast/replaced/table-percent-height.html
Build Bot
Comment 23 2013-02-09 15:12:07 PST
Comment on attachment 187424 [details] Patch Attachment 187424 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16465674 New failing tests: http/tests/cache/cached-main-resource.html fast/replaced/table-percent-height.html
Julien Chaffraix
Comment 24 2013-02-27 14:19:15 PST
Comment on attachment 187424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187424&action=review Also there seem to be a fair amount of behavioral change in our tests that will probably cause some compatibility issues (I have been CC'ed on more regressions that I would have liked lately). Thus the bar to us changing should be higher and I would prefer if the tests (at least the 2 cores) were accepted into the official CSS 2.1 test suite as that would ensure our compliance. > LayoutTests/fast/css/min-height-percentage-replaced-001.htm:7 > + <link rel="author" title="WebKit" href="http://www.webkit.org/"> > + <link rel="author" title="Robert Hogan" href="robert@webkit.org"> > + <link rel="help" href="http://www.w3.org/TR/CSS21/visudet.html#min-max-heights"> If you don't intent to submit these to the W3C these entries are just useless and I find them confusing to be frank. However as you seem to be changing our behavior (and breaking table-percent-height.html), I would rather see them accepted first, especially: * fast/css/height-percentage-replaced-004.htm * fast/css/max-height-percentage-replaced-003.htm as they represents the core of the change. > LayoutTests/platform/chromium-win/fast/replaced/table-percent-height-expected.txt:71 > +FAIL getHeight('input-checkbox-75') [4.5px] is not 75% of getHeight('input-checkbox-100') [6px]. > PASS getWidth('input-file-75') is getWidth('input-file-100') > PASS getHeight('input-file-75') != '0px' is true > -PASS getHeight('input-file-75') is 75% of getHeight('input-file-100'). > -PASS getWidth('input-image-75') is '75px' > -PASS getHeight('input-image-75') is '75px' > +FAIL getHeight('input-file-75') [16.5px] is not 75% of getHeight('input-file-100') [22px]. > +PASS getWidth('input-image-75') is '100px' > +PASS getHeight('input-image-75') is '100px' > PASS getWidth('input-image-100') is '100px' > PASS getHeight('input-image-100') is '100px' > PASS getWidth('input-radio-75') is getWidth('input-radio-100') > PASS getHeight('input-radio-75') != '0px' is true > -PASS getHeight('input-radio-75') is 75% of getHeight('input-radio-100'). > +FAIL getHeight('input-radio-75') [2.25px] is not 75% of getHeight('input-radio-100') [3px]. FAIL :( Is there any way you can make them say PASS as it's a no-go from my perspective to have a passing test say fail.
James Wilson
Comment 25 2019-03-12 04:38:45 PDT
There is also an issue in my site. It turns out the behaviour in the reduction is undefined by CSS. https://centralmenus.com/css/max-height-percentage-replaced-001.htm html, body, table, tbody, td { width: 100%; height: 100%; padding: 0; margin: 0; vertical-align: top; } table-row-groups can not be set a height; that's given by the spec or
Note You need to log in before you can comment on or make changes to this bug.