Bug 118516 – % unit heights don't work if parent block height is set in vh

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

[DJ Mountney]

Created attachment 206343 [details]
screenshot

What steps will reproduce the problem?
1. Place below code in body 

<div id="outer" style="height: 50vh; width: 50vw; background-color: lightcyan; border: solid 1px black;" >
	<div id="inner" style="width: 50%; height: 50%; background-color: lightseagreen; border: solid 1px black;"></div>
</div>

What is the expected result?
You should see a seagreen box inside a lightcyan one.

What happens instead?
The inner box has a height of 0.

(In reply to comment #0)
> Created an attachment (id=206343) [details]
> screenshot
> 
> What steps will reproduce the problem?
> 1. Place below code in body 
> 
> <div id="outer" style="height: 50vh; width: 50vw; background-color: lightcyan; border: solid 1px black;" >
>     <div id="inner" style="width: 50%; height: 50%; background-color: lightseagreen; border: solid 1px black;"></div>
> </div>
> 
> What is the expected result?
> You should see a seagreen box inside a lightcyan one.
> 
> What happens instead?
> The inner box has a height of 0.

The issue is already fixed in chromium. Merge can be done.
https://code.google.com/p/chromium/issues/detail?id=225664

Created attachment 211321 [details]
Patch

(In reply to comment #2)
> Created an attachment (id=211321) [details]
> Patch

Hi Darin and Simon. Can you please review this patch?

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=211321) [details] [details]
> > Patch
> 
> Hi Darin and Simon. Can you please review this patch?

Hi Darin and Simon. Was earlier working on this but then saw that its already fixed on chromium branch. I have merged patch. Please review.

[Darin Adler]

I don’t have enough rendering knowledge to evaluate this patch. I’m sure one of the rendering experts can give it a review+.

(In reply to comment #5)
> I don’t have enough rendering knowledge to evaluate this patch. I’m sure one of the rendering experts can give it a review+.

Thanks Darin.

Hi David. Could you please review this?

(In reply to comment #6)
> (In reply to comment #5)
> > I don’t have enough rendering knowledge to evaluate this patch. I’m sure one of the rendering experts can give it a review+.
> 
> Thanks Darin.
> 
> Hi David. Could you please review this?

Hi Beth. Could you please review this?

[Benjamin Poulain]

Comment on attachment 211321 [details]
Patch

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

First round: some style issues:

> Source/WebCore/rendering/RenderBox.cpp:2748
> +            // We need to adjust for min/max height because this method does not
> +            // handle the min/max of the current block, its caller does. So the
> +            // return value from the recursive call will not have been adjusted
> +            // yet.

WebKit comments are not limited to the 80 characters mark.

> LayoutTests/ChangeLog:9
> +        * fast/css/viewport-percentage-compute-box-height-expected.txt: Added.
> +        * fast/css/viewport-percentage-compute-box-height.html: Added.

Do we already have test coverage for viewport width?
If not, that is a good opportunity to add it, even if it is not directly related to your patch.

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:7
> +                description('Tests that % unit heights work if parent block height is set in vh');

Missing period.
The description should also describe what a correct result looks like (for manual testing).

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:8
> +                shouldBeNonZero("document.getElementById('inner').offsetHeight");

This should be testing an exact value if testRunner is present.

Tests are run in a 800*600px viewport, you can rely on that for testing.

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:9
> +                isSuccessfullyParsed();

You should not do this!
Instead, insert js-test-post.js

Created attachment 212989 [details]
Patch

(In reply to comment #8)
> (From update of attachment 211321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211321&action=review
> 
> First round: some style issues:
> 
> > Source/WebCore/rendering/RenderBox.cpp:2748
> > +            // We need to adjust for min/max height because this method does not
> > +            // handle the min/max of the current block, its caller does. So the
> > +            // return value from the recursive call will not have been adjusted
> > +            // yet.
> 
> WebKit comments are not limited to the 80 characters mark.
> 
> > LayoutTests/ChangeLog:9
> > +        * fast/css/viewport-percentage-compute-box-height-expected.txt: Added.
> > +        * fast/css/viewport-percentage-compute-box-height.html: Added.
> 
> Do we already have test coverage for viewport width?
> If not, that is a good opportunity to add it, even if it is not directly related to your patch.
> 
> > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:7
> > +                description('Tests that % unit heights work if parent block height is set in vh');
> 
> Missing period.
> The description should also describe what a correct result looks like (for manual testing).
> 
> > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:8
> > +                shouldBeNonZero("document.getElementById('inner').offsetHeight");
> 
> This should be testing an exact value if testRunner is present.
> 
> Tests are run in a 800*600px viewport, you can rely on that for testing.
> 
> > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:9
> > +                isSuccessfullyParsed();
> 
> You should not do this!
> Instead, insert js-test-post.js

Hi Benjamin.Thanks for the review. Uploaded patch with the changes.

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 211321 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=211321&action=review
> > 
> > First round: some style issues:
> > 
> > > Source/WebCore/rendering/RenderBox.cpp:2748
> > > +            // We need to adjust for min/max height because this method does not
> > > +            // handle the min/max of the current block, its caller does. So the
> > > +            // return value from the recursive call will not have been adjusted
> > > +            // yet.
> > 
> > WebKit comments are not limited to the 80 characters mark.
> > 
> > > LayoutTests/ChangeLog:9
> > > +        * fast/css/viewport-percentage-compute-box-height-expected.txt: Added.
> > > +        * fast/css/viewport-percentage-compute-box-height.html: Added.
> > 
> > Do we already have test coverage for viewport width?
> > If not, that is a good opportunity to add it, even if it is not directly related to your patch.
> > 
> > > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:7
> > > +                description('Tests that % unit heights work if parent block height is set in vh');
> > 
> > Missing period.
> > The description should also describe what a correct result looks like (for manual testing).
> > 
> > > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:8
> > > +                shouldBeNonZero("document.getElementById('inner').offsetHeight");
> > 
> > This should be testing an exact value if testRunner is present.
> > 
> > Tests are run in a 800*600px viewport, you can rely on that for testing.
> > 
> > > LayoutTests/fast/css/viewport-percentage-compute-box-height.html:9
> > > +                isSuccessfullyParsed();
> > 
> > You should not do this!
> > Instead, insert js-test-post.js
> 
> Hi Benjamin.Thanks for the review. Uploaded patch with the changes.

Hi Benjamin. Can you please review? Thanks

[Brent Fulgham]

Right now, this patch looks as if it was written by us. But it sounds like it is a merge from Blink. We should show proper attribution (and perhaps link to their bug) so that we don't appear to be taking credit for work we did not do.

I think the ChangeLog should be updated to reflect the Blink involvement.

Otherwise, this patch looks fine to me.

(In reply to comment #12)
> Right now, this patch looks as if it was written by us. But it sounds like it is a merge from Blink. We should show proper attribution (and perhaps link to their bug) so that we don't appear to be taking credit for work we did not do.
> 
> I think the ChangeLog should be updated to reflect the Blink involvement.
> 
> Otherwise, this patch looks fine to me.

This is my first merge patch. So I was not sure what extra needs to be added or is there any process to follow when we take merge from other projects.

I have no intention of taking credit for the work I have not done so had initially mentioned "I have merged patch."

Can you please elaborate as is what I need to do?

[Brent Fulgham]

(In reply to comment #13)
> (In reply to comment #12)
> > Right now, this patch looks as if it was written by us. But it sounds like it is a merge from Blink. We should show proper attribution (and perhaps link to their bug) so that we don't appear to be taking credit for work we did not do.
> > 
> > I think the ChangeLog should be updated to reflect the Blink involvement.
> > 
> > Otherwise, this patch looks fine to me.
> 
> This is my first merge patch. So I was not sure what extra needs to be added or is there any process to follow when we take merge from other projects.
> 
> I have no intention of taking credit for the work I have not done so had initially mentioned "I have merged patch."
> 
> Can you please elaborate as is what I need to do?

All you need to do is add a line in the Changelog that says something like:

"From Blink r152914 by <[e-mail]>"

Where "r152914" should be replaced by the revision that added the code to the Blink repository, and "jchaffraix...." should be changed to whoever did the original patch.

Thanks!

Created attachment 216907 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 216907 [details]
Patch

Attachment 216907 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22370224

[EFL EWS Bot]

Comment on attachment 216907 [details]
Patch

Attachment 216907 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22720134

Created attachment 216911 [details]
Patch

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Right now, this patch looks as if it was written by us. But it sounds like it is a merge from Blink. We should show proper attribution (and perhaps link to their bug) so that we don't appear to be taking credit for work we did not do.
> > > 
> > > I think the ChangeLog should be updated to reflect the Blink involvement.
> > > 
> > > Otherwise, this patch looks fine to me.
> > 
> > This is my first merge patch. So I was not sure what extra needs to be added or is there any process to follow when we take merge from other projects.
> > 
> > I have no intention of taking credit for the work I have not done so had initially mentioned "I have merged patch."
> > 
> > Can you please elaborate as is what I need to do?
> 
> All you need to do is add a line in the Changelog that says something like:
> 
> "From Blink r152914 by <[e-mail]>"
> 
> Where "r152914" should be replaced by the revision that added the code to the Blink repository, and "jchaffraix...." should be changed to whoever did the original patch.
> 
> Thanks!

Hi Brent. Thanks for the review.
Uploaded new patch. Please review.

(In reply to comment #19)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > Right now, this patch looks as if it was written by us. But it sounds like it is a merge from Blink. We should show proper attribution (and perhaps link to their bug) so that we don't appear to be taking credit for work we did not do.
> > > > 
> > > > I think the ChangeLog should be updated to reflect the Blink involvement.
> > > > 
> > > > Otherwise, this patch looks fine to me.
> > > 
> > > This is my first merge patch. So I was not sure what extra needs to be added or is there any process to follow when we take merge from other projects.
> > > 
> > > I have no intention of taking credit for the work I have not done so had initially mentioned "I have merged patch."
> > > 
> > > Can you please elaborate as is what I need to do?
> > 
> > All you need to do is add a line in the Changelog that says something like:
> > 
> > "From Blink r152914 by <[e-mail]>"
> > 
> > Where "r152914" should be replaced by the revision that added the code to the Blink repository, and "jchaffraix...." should be changed to whoever did the original patch.
> > 
> > Thanks!
> 
> Hi Brent. Thanks for the review.
> Uploaded new patch. Please review.

Hello.Can someone please review this? Thanks.

[Mihai Balan]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #14)
> > 
> > Hi Brent. Thanks for the review.
> > Uploaded new patch. Please review.
> 
> Hello.Can someone please review this? Thanks.

I suggest you ask for a review on IRC (#webkit on freenode). It's unlikely you'll get a review based on bugmail, unless a reviewer is subscribed to this bug and actively watching it.

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #14)
> > > 
> > > Hi Brent. Thanks for the review.
> > > Uploaded new patch. Please review.
> > 
> > Hello.Can someone please review this? Thanks.
> 
> I suggest you ask for a review on IRC (#webkit on freenode). It's unlikely you'll get a review based on bugmail, unless a reviewer is subscribed to this bug and actively watching it.

Have done that as well but no action.
Brent gave some comments I worked on that and submitted patch so thought would review again.

[Brent Fulgham]

Comment on attachment 216911 [details]
Patch

This seems fine to me. I'll see if I can get another renderer review.

[Simon Fraser (smfr)]

Comment on attachment 216911 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2729
> +    } else if (cbstyle.logicalHeight().isViewportPercentage()) {

Ugh, we should rename the cbstyle variable.

> LayoutTests/ChangeLog:13
> +        * fast/css/viewport-percentage-compute-box-height-expected.txt: Added.
> +        * fast/css/viewport-percentage-compute-box-height.html: Added.
> +        * fast/css/viewport-percentage-compute-box-width-expected.txt: Added.
> +        * fast/css/viewport-percentage-compute-box-width.html: Added.

I would prefer these to be ref tests.

Created attachment 218386 [details]
Patch

(In reply to comment #24)
> (From update of attachment 216911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216911&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:2729
> > +    } else if (cbstyle.logicalHeight().isViewportPercentage()) {
> 
> Ugh, we should rename the cbstyle variable.
> 
> > LayoutTests/ChangeLog:13
> > +        * fast/css/viewport-percentage-compute-box-height-expected.txt: Added.
> > +        * fast/css/viewport-percentage-compute-box-height.html: Added.
> > +        * fast/css/viewport-percentage-compute-box-width-expected.txt: Added.
> > +        * fast/css/viewport-percentage-compute-box-width.html: Added.
> 
> I would prefer these to be ref tests.

Thanks Simon for the review. Have uploade new patch as per your suggestions for test cases.

Ugh, we should rename the cbstyle variable : I didnot get this. I mean the in the function other placed this name is used.

[Simon Fraser (smfr)]

Comment on attachment 218386 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 218386 [details]
Patch

Clearing flags on attachment: 218386

Committed r160159: <http://trac.webkit.org/changeset/160159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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