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.
rdar://problem/58858225
(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?
(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.
Created attachment 389111 [details] Patch
(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 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()
Created attachment 389238 [details] Patch for landing
(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 on attachment 389238 [details] Patch for landing Clearing flags on attachment: 389238 Committed r255420: <https://trac.webkit.org/changeset/255420>
All reviewed patches have been landed. Closing bug.
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.
(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.