Bug 82261 - Increase code sharing between CSSComputedStyleDeclaration and CSSPropertyLonghand.
Summary: Increase code sharing between CSSComputedStyleDeclaration and CSSPropertyLong...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-26 16:45 PDT by Alexis Menard (darktears)
Modified: 2012-03-27 05:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.00 KB, patch)
2012-03-26 16:48 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (9.79 MB, application/zip)
2012-03-26 21:19 PDT, WebKit Review Bot
no flags Details
Patch for landing (14.81 KB, patch)
2012-03-27 04:19 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-03-26 16:45:00 PDT
Increase code sharing between CSSComputedStyleDeclaration and CSSPropertyLonghand.
Comment 1 Alexis Menard (darktears) 2012-03-26 16:48:14 PDT
Created attachment 133929 [details]
Patch
Comment 2 Ryosuke Niwa 2012-03-26 17:41:27 PDT
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.
Comment 3 Alexis Menard (darktears) 2012-03-26 17:45:30 PDT
(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.
Comment 4 Ryosuke Niwa 2012-03-26 18:45:58 PDT
(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?
Comment 5 Alexis Menard (darktears) 2012-03-26 18:53:40 PDT
(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.
Comment 6 Ryosuke Niwa 2012-03-26 19:05:14 PDT
(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?
Comment 7 Alexis Menard (darktears) 2012-03-26 19:38:43 PDT
(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...
Comment 8 Ryosuke Niwa 2012-03-26 20:05:07 PDT
(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?
Comment 9 Alexis Menard (darktears) 2012-03-26 20:13:41 PDT
(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.
Comment 10 Ryosuke Niwa 2012-03-26 20:15:06 PDT
Comment on attachment 133929 [details]
Patch

Okay.
Comment 11 WebKit Review Bot 2012-03-26 21:19:18 PDT
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
Comment 12 WebKit Review Bot 2012-03-26 21:19:25 PDT
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
Comment 13 Alexis Menard (darktears) 2012-03-27 04:19:56 PDT
Created attachment 134019 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-03-27 05:01:11 PDT
Comment on attachment 134019 [details]
Patch for landing

Clearing flags on attachment: 134019

Committed r112254: <http://trac.webkit.org/changeset/112254>
Comment 15 WebKit Review Bot 2012-03-27 05:01:41 PDT
All reviewed patches have been landed.  Closing bug.