Bug 75441 – getComputedStyle for outline is not implemented.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexis Menard (darktears)]

getComputedStyle for outline is not implemented.

[Alexis Menard (darktears)]

Created attachment 120888 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 120888 [details]
Patch

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

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html:33
> +shouldBe("computedStyle.getPropertyValue('outline')", "'rgb(0, 0, 0) solid 32px'");

I'm a bit puzzled on that return value.

http://www.w3.org/TR/CSS2/ui.html#propdef-outline-color specify the default value to be invert (if supported) otherwise as explained "the 'outline-color' property is the value of the 'color' property, similar to the initial value of the 'border-top-color' property." so black here. My hesitation comes from "Computed value: as specified", in that case I didn't specified one so it should return nothing as it fallback to the default color property. It is different from let say border-color where each border-color sides computed value are "when taken from the 'color' property, the computed value of 'color'; otherwise, as specified". So should I return "solid 32px" rather than "rgb(0, 0, 0) solid 32px"? At the end I prefer the value "rgb(0, 0, 0) solid 32px" as the default value is not obvious, if the user is interested in the computed value having the actual displayed color is nice no matter if specified or not, just like border-color especially the fact that the computed color of the outline may not be obvious.

On a side note Opera returns "solid 32px" on the same test case. IE and FF doesn't work for this case.

[Alexis Menard (darktears)]

Antonio, what do you think about my comment as I added after you r+ it.

[Antonio Gomes]

I'd say returning the current value is acceptable as long as we file a bug to actually return the "proper" value.

Being compatible with Opera would be good enough to leave it as is? Is Opera returning the right value? Is our current return value "worst or better"? Etc ... These are questions to be answered by this follow up bug I am suggesting.

(In reply to comment #2)
> (From update of attachment 120888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120888&action=review
> 
> > LayoutTests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html:33
> > +shouldBe("computedStyle.getPropertyValue('outline')", "'rgb(0, 0, 0) solid 32px'");
> 
> I'm a bit puzzled on that return value.
> 
> http://www.w3.org/TR/CSS2/ui.html#propdef-outline-color specify the default value to be invert (if supported) otherwise as explained "the 'outline-color' property is the value of the 'color' property, similar to the initial value of the 'border-top-color' property." so black here. My hesitation comes from "Computed value: as specified", in that case I didn't specified one so it should return nothing as it fallback to the default color property. It is different from let say border-color where each border-color sides computed value are "when taken from the 'color' property, the computed value of 'color'; otherwise, as specified". So should I return "solid 32px" rather than "rgb(0, 0, 0) solid 32px"? At the end I prefer the value "rgb(0, 0, 0) solid 32px" as the default value is not obvious, if the user is interested in the computed value having the actual displayed color is nice no matter if specified or not, just like border-color especially the fact that the computed color of the outline may not be obvious.
> 
> On a side note Opera returns "solid 32px" on the same test case. IE and FF doesn't work for this case.

[WebKit Review Bot]

Comment on attachment 120888 [details]
Patch

Clearing flags on attachment: 120888

Committed r103934: <http://trac.webkit.org/changeset/103934>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

This change seems to create a performance regression for Chromium, see:

http://build.chromium.org/p/chromium.perf/builders/Mac10.5%20Perf%281%29/builds/12953

The increase of runtime is coming from WebKit changes between 103933 and 103936, which seems to be coming from this change. Any ideas why this happens?

[Alexis Menard (darktears)]

(In reply to comment #7)
> This change seems to create a performance regression for Chromium, see:
> 
> http://build.chromium.org/p/chromium.perf/builders/Mac10.5%20Perf%281%29/builds/12953
> 
> The increase of runtime is coming from WebKit changes between 103933 and 103936, which seems to be coming from this change. Any ideas why this happens?

What benchmark is regressing/where is this benchmark? This patch adds a new feature, a new code path was added so I'm wondering how a bench could hit that one (but maybe I'm wrong).

[Hin-Chung Lam]

(In reply to comment #8)
> (In reply to comment #7)
> > This change seems to create a performance regression for Chromium, see:
> > 
> > http://build.chromium.org/p/chromium.perf/builders/Mac10.5%20Perf%281%29/builds/12953
> > 
> > The increase of runtime is coming from WebKit changes between 103933 and 103936, which seems to be coming from this change. Any ideas why this happens?
> 
> What benchmark is regressing/where is this benchmark? This patch adds a new feature, a new code path was added so I'm wondering how a bench could hit that one (but maybe I'm wrong).

The benchmark runs through several sites, in particular only www.techcrunch.com in the test uses getPropertyValue. Does call getPropertyValue('outline') goes through this code path?

[Alexis Menard (darktears)]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > This change seems to create a performance regression for Chromium, see:
> > > 
> > > http://build.chromium.org/p/chromium.perf/builders/Mac10.5%20Perf%281%29/builds/12953
> > > 
> > > The increase of runtime is coming from WebKit changes between 103933 and 103936, which seems to be coming from this change. Any ideas why this happens?
> > 
> > What benchmark is regressing/where is this benchmark? This patch adds a new feature, a new code path was added so I'm wondering how a bench could hit that one (but maybe I'm wrong).
> 
> The benchmark runs through several sites, in particular only www.techcrunch.com in the test uses getPropertyValue. Does call getPropertyValue('outline') goes through this code path?

Yes we implemented it finally. So before the patch getPropertyValue('outline') would early return and return nothing, versus now it actually does stuff and return the correct value. So that explain that it is slower because we finally do something. Now the bench says it is slower but what is the magnitude? To me it sounds like a false alarm unless getting the outline property takes seconds.

[Hin-Chung Lam]

Thanks for your info! Looks like this is a false alarm, revert WebKit merge didn't help the situation.