Bug 32541 – Percentage height should be ignored in some cases

• Component: Layout and Rendering
• Status: NEW

[Kent Tamura]

Open LayoutTests/fast/replaced/table-percent-height.html with Firefox, IE8, or Opera.  The page has some pairs of 75%-height element and 100%-height element.  You'll see each of pairs has identical elements with these browsers, and not with WebKit browsers.
Other browsers ignore "height:N%" in it because of the following rule of CSS 2.1:

<percentage>
    Specifies a percentage height. The percentage is calculated with respect to the height
    of the generated box's containing block. If the height of the containing block is not
    specified explicitly (i.e., it depends on content height), and this element is not absolutely
    positioned, the value computes to 'auto'. A percentage height on the root element is
    relative to the initial containing block. 

We should fix this because of browser compatibility and standard compliance.

[Kent Tamura]

Created attachment 44921 [details]
Proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44921 [details] without any errors.

[Kent Tamura]

Created attachment 44923 [details]
Proposed patch (rev.2)

The previous patch had a unexpected change.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44923 [details] without any errors.

[Kent Tamura]

Ah, I need to add -expected.png to the patch.

[Kent Tamura]

Created attachment 44927 [details]
Proposed patch (rev.3)

* Add expected PNGs
* Fix some typos

[Beth Dakin]

Comment on attachment 44927 [details]
Proposed patch (rev.3)

> +            if (cb->isTableCell() && cb->style()->height().isAuto()
> +                // Check the table for the cell too.. This is needed to pass
> +                // LayoutTests/tables/mozilla/bug/bug32205-2.html.
> +                && cb->containingBlock()->style()->height().isAuto()) {

We don't usually put comments in the middle of the condition of an if-statement like you have done here, and I don't think it's a style we want to encourage. This comment should be moved out. I also think there needs to be a better justification for checking the containing block's containing block than that it is needed to make a test pass. Are you sure this isn't just a hack for that test?

> +                // Reset to Auto.
> +                style()->setHeight(Length());

Generally, we avoid changing the RenderStyle's values like this. Instead, when this case is detected while calculating the height of the renderer, the height of the renderer should be adjusted accordingly, but you should not have to change the RenderStyle.

> +                    RenderTheme::defaultTheme()->adjustSize(style());

You should also avoid calling into the RenderTheme. Again, you want to change the render's height value, not the RenderStyle's. If you work on fixing the renderer's calculated height value, calling into the theme should not be necessary.

> +void RenderTheme::adjustSize(RenderStyle* style)
>
I don't understand how this new function is related to this bug or why it is necessary. But it certainly won't be necessary when you focus on calculating the correct height for the renderer rather than changing the RenderStyle.

Also, I we try to avoid such class-specific code (in this case, it's table-specific code) in RenderBox. If you find that calcHeight() is still the right place to patch when you work on the calculation of the renderer's height, then you should consider creating a version of calcHeight in one of the RenderTable* files to do the table-specific parts of the calculation.

[Kent Tamura]

(In reply to comment #7)

Ok, I understand we should not modify RenderStyle values.

A difficult point to fix this bug is that only RenderTheme::adjustStyle() knows the intrinsic sizes of form controls, and adjustStyle() is called before constructing renderer nodes.  It's hard to check if we should ignore percentage heights in adjustStyle() because it's hard to obtain containingBlock in it.

A solution would be to introduce a function returning intrinsic sizes of form controls.  It will be called by adjustStyle() and renderers.
What do you think?

[Kent Tamura]

*** Bug 47286 has been marked as a duplicate of this bug. ***