Bug 11355

Summary: Content size of child having percent height inside a fixed height container having overflow:auto is wrongly calculated
Product: WebKit Reporter: Jonas Hartmann <fwd_webkit_bugzilla>
Component: Layout and RenderingAssignee: Pravin D <pravind.2k4>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, fwd_webkit_bugzilla, hyatt, jchaffraix, mitz, pravind.2k4, robert, webkit.review.bot
Priority: P2 Keywords: LayoutTestFailure
Version: 525.x (Safari 3.2)   
Hardware: All   
OS: All   
URL: http://jonas-hartmann.com/css2/index1.html
Attachments:
Description Flags
Wrong Rendering
none
Correct Rendering (by swapping widths and heights)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jonas Hartmann 2006-10-19 06:20:39 PDT
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
Comment 1 mitz 2006-10-19 07:21:45 PDT
The test case renders the same (with two vertical scrollbars) in Firefox, Opera and Safari 2.0.4.
Comment 2 Jonas Hartmann 2006-10-19 07:51:35 PDT
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
Comment 3 Dave Hyatt 2006-10-19 14:21:27 PDT
Why do you think there should only be one set of scrollbars?  This doesn't seem like a bug to me.
Comment 4 Dave Hyatt 2006-10-19 23:10:30 PDT
Resolving invalid.
Comment 5 Jonas Hartmann 2006-10-21 10:03:50 PDT
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?
Comment 6 Dave Hyatt 2006-10-21 14:57:10 PDT
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.
Comment 7 Dave Hyatt 2006-10-21 14:57:33 PDT
Yes, this is a bug.  We do the right thing for width, but not for height.
Comment 8 Jonas Hartmann 2006-10-21 15:31:30 PDT
OT:
Thanks for your time.
And thanks for investing much work into yet another ^^ GOOD browser webdevs like me tend to like :-)
Comment 9 Jonas Hartmann 2006-11-01 10:57:05 PST
Created attachment 11331 [details]
Wrong Rendering
Comment 10 Jonas Hartmann 2006-11-01 10:58:29 PST
Created attachment 11332 [details]
Correct Rendering (by swapping widths and heights)
Comment 11 Jonas Hartmann 2009-02-08 04:53:25 PST
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.
Comment 12 Pravin D 2012-07-12 10:41:55 PDT
Created attachment 151995 [details]
Patch
Comment 13 Dave Hyatt 2012-07-12 10:55:17 PDT
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.
Comment 14 Pravin D 2012-07-12 14:31:25 PDT
Created attachment 152062 [details]
Patch
Comment 15 Pravin D 2012-07-12 15:00:49 PDT
(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...
Comment 16 Julien Chaffraix 2012-07-17 12:49:25 PDT
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.
Comment 17 Pravin D 2012-07-18 12:50:46 PDT
Created attachment 153068 [details]
Patch
Comment 18 Pravin D 2012-07-18 12:55:18 PDT
Created attachment 153069 [details]
Patch
Comment 19 Pravin D 2012-07-18 13:02:08 PDT
(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...
Comment 20 Julien Chaffraix 2012-07-18 13:27:04 PDT
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.
Comment 21 Pravin D 2012-07-18 13:40:32 PDT
Created attachment 153080 [details]
Patch
Comment 22 WebKit Review Bot 2012-07-18 15:28:34 PDT
Comment on attachment 153080 [details]
Patch

Clearing flags on attachment: 153080

Committed r123031: <http://trac.webkit.org/changeset/123031>
Comment 23 WebKit Review Bot 2012-07-18 15:28:53 PDT
All reviewed patches have been landed.  Closing bug.