WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
11355
Content size of child having percent height inside a fixed height container having overflow:auto is wrongly calculated
https://bugs.webkit.org/show_bug.cgi?id=11355
Summary
Content size of child having percent height inside a fixed height container h...
Jonas Hartmann
Reported
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
Attachments
Wrong Rendering
(1.02 KB, text/html)
2006-11-01 10:57 PST
,
Jonas Hartmann
no flags
Details
Correct Rendering (by swapping widths and heights)
(1.01 KB, text/html)
2006-11-01 10:58 PST
,
Jonas Hartmann
no flags
Details
Patch
(5.91 KB, patch)
2012-07-12 10:41 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(9.93 KB, patch)
2012-07-12 14:31 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(11.03 KB, text/plain)
2012-07-18 12:50 PDT
,
Pravin D
no flags
Details
Patch
(11.00 KB, patch)
2012-07-18 12:55 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Patch
(10.82 KB, patch)
2012-07-18 13:40 PDT
,
Pravin D
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
mitz
Comment 1
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.
Jonas Hartmann
Comment 2
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
Dave Hyatt
Comment 3
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.
Dave Hyatt
Comment 4
2006-10-19 23:10:30 PDT
Resolving invalid.
Jonas Hartmann
Comment 5
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?
Dave Hyatt
Comment 6
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.
Dave Hyatt
Comment 7
2006-10-21 14:57:33 PDT
Yes, this is a bug. We do the right thing for width, but not for height.
Jonas Hartmann
Comment 8
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 :-)
Jonas Hartmann
Comment 9
2006-11-01 10:57:05 PST
Created
attachment 11331
[details]
Wrong Rendering
Jonas Hartmann
Comment 10
2006-11-01 10:58:29 PST
Created
attachment 11332
[details]
Correct Rendering (by swapping widths and heights)
Jonas Hartmann
Comment 11
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.
Pravin D
Comment 12
2012-07-12 10:41:55 PDT
Created
attachment 151995
[details]
Patch
Dave Hyatt
Comment 13
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.
Pravin D
Comment 14
2012-07-12 14:31:25 PDT
Created
attachment 152062
[details]
Patch
Pravin D
Comment 15
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...
Julien Chaffraix
Comment 16
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.
Pravin D
Comment 17
2012-07-18 12:50:46 PDT
Created
attachment 153068
[details]
Patch
Pravin D
Comment 18
2012-07-18 12:55:18 PDT
Created
attachment 153069
[details]
Patch
Pravin D
Comment 19
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...
Julien Chaffraix
Comment 20
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.
Pravin D
Comment 21
2012-07-18 13:40:32 PDT
Created
attachment 153080
[details]
Patch
WebKit Review Bot
Comment 22
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
>
WebKit Review Bot
Comment 23
2012-07-18 15:28:53 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug