Bug 159373

Summary: "image-src" support is missing. We only support "-webkit-image-src"
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: CSSAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: 50167214, bfulgham, ddkilzer, dino, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=159367
Attachments:
Description Flags
Test case showing the null image
none
Patch
dino: review+
CSSParser.diff none

Description Brent Fulgham 2016-07-01 15:15:46 PDT
We only support the prefixed version of "image-src" (i.e., "-webkit-image-src").

We should support the unprefixed variant of this CSS.
Comment 1 Brent Fulgham 2016-07-01 15:18:41 PDT
Created attachment 282590 [details]
Test case showing the null image

Test case showing "image-src(...)" is not supported
Comment 2 Radar WebKit Bug Importer 2016-07-01 15:19:19 PDT
<rdar://problem/27140443>
Comment 3 Brent Fulgham 2016-07-01 16:52:41 PDT
Created attachment 282608 [details]
Patch
Comment 4 Dean Jackson 2016-07-01 16:57:04 PDT
Comment on attachment 282608 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:2096
> +            } else if (value->unit == CSSParserValue::Function && (equalLettersIgnoringASCIICase(value->function->name, "image-set(")
> +                || equalLettersIgnoringASCIICase(value->function->name, "-webkit-image-set("))) {

I did this:

+static bool isImageSetFunctionValue(const CSSParserValue& value)
+{
+    return value.unit == CSSParserValue::Function && (equalLettersIgnoringASCIICase(value.function->name, "image-set(") || equalLettersIgnoringASCIICase(value.function->name, "-webkit-image-set("));
+}
+

And then used it throughout CSSParser.cpp

> LayoutTests/fast/css/script-tests/image-set-setting.js:18
> -    var fullRule = "-webkit-image-set(" + rule + ")";
> +    var fullRule = "image-set(" + rule + ")";

I think we should have this test both forms.
Comment 5 Dean Jackson 2016-07-01 16:58:24 PDT
Created attachment 282609 [details]
CSSParser.diff
Comment 6 Brent Fulgham 2016-07-01 16:59:55 PDT
Comment on attachment 282608 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:2096
>> +                || equalLettersIgnoringASCIICase(value->function->name, "-webkit-image-set("))) {
> 
> I did this:
> 
> +static bool isImageSetFunctionValue(const CSSParserValue& value)
> +{
> +    return value.unit == CSSParserValue::Function && (equalLettersIgnoringASCIICase(value.function->name, "image-set(") || equalLettersIgnoringASCIICase(value.function->name, "-webkit-image-set("));
> +}
> +
> 
> And then used it throughout CSSParser.cpp

That's much better.

>> LayoutTests/fast/css/script-tests/image-set-setting.js:18
>> +    var fullRule = "image-set(" + rule + ")";
> 
> I think we should have this test both forms.

I tried that initially, but the behavior of 'cssText' causes trouble.

If I create a rule using "-webkit-image-set" it round-trips as "image-set", which breaks the test.
Comment 7 Dean Jackson 2016-07-01 17:23:21 PDT
Committed r202765: <http://trac.webkit.org/changeset/202765>
Comment 8 David Kilzer (:ddkilzer) 2016-07-03 05:01:42 PDT
(In reply to comment #7)
> Committed r202765: <http://trac.webkit.org/changeset/202765>

Layout test results fix for iOS Simulator:

Committed r202786: <https://trac.webkit.org/changeset/202786>
Comment 9 yisibl 2016-08-15 01:04:08 PDT
Please support the standard syntax: 

Each <string> inside image-set() represents a <url>, just like in image().

image-set( "foo.png" 1x, "foo-2x.png" 2x, "foo-print.png" 600dpi );
Comment 10 Brent Fulgham 2016-08-16 10:34:54 PDT
(In reply to comment #9)
> Please support the standard syntax: 
> 
> Each <string> inside image-set() represents a <url>, just like in image().
> 
> image-set( "foo.png" 1x, "foo-2x.png" 2x, "foo-print.png" 600dpi );

Could you please file a new Bug for this issue so we can track it more easily, since this bug is already closed?

Thanks!