Bug 43319 - CSS 2.1 failure: max-height doesn't work for elements within table cell
: CSS 2.1 failure: max-height doesn't work for elements within table cell
Status: NEW
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Major
Assigned To:
: http://elv1s.ru/files/html+css/max-he...
: HasReduction
:
:
  Show dependency treegraph
 
Reported: 2010-08-01 15:35 PST by
Modified: 2013-05-28 11:59 PST (History)


Attachments
Patch (48.64 KB, patch)
2012-08-09 13:35 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (376.29 KB, application/zip)
2012-08-09 15:25 PST, WebKit Review Bot
no flags Details
Patch (55.18 KB, patch)
2012-08-11 05:40 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (370.71 KB, application/zip)
2012-08-11 06:17 PST, WebKit Review Bot
no flags Details
Patch (55.19 KB, patch)
2012-08-11 07:30 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (56.98 KB, patch)
2012-12-22 03:34 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (57.13 KB, patch)
2012-12-22 07:51 PST, Robert Hogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (57.26 KB, patch)
2013-02-09 05:14 PST, Robert Hogan
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-01 15:35:07 PST
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
------- Comment #1 From 2010-10-07 02:36:35 PST -------
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.
------- Comment #2 From 2012-08-09 13:35:50 PST -------
Created an attachment (id=157531) [details]
Patch
------- Comment #3 From 2012-08-09 15:25:03 PST -------
(From update of attachment 157531 [details])
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
------- Comment #4 From 2012-08-09 15:25:06 PST -------
Created an attachment (id=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
------- Comment #5 From 2012-08-11 01:32:30 PST -------
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."
------- Comment #6 From 2012-08-11 05:40:57 PST -------
Created an attachment (id=157869) [details]
Patch
------- Comment #7 From 2012-08-11 06:17:00 PST -------
(From update of attachment 157869 [details])
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
------- Comment #8 From 2012-08-11 06:17:04 PST -------
Created an attachment (id=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
------- Comment #9 From 2012-08-11 07:30:04 PST -------
Created an attachment (id=157873) [details]
Patch
------- Comment #10 From 2012-09-04 15:19:34 PST -------
(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.

> 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.
------- Comment #11 From 2012-09-04 15:20:22 PST -------
CC'ing Mike in case he has a comment about the -webkit-calc change.
------- Comment #12 From 2012-09-05 10:49:10 PST -------
(In reply to comment #10)
> (From update of attachment 157873 [details] [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.
------- Comment #13 From 2012-09-05 20:49:32 PST -------
> > 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.
------- Comment #14 From 2012-10-22 20:02:31 PST -------
(From update of attachment 157873 [details])
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.
------- Comment #15 From 2012-12-22 03:34:04 PST -------
Created an attachment (id=180601) [details]
Patch
------- Comment #16 From 2012-12-22 05:35:17 PST -------
(From update of attachment 180601 [details])
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
------- Comment #17 From 2012-12-22 07:51:42 PST -------
Created an attachment (id=180604) [details]
Patch
------- Comment #18 From 2012-12-22 08:17:20 PST -------
(From update of attachment 180604 [details])
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
------- Comment #19 From 2013-01-16 04:35:17 PST -------
(From update of attachment 180604 [details])
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
------- Comment #20 From 2013-02-09 05:14:52 PST -------
Created an attachment (id=187424) [details]
Patch
------- Comment #21 From 2013-02-09 08:06:05 PST -------
(From update of attachment 187424 [details])
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
------- Comment #22 From 2013-02-09 08:35:15 PST -------
(From update of attachment 187424 [details])
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
------- Comment #23 From 2013-02-09 15:12:07 PST -------
(From update of attachment 187424 [details])
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
------- Comment #24 From 2013-02-27 14:19:15 PST -------
(From update of attachment 187424 [details])
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.