CSS 2.1 failure: max-height doesn't work for elements within table cell

• Component: CSS
• Status: NEW ---

[Nikita Vasilyev]

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

[kyounga]

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]

Created attachment 157531 [details]
Patch

[WebKit Review Bot]

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]

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]

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]

Created attachment 157869 [details]
Patch

[WebKit Review Bot]

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]

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]

Created attachment 157873 [details]
Patch

[Julien Chaffraix]

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]

CC'ing Mike in case he has a comment about the -webkit-calc change.

[Robert Hogan]

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

> > 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 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]

Created attachment 180601 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 180604 [details]
Patch

[Build Bot]

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 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]

Created attachment 187424 [details]
Patch

[Build Bot]

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 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 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 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="[e-mail]">
> +        <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]

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