Bug 73038 - Fix Aspect Ratio Property Inheritance And Make the Computed Value Equal the Specified Value
Summary: Fix Aspect Ratio Property Inheritance And Make the Computed Value Equal the S...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2011-11-23 11:55 PST by Fady Samuel
Modified: 2011-11-28 12:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.44 KB, patch)
2011-11-23 12:40 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.18 KB, patch)
2011-11-23 13:43 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2011-11-23 17:15 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2011-11-23 20:00 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2011-11-28 12:51 PST, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2011-11-28 12:52 PST, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2011-11-23 11:55:11 PST
Aspect-ratio inheritance is broken. This is probably a one line fix.
Comment 1 Fady Samuel 2011-11-23 12:40:16 PST
Created attachment 116401 [details]
Patch
Comment 2 Ojan Vafai 2011-11-23 12:45:16 PST
Comment on attachment 116401 [details]
Patch

Can you add a test for this? The computed style on an element that inherits the value should be wrong without this patch, right?

Also, now that I think about it, it doesn't make sense to me that this value would inherit. http://www.xanthir.com/blog/b4810 doesn't say whether it should, but I think it probably shouldn't. Maybe bring this up on www-style and then we'll probably want to delete this chunk of code.
Comment 3 Fady Samuel 2011-11-23 13:08:47 PST
(In reply to comment #2)
> (From update of attachment 116401 [details])
> Can you add a test for this? The computed style on an element that inherits the value should be wrong without this patch, right?
> 
> Also, now that I think about it, it doesn't make sense to me that this value would inherit. http://www.xanthir.com/blog/b4810 doesn't say whether it should, but I think it probably shouldn't. Maybe bring this up on www-style and then we'll probably want to delete this chunk of code.

I spoke to Tab about this a few days ago: 

http://www.w3.org/TR/CSS2/cascade.html#value-def-inherit

He says inherit should work for every property and "6.2.1 The 'inherit' value" seems to suggest the same thing. This is broken on IE up to version 8 according to some sources, however.
Comment 4 Tab Atkins 2011-11-23 13:14:52 PST
The *value* 'inherit' is a global value that every property accepts.  'aspect-ratio' should not inherit by default, though.
Comment 5 Fady Samuel 2011-11-23 13:43:57 PST
Created attachment 116415 [details]
Patch
Comment 6 Fady Samuel 2011-11-23 13:45:48 PST
(In reply to comment #2)
> (From update of attachment 116401 [details])
> Can you add a test for this? The computed style on an element that inherits the value should be wrong without this patch, right?
> 

I added a test for parsing, but I realized I should've (as you said) add a test that checks the computed style. I will do that now, thanks.
Comment 7 Ojan Vafai 2011-11-23 14:10:11 PST
(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 116401 [details] [details])
> > Can you add a test for this? The computed style on an element that inherits the value should be wrong without this patch, right?
> > 
> 
> I added a test for parsing, but I realized I should've (as you said) add a test that checks the computed style. I will do that now, thanks.

Yup. Both tests would be good. For the computed style tests, can you check both that we do inherit when the value is inherit and that we don't inherit when it's not?
Comment 8 WebKit Review Bot 2011-11-23 14:24:14 PST
Comment on attachment 116415 [details]
Patch

Attachment 116415 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10645020

New failing tests:
fast/css/aspect-ratio-parsing-tests.html
Comment 9 Fady Samuel 2011-11-23 17:15:53 PST
Created attachment 116458 [details]
Patch
Comment 10 Fady Samuel 2011-11-23 17:16:30 PST
(In reply to comment #9)
> Created an attachment (id=116458) [details]
> Patch

Fixed the computed value of -webkit-aspect-ratio as well after a discussion with Tab.
Comment 11 Ojan Vafai 2011-11-23 18:46:18 PST
Comment on attachment 116458 [details]
Patch

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

C++ code is all fine except for the include. A bunch of nits about the test.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:56
> +#include "RenderStyle.h"

Is this include necessary?

> LayoutTests/fast/css/aspect-ratio-inheritance.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

should use the html doctype
<!DOCTYPE html>

> LayoutTests/fast/css/aspect-ratio-inheritance.html:4
> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">

You don't need the stylesheet. It's automatically inserted. Also, I'd drop the head element as well and just use a div instead of the body element.

> LayoutTests/fast/css/aspect-ratio-inheritance.html:9
> +  <body style="-webkit-aspect-ratio: 1 / 2;">
> +    <div id="aspectRatioTest"></div>

There's no official style for webkit tests, but it makes sense to be consistent where we can. In this case, 4-space indent.

Also, typically we don't indent the top-level elements since every page has them. So, the html, head, body elements and their immediate children are usually not indented.

> LayoutTests/fast/css/aspect-ratio-inheritance.html:21
> +      <script>
> +          function testComputedValue(elementId, value, styleAttribute)
> +          {
> +              var div = document.getElementById(elementId);
> +              div.style[styleAttribute] = value;
> +              var computedValue = window.getComputedStyle(div)[styleAttribute];
> +              return computedValue;
> +          }
> +          shouldBeEqualToString('testComputedValue("aspectRatioTest", "1 / 4", "-webkit-aspect-ratio")', '1/4');
> +          shouldBeEqualToString('testComputedValue("aspectRatioTest", "inherit", "-webkit-aspect-ratio")', '1/2');
> +          shouldBeEqualToString('testComputedValue("aspectRatioTest", "none", "-webkit-aspect-ratio")', 'none');
> +      </script>

This is indented more than the script element below it.
Comment 12 Fady Samuel 2011-11-23 19:41:39 PST
Comment on attachment 116458 [details]
Patch

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

Will make the changes momentarily. I'm just confused about the include comment thanks.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:56
>> +#include "RenderStyle.h"
> 
> Is this include necessary?

Are we not supposed to include what we use? Certainly we're using RenderStyle...
Comment 13 Fady Samuel 2011-11-23 20:00:00 PST
Comment on attachment 116458 [details]
Patch

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

>> LayoutTests/fast/css/aspect-ratio-inheritance.html:1
>> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> should use the html doctype
> <!DOCTYPE html>

Done.

>> LayoutTests/fast/css/aspect-ratio-inheritance.html:4
>> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> 
> You don't need the stylesheet. It's automatically inserted. Also, I'd drop the head element as well and just use a div instead of the body element.

Removed the stylesheet (sorry, been using other layout tests as templates. If I drop the head element, where do I place the pre script? Also, getting rid of the body seems odd (is this valid HTML?). I moved the -webkit-aspect-ratio property to a parent div instead of body though.

>> LayoutTests/fast/css/aspect-ratio-inheritance.html:9
>> +    <div id="aspectRatioTest"></div>
> 
> There's no official style for webkit tests, but it makes sense to be consistent where we can. In this case, 4-space indent.
> 
> Also, typically we don't indent the top-level elements since every page has them. So, the html, head, body elements and their immediate children are usually not indented.

Done.

>> LayoutTests/fast/css/aspect-ratio-inheritance.html:21
>> +      </script>
> 
> This is indented more than the script element below it.

Done.
Comment 14 Fady Samuel 2011-11-23 20:00:50 PST
Created attachment 116473 [details]
Patch
Comment 15 Luke Macpherson 2011-11-23 21:01:27 PST
Could someone please explain why this is stored as two values instead of just a single floating point value?
Comment 16 Fady Samuel 2011-11-23 21:04:17 PST
(In reply to comment #15)
> Could someone please explain why this is stored as two values instead of just a single floating point value?

The specified value has to show both the numerator and denominator so we must store them separately soemwhere.
Comment 17 Fady Samuel 2011-11-28 11:56:58 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Could someone please explain why this is stored as two values instead of just a single floating point value?
> 
> The specified value has to show both the numerator and denominator so we must store them separately soemwhere.

Does anyone object to landing this patch and then iterating over it as Tab Atkins, Luke Macpherson, and I iterate on the details of the spec?
Comment 18 Ojan Vafai 2011-11-28 12:06:23 PST
(In reply to comment #13)
> (From update of attachment 116458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116458&action=review
> >> LayoutTests/fast/css/aspect-ratio-inheritance.html:4
> >> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
> > 
> > You don't need the stylesheet. It's automatically inserted. Also, I'd drop the head element as well and just use a div instead of the body element.
> 
> Removed the stylesheet (sorry, been using other layout tests as templates. If I drop the head element, where do I place the pre script? Also, getting rid of the body seems odd (is this valid HTML?). I moved the -webkit-aspect-ratio property to a parent div instead of body though.

Getting rid of the body is perfectly valid HTML. It gets created automatically. I typically write tests like follows:
<!DOCTYPE html>
<script src="path/to/js-test-pre.js"></script>
<div></div>  <---- test content here.
<script src="path/to/js-test-post.js"></script>

There's no need for html/body/head elements unless your test-case specifically requires them.
Comment 19 Fady Samuel 2011-11-28 12:51:11 PST
Created attachment 116803 [details]
Patch
Comment 20 Fady Samuel 2011-11-28 12:52:36 PST
Created attachment 116804 [details]
Patch
Comment 21 Fady Samuel 2011-11-28 12:55:35 PST
Comment on attachment 116804 [details]
Patch

Clearing flags on attachment: 116804

Committed r101275: <http://trac.webkit.org/changeset/101275>
Comment 22 Fady Samuel 2011-11-28 12:55:42 PST
All reviewed patches have been landed.  Closing bug.