Bug 82261 – Increase code sharing between CSSComputedStyleDeclaration and CSSPropertyLonghand.

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

[Alexis Menard (darktears)]

Increase code sharing between CSSComputedStyleDeclaration and CSSPropertyLonghand.

[Alexis Menard (darktears)]

Created attachment 133929 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 133929 [details]
Patch

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

> Source/WebCore/css/CSSPropertyLonghand.cpp:225
> -        CSSPropertyOutlineWidth,
> -        CSSPropertyOutlineStyle,
>          CSSPropertyOutlineColor,
> -        CSSPropertyOutlineOffset
> +        CSSPropertyOutlineStyle,
> +        CSSPropertyOutlineWidth

This change doesn't look right. border property's values are width, style, and then color, not color, style, then width unless you're traversing backwards.

[Alexis Menard (darktears)]

(In reply to comment #2)
> (From update of attachment 133929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133929&action=review
> 
> > Source/WebCore/css/CSSPropertyLonghand.cpp:225
> > -        CSSPropertyOutlineWidth,
> > -        CSSPropertyOutlineStyle,
> >          CSSPropertyOutlineColor,
> > -        CSSPropertyOutlineOffset
> > +        CSSPropertyOutlineStyle,
> > +        CSSPropertyOutlineWidth
> 
> This change doesn't look right. border property's values are width, style, and then color, not color, style, then width unless you're traversing backwards.


http://www.w3.org/TR/css3-ui/#outline says :

[ <‘outline-color’> || <‘outline-style’> || <‘outline-width’> ] | inherit

The order is not enforced for sure but we should follow the other browsers and they return that order when possible.

[Ryosuke Niwa]

(In reply to comment #3)
> http://www.w3.org/TR/css3-ui/#outline says :
> 
> [ <‘outline-color’> || <‘outline-style’> || <‘outline-width’> ] | inherit
> 
> The order is not enforced for sure but we should follow the other browsers and they return that order when possible.

Huh, they use a different ordering huh? That's annoying. Can we give a feedback to the CSS WG to match border?

[Alexis Menard (darktears)]

(In reply to comment #4)
> (In reply to comment #3)
> > http://www.w3.org/TR/css3-ui/#outline says :
> > 
> > [ <‘outline-color’> || <‘outline-style’> || <‘outline-width’> ] | inherit
> > 
> > The order is not enforced for sure but we should follow the other browsers and they return that order when possible.
> 
> Huh, they use a different ordering huh? That's annoying. Can we give a feedback to the CSS WG to match border?

Has been like this for a while now. Nobody should rely on the order though but I'm not sure if it will break real use case out there.

[Ryosuke Niwa]

(In reply to comment #5)
> Has been like this for a while now. Nobody should rely on the order though but I'm not sure if it will break real use case out there.

What do other user agents do? Do they follow the spec? Or do they generate in the same order WebKit currently does?

[Alexis Menard (darktears)]

(In reply to comment #6)
> (In reply to comment #5)
> > Has been like this for a while now. Nobody should rely on the order though but I'm not sure if it will break real use case out there.
> 
> What do other user agents do? Do they follow the spec? Or do they generate in the same order WebKit currently does?

When I wrote the patches for shorthand support in CSSComputedStyleDeclaration I tried to make sure we were matching FF and Opera so http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html expect that...

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Has been like this for a while now. Nobody should rely on the order though but I'm not sure if it will break real use case out there.
> > 
> > What do other user agents do? Do they follow the spec? Or do they generate in the same order WebKit currently does?
> 
> When I wrote the patches for shorthand support in CSSComputedStyleDeclaration I tried to make sure we were matching FF and Opera so http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html expect that...

Do they match?

[Alexis Menard (darktears)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Has been like this for a while now. Nobody should rely on the order though but I'm not sure if it will break real use case out there.
> > > 
> > > What do other user agents do? Do they follow the spec? Or do they generate in the same order WebKit currently does?
> > 
> > When I wrote the patches for shorthand support in CSSComputedStyleDeclaration I tried to make sure we were matching FF and Opera so http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-outline-shorthand.html expect that...
> 
> Do they match?

FF11 doesn't implement it but Opera returns the same order (modulo the style differences) when I load the test case.

FAIL computedStyle.getPropertyValue('outline') should be rgb(255, 0, 0) solid 5px. Was rgb(255, 0, 0) solid 6px.

[Ryosuke Niwa]

Comment on attachment 133929 [details]
Patch

Okay.

[WebKit Review Bot]

Comment on attachment 133929 [details]
Patch

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

New failing tests:
fast/css/cssText-shorthand.html

[WebKit Review Bot]

Created attachment 133970 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexis Menard (darktears)]

Created attachment 134019 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 134019 [details]
Patch for landing

Clearing flags on attachment: 134019

Committed r112254: <http://trac.webkit.org/changeset/112254>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.