Bug 99725 - image-set doesn't round-trip properly with cssText
Summary: image-set doesn't round-trip properly with cssText
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rick Byers
URL:
Keywords: WebExposed
Depends on:
Blocks: 99493
  Show dependency treegraph
 
Reported: 2012-10-18 08:55 PDT by Rick Byers
Modified: 2012-11-16 22:51 PST (History)
9 users (show)

See Also:


Attachments
Patch (9.71 KB, patch)
2012-10-23 12:46 PDT, Rick Byers
no flags Details | Formatted Diff | Diff
Update existing test I missed rather than create a new one (9.89 KB, patch)
2012-10-23 14:03 PDT, Rick Byers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 2012-10-18 08:55:01 PDT
Given a rule like:
background-image: -webkit-image-set(url(foo) 1x, url(bar) 2x);
Calling cssText on this rule gives the malformed text:
background-image: -webkit-image-set(url(foo), 1, url(bar), 2);

This is due to this code in CSSImageSetValue::customCssText:
    return "-webkit-image-set(" + CSSValueList::customCssText() + ")";
Comment 1 Glenn Adams 2012-10-18 17:44:38 PDT
FYI, neither CSS OM or DOM-2 Style specifies (at present) what cssText (of a specified style, i.e., elt.style) should be if the declaration encounters a parse error. In fact, whether a rule should be instantiated in the OM in this case is similarly unspecified. So any WK behavior in this regard should be considered experimental and WK specific (at least as far as the specs are currently written).
Comment 2 Rick Byers 2012-10-19 06:31:19 PDT
(In reply to comment #1)
> FYI, neither CSS OM or DOM-2 Style specifies (at present) what cssText (of a specified style, i.e., elt.style) should be if the declaration encounters a parse error. In fact, whether a rule should be instantiated in the OM in this case is similarly unspecified. So any WK behavior in this regard should be considered experimental and WK specific (at least as far as the specs are currently written).

Makes sense.  But I'm talking only about valid CSS here (per http://dev.w3.org/csswg/css4-images/#image-set-notation).
Comment 3 Glenn Adams 2012-10-19 08:42:26 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > FYI, neither CSS OM or DOM-2 Style specifies (at present) what cssText (of a specified style, i.e., elt.style) should be if the declaration encounters a parse error. In fact, whether a rule should be instantiated in the OM in this case is similarly unspecified. So any WK behavior in this regard should be considered experimental and WK specific (at least as far as the specs are currently written).
> 
> Makes sense.  But I'm talking only about valid CSS here (per http://dev.w3.org/csswg/css4-images/#image-set-notation).

Ah, right, well, not entirely valid, since 'x' is not a a defined unit identifier, while 'dppx' is. If we're following the spec, 'dppx' probably should be given preference over 'x' when serializing, at least until the CSSWG admits 'x' as a legitimate synonym for 'dppx'.

In any case, what cssText should return for a specified (inline) style is rather ambiguous, since it isn't well specified as to whether any canonicalization should occur or not when serializing this property value, i.e., when serializing an <image-set>.

But I agree it is a bug to return a unit-less dimension.
Comment 4 Rick Byers 2012-10-19 08:56:03 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > FYI, neither CSS OM or DOM-2 Style specifies (at present) what cssText (of a specified style, i.e., elt.style) should be if the declaration encounters a parse error. In fact, whether a rule should be instantiated in the OM in this case is similarly unspecified. So any WK behavior in this regard should be considered experimental and WK specific (at least as far as the specs are currently written).
> > 
> > Makes sense.  But I'm talking only about valid CSS here (per http://dev.w3.org/csswg/css4-images/#image-set-notation).
> 
> Ah, right, well, not entirely valid, since 'x' is not a a defined unit identifier, while 'dppx' is. If we're following the spec, 'dppx' probably should be given preference over 'x' when serializing, at least until the CSSWG admits 'x' as a legitimate synonym for 'dppx'.
> 
> In any case, what cssText should return for a specified (inline) style is rather ambiguous, since it isn't well specified as to whether any canonicalization should occur or not when serializing this property value, i.e., when serializing an <image-set>.
> 
> But I agree it is a bug to return a unit-less dimension.

Note also the extra ',' between the url and scale factor which makes the string not parse at all.

I assume that at a minimum, we can agree that it's always a bug whenever a string that comes out of cssText causes a parse error (i.e. is ignored) when written back to cssText.
Comment 5 Glenn Adams 2012-10-19 09:21:51 PDT
(In reply to comment #4)
> (In reply to comment #3)
> Note also the extra ',' between the url and scale factor which makes the string not parse at all.

it looks like there is an error in the current syntax definition in css4-images:

<image-set> = image-set( [ <image-set-decl>, ]* [ <image-set-decl> | <color>] )
<image-set-decl> = [ <image> | <string> ] <resolution>

i suspect this should read

<image-set-decl> = ( <image> | <string> ) <resolution>

otherwise, an entry in an image set could be simply a resolution and nothing else

or if the spec intends that a resolution by itself is legal, e.g., as a shorthand for repeating the previous image-set-decl that does contain an <image>|<string> but with a different resolution, then it should say so (it doesn't)

if a bare <resolution> is permitted, then the output

url(foo), 1dppx, url(bar), 2dppx

wouldn't be illegal syntax, but certainly not what is expected given an input of

url(foo) 1dppx, url(bar) 2dppx

or its 'x' equivalent

> I assume that at a minimum, we can agree that it's always a bug whenever a string that comes out of cssText causes a parse error (i.e. is ignored) when written back to cssText.

that was previously my position as well; however, some recent input i have on different browser behavior in regards to the retention of original (but at least partially bad) input text in the case of specified (inline) styles has me wondering, and in need of some tests to convince myself that is the consensus among implementers

in any case, i concur that an input of

url(foo) 1dppx, url(bar) 2dppx

or

url(foo) 1x, url(bar) 2x (if this variant unit is supported)

should serialize as

url(foo) 1dppx, url(bar) 2dppx

[i prefer the standard unit to be used for serialization purposes]
Comment 6 Rick Byers 2012-10-23 06:48:02 PDT
Without this fix, my tests in issue 99493 will look funny.  So I'll fix this first.
Comment 7 Rick Byers 2012-10-23 07:07:12 PDT
I've filed bug 100120 to track the separate issue of handling of other units than 'x'.  I'd like to keep this bug focused on fixing just cssText so we can round-trip the existing parser behavior.
Comment 8 Rick Byers 2012-10-23 12:46:48 PDT
Created attachment 170206 [details]
Patch
Comment 9 Build Bot 2012-10-23 13:39:06 PDT
Comment on attachment 170206 [details]
Patch

Attachment 170206 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14490945

New failing tests:
fast/css/image-set-setting.html
Comment 10 WebKit Review Bot 2012-10-23 13:59:11 PDT
Comment on attachment 170206 [details]
Patch

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

New failing tests:
fast/css/image-set-setting.html
Comment 11 Rick Byers 2012-10-23 14:03:42 PDT
Created attachment 170232 [details]
Update existing test I missed rather than create a new one
Comment 12 Rick Byers 2012-10-24 12:42:17 PDT
Comment on attachment 170232 [details]
Update existing test I missed rather than create a new one

Thanks!
Comment 13 WebKit Review Bot 2012-10-24 13:18:49 PDT
Comment on attachment 170232 [details]
Update existing test I missed rather than create a new one

Clearing flags on attachment: 170232

Committed r132388: <http://trac.webkit.org/changeset/132388>
Comment 14 WebKit Review Bot 2012-10-24 13:18:53 PDT
All reviewed patches have been landed.  Closing bug.