Bug 11355 – Content size of child having percent height inside a fixed height container having overflow:auto is wrongly calculated

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jonas Hartmann]

Load http://jonas-hartmann.com/css2/index1.html

Scroll down and to the sides.

What happens:
Multiple scrollbars to get space for inner box+scrollbarwidth

What should happen:
Only the outer scrollbar should be displayed.

Also see:
http://www.w3.org/TR/CSS21/visufx.html#overflow

The test case renders the same (with two vertical scrollbars) in Firefox, Opera and Safari 2.0.4.

[Jonas Hartmann]

This is true. It renders wrong there too.

"In the case of a scrollbar being placed on an edge of the element's box, it should be inserted between the inner border edge and the outer padding edge. Any space taken up by the scrollbars should be subtracted from the computed width/height, thus preserving the inner border edge."

Source: http://www.w3.org/TR/CSS21/visufx.html#overflow


A Realworld Example - Take a look at this spartial navigation:
http://jonas-hartmann.com/rach/
It's broken.

(My testcase just isolates the Problem)

Regards
 Jonas

[Dave Hyatt]

Why do you think there should only be one set of scrollbars?  This doesn't seem like a bug to me.

[Dave Hyatt]

Resolving invalid.

[Jonas Hartmann]

Because:
"...Any space taken up by the scrollbars should be subtracted from the computed
width/height, thus preserving the inner border edge."

If you have one box with 200x100 pixels and another box with 200% width within that. The inner Box should be 200-scrollbarheight pixel. Because space should be substracted from the computed width/height.

What is unclear about that?

[Dave Hyatt]

Oh, I didn't understand this correctly.  You expect the outer horizontal scrollbar to display and the inner vertical scrollbar to display, but the outer vertical scrollbar is bogus.

Got it.

[Dave Hyatt]

Yes, this is a bug.  We do the right thing for width, but not for height.

[Jonas Hartmann]

OT:
Thanks for your time.
And thanks for investing much work into yet another ^^ GOOD browser webdevs like me tend to like :-)

[Jonas Hartmann]

Created attachment 11331 [details]
Wrong Rendering

[Jonas Hartmann]

Created attachment 11332 [details]
Correct Rendering (by swapping widths and heights)

[Jonas Hartmann]

Both test and reference work perfect on Firefox since 3.x (last tested 3.0.6) now. The equal bug has been fixed in on mozilla's bugzilla. There you will find hints on why mozilla first did not want to fix the bug but later on it seemed to have happend https://bugzilla.mozilla.org/show_bug.cgi?id=301726 (I think it was reflow refactoring which made it)

Here is the test and reference again:
http://devedge.jonas-hartmann.com/css2/index1_test.html
http://devedge.jonas-hartmann.com/css2/index1_reference.html

JFYI: Opera 10 Alpha still renders wrong as well.

[Pravin D]

Created attachment 151995 [details]
Patch

[Dave Hyatt]

Comment on attachment 151995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151995&action=review

> Source/WebCore/ChangeLog:14
> +        Subtracting the height of the scrollbar from the client height when the client has percentage width.

I think you meant to say "client has percentage height."

> Source/WebCore/rendering/RenderBox.cpp:2147
> +        result -= LayoutUnit(cb->scrollbarLogicalHeight());

You don't want to go negative here, e.g., if the scrollbar height is > than the available height. You should max with 0, e.g., result = max(0, result - cb->scrollbarLogicalHeight());

I am assuming that isn't handled later. If it is, disregard.

Don't you need to patch the replaced case also? I would like to see a test where the inner object is a replaced element like an image, since I suspect you need to patch the replaced logical height computation as well? Specifically computeReplacedLogicalHeightUsing.

[Pravin D]

Created attachment 152062 [details]
Patch

[Pravin D]

(In reply to comment #13)
> (From update of attachment 151995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151995&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        Subtracting the height of the scrollbar from the client height when the client has percentage width.
> 
> I think you meant to say "client has percentage height."
> 
> > Source/WebCore/rendering/RenderBox.cpp:2147
> > +        result -= LayoutUnit(cb->scrollbarLogicalHeight());
> 
> You don't want to go negative here, e.g., if the scrollbar height is > than the available height. You should max with 0, e.g., result = max(0, result - cb->scrollbarLogicalHeight());
> 
> I am assuming that isn't handled later. If it is, disregard.
> 
> Don't you need to patch the replaced case also? I would like to see a test where the inner object is a replaced element like an image, since I suspect you need to patch the replaced logical height computation as well? Specifically computeReplacedLogicalHeightUsing.
> 

Upload new patch with the suggested fixes.
I'm not sure if we have to handle the case where we have percent width child(replaced or normal) inside a table cell. 
Current behavior in case of repplaced elements is that the table cell ignores its fixed height value and expands sufficiently so that horizontal scroll bars  + some paddings are accounted for. 
A vertical scroll bar appears in case the child is non-replaced element. 

Other browsers especial FF as very weird behavior.

Also check positioned objs case. Could only think of 2 cases
1) Positioned child(replaced or normal) inside non-positioned container.
In this case if the child has percent width, it fills the viewport... so don't without a scroll bar.

2) Positioned child(replaced or normal) inside positioned container.
Things seem to work fine in this case. 

