Bug 206909

Summary: REGRESSION (r254406): Gmail.com star/favorite icons are not rendering
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ImagesAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, mjs, noam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=160934
https://bugs.webkit.org/show_bug.cgi?id=229909
https://bugs.webkit.org/show_bug.cgi?id=231078
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Simon Fraser (smfr) 2020-01-28 14:44:28 PST
r254406 broke the icons in the "star" column in Gmail.

The ::before has:
content: -webkit-image-set('' 1x,'' 2x);

It seems this was previously invalid, so that content:'' was used. But now it's treated as valid, so we show the missing image icon.

I think we need to preserve the old behavior of image-set in -webkit-image-set.
Comment 1 Simon Fraser (smfr) 2020-01-28 14:44:45 PST
rdar://problem/58858225
Comment 2 Noam Rosenthal 2020-01-28 22:42:04 PST
(In reply to Simon Fraser (smfr) from comment #0)
> r254406 broke the icons in the "star" column in Gmail.
> 
> The ::before has:
> content: -webkit-image-set('' 1x,'' 2x);
> 
> It seems this was previously invalid, so that content:'' was used. But now
> it's treated as valid, so we show the missing image icon.
> 
> I think we need to preserve the old behavior of image-set in
> -webkit-image-set.

or maybe disallow empty images?
Comment 3 Noam Rosenthal 2020-01-28 22:56:40 PST
(In reply to Noam Rosenthal from comment #2)
> (In reply to Simon Fraser (smfr) from comment #0)
> > r254406 broke the icons in the "star" column in Gmail.
> > 
> > The ::before has:
> > content: -webkit-image-set('' 1x,'' 2x);
> > 
> > It seems this was previously invalid, so that content:'' was used. But now
> > it's treated as valid, so we show the missing image icon.
> > 
> > I think we need to preserve the old behavior of image-set in
> > -webkit-image-set.
> 
> or maybe disallow empty images?
I think I'll do both for now.
Comment 4 Noam Rosenthal 2020-01-29 00:52:37 PST
Created attachment 389111 [details]
Patch
Comment 5 Simon Fraser (smfr) 2020-01-29 10:51:48 PST
(In reply to Noam Rosenthal from comment #2)
> (In reply to Simon Fraser (smfr) from comment #0)
> > r254406 broke the icons in the "star" column in Gmail.
> > 
> > The ::before has:
> > content: -webkit-image-set('' 1x,'' 2x);
> > 
> > It seems this was previously invalid, so that content:'' was used. But now
> > it's treated as valid, so we show the missing image icon.
> > 
> > I think we need to preserve the old behavior of image-set in
> > -webkit-image-set.
> 
> or maybe disallow empty images?

Are empty images allowed per spec?
Comment 6 Simon Fraser (smfr) 2020-01-29 10:55:43 PST
Comment on attachment 389111 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1706
> +    if ((range.peek().type() == StringToken) && (allowedImageTypes & AllowedImageType::RawStringAsURL)) {

I think we prefer allowedImageTypes.contains(AllowedImageType::RawStringAsURL)

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1714
>          CSSValueID id = range.peek().functionId();

Not this patch, but please don't use 'id'; it's a reserved word in Objective-C.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1715
> +        if ((allowedImageTypes & AllowedImageType::GeneratedImage) && isGeneratedImage(id))

.contains()

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1718
> +        if (allowedImageTypes & AllowedImageType::ImageSet) {

.contains()

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1726
> +    if (allowedImageTypes & AllowedImageType::URLFunction) {

.contains()
Comment 7 Noam Rosenthal 2020-01-30 04:27:51 PST
Created attachment 389238 [details]
Patch for landing
Comment 8 Noam Rosenthal 2020-01-30 04:28:40 PST
(In reply to Simon Fraser (smfr) from comment #5)
> (In reply to Noam Rosenthal from comment #2)
> > (In reply to Simon Fraser (smfr) from comment #0)
> > > r254406 broke the icons in the "star" column in Gmail.
> > > 
> > > The ::before has:
> > > content: -webkit-image-set('' 1x,'' 2x);
> > > 
> > > It seems this was previously invalid, so that content:'' was used. But now
> > > it's treated as valid, so we show the missing image icon.
> > > 
> > > I think we need to preserve the old behavior of image-set in
> > > -webkit-image-set.
> > 
> > or maybe disallow empty images?
> 
> Are empty images allowed per spec?

They actually are, considered empty URLs. Removed that part from the patch.
Comment 9 WebKit Commit Bot 2020-01-30 05:10:12 PST
Comment on attachment 389238 [details]
Patch for landing

Clearing flags on attachment: 389238

Committed r255420: <https://trac.webkit.org/changeset/255420>
Comment 10 WebKit Commit Bot 2020-01-30 05:10:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2020-02-01 16:35:55 PST
Did we check the behavior of other browsers in image-set? It would be unfortunate if we were the only one who matched the spec on this.
Comment 12 Noam Rosenthal 2020-02-02 01:18:28 PST
(In reply to Darin Adler from comment #11)
> Did we check the behavior of other browsers in image-set? It would be
> unfortunate if we were the only one who matched the spec on this.

Of course, working on other browsers in parallel, currently getting the W3C tests around this to be comprehensive.