image-set doesn't round-trip properly with cssText

• Component: CSS
• Status: RESOLVED FIXED

[Rick Byers]

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() + ")";

[Glenn Adams]

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).

[Rick Byers]

(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).

[Glenn Adams]

(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.

[Rick Byers]

(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.

[Glenn Adams]

(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]

[Rick Byers]

Without this fix, my tests in issue 99493 will look funny.  So I'll fix this first.

[Rick Byers]

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.

[Rick Byers]

Created attachment 170206 [details]
Patch

[Build Bot]

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

[WebKit Review Bot]

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

[Rick Byers]

Created attachment 170232 [details]
Update existing test I missed rather than create a new one

[Rick Byers]

Comment on attachment 170232 [details]
Update existing test I missed rather than create a new one

Thanks!

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.