Atleast from what i could conclude was that we need not fix for positioned replaced/non-replaced objs.

Kindly let me know you opinion...

[Julien Chaffraix]

Comment on attachment 152062 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152062&action=review

> Source/WebCore/ChangeLog:3
> +        Overflow: auto; in 2 cascaded boxes, outer having overflow: auto;, inner having overflow: auto; height: 100%; width: 200%; - Renders scrollbars on cascaded block elements in wrong way

Could you change the name to something that underlines the bug a bit more here?

> Source/WebCore/rendering/RenderBox.cpp:2145
>          result = cb->computeContentBoxLogicalHeight(cbstyle->logicalHeight().value());

Nit: Probably more readable to use a temporary variable here to avoid re-using |result| 3 times.

> Source/WebCore/rendering/RenderBox.cpp:2146
> +        result = max<LayoutUnit>(0, result - LayoutUnit(cb->scrollbarLogicalHeight()));

No need for the conversion to LayoutUnit here.

> Source/WebCore/rendering/RenderBox.cpp:2285
> +                availableHeight = max<LayoutUnit>(0, availableHeight - LayoutUnit(toRenderBox(cb)->scrollbarLogicalHeight()));

Unneeded conversion to LayoutUnit here too.

> LayoutTests/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

This should be removed.

> LayoutTests/fast/overflow/child-100percent-height-inside-fixed-container-with-overflow-auto-expected.txt:1
> +The container element should not contain a vertical scrollbar. The height of the inner box should be 100% of the containers height minus the height of horizontal scrollbar

I like to include the bug number and description in the tests. You can either append it to the description() or use debug() here to achieve that.

[Pravin D]

Created attachment 153068 [details]
Patch

[Pravin D]

Created attachment 153069 [details]
Patch

[Pravin D]

(In reply to comment #16)
> (From update of attachment 152062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152062&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Overflow: auto; in 2 cascaded boxes, outer having overflow: auto;, inner having overflow: auto; height: 100%; width: 200%; - Renders scrollbars on cascaded block elements in wrong way
> 
> Could you change the name to something that underlines the bug a bit more here?
> 
Done...
> > Source/WebCore/rendering/RenderBox.cpp:2145
> >          result = cb->computeContentBoxLogicalHeight(cbstyle->logicalHeight().value());
> 
> Nit: Probably more readable to use a temporary variable here to avoid re-using |result| 3 times.
> 
Done...
> > Source/WebCore/rendering/RenderBox.cpp:2146
> > +        result = max<LayoutUnit>(0, result - LayoutUnit(cb->scrollbarLogicalHeight()));
> 
> No need for the conversion to LayoutUnit here.
> 
> > Source/WebCore/rendering/RenderBox.cpp:2285
> > +                availableHeight = max<LayoutUnit>(0, availableHeight - LayoutUnit(toRenderBox(cb)->scrollbarLogicalHeight()));
> 
> Unneeded conversion to LayoutUnit here too.
> 
I guess you where refering to the conversion LayoutUnit(cb->scrollbarLogicalHeight()) ... Have also removed the conversion from line 2146.
> > LayoutTests/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> This should be removed.
> 
Done...
> > LayoutTests/fast/overflow/child-100percent-height-inside-fixed-container-with-overflow-auto-expected.txt:1
> > +The container element should not contain a vertical scrollbar. The height of the inner box should be 100% of the containers height minus the height of horizontal scrollbar
> 
> I like to include the bug number and description in the tests. You can either append it to the description() or use debug() here to achieve that.
>  

Done...

[Julien Chaffraix]

Comment on attachment 153069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153069&action=review

> Source/WebCore/rendering/RenderBox.cpp:2131
> +        // height.

Nit: you can move the last word of the comment to the previous line.

> LayoutTests/fast/overflow/child-100percent-height-inside-fixed-container-with-overflow-auto.html:22
> +if(window.layoutTestController)

It should be window.testRunner (we are renaming layoutTestController, see thread on webkit-dev about that).

I would rather have one representation between DRT and non-DRT. Something like:

<a href="https://bugs.webkit.org/show_bug.cgi?id=11355">https://bugs.webkit.org/show_bug.cgi?id=11355</a>

would dump in the same manner in DRT and a browser.

> LayoutTests/fast/overflow/replaced-child-100percent-height-inside-fixed-container-with-overflow-auto.html:25
> +if(window.layoutTestController)
> +    linkToBug = 'Bug https://bugs.webkit.org/show_bug.cgi?id=11355';
> +else
> +    linkToBug = '<a href="https://bugs.webkit.org/show_bug.cgi?id=11355">Bug #11355</a>';

Same comment.

[Pravin D]

Created attachment 153080 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153080 [details]
Patch

Clearing flags on attachment: 153080

Committed r123031: <http://trac.webkit.org/changeset/123031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.