Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-images] Consider making -webkit-image-set a parse-time alias to image-set #6285

Closed
emilio opened this issue May 12, 2021 · 12 comments
Closed
Assignees

Comments

@emilio
Copy link
Collaborator

emilio commented May 12, 2021

Per the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1709415 it seems -webkit-image-set may be used enough on the wild that supporting it as a parse-time alias would be a low effort compat win.

If my reading of the WebKit code is right, WebKit supports the prefixed version with an slightly stricter syntax, but they serialize to the modern syntax. I think it'd be good if they just supported the same syntax, since the modern syntax is an extension of the webkit one, and it's less maintenance.

cc @smfr.

@emilio
Copy link
Collaborator Author

emilio commented May 12, 2021

See also whatwg/compat#144. CC @schenney-chromium @foolip

@emilio
Copy link
Collaborator Author

emilio commented May 12, 2021

Yeah, confirmed that WebKit serializes -webkit-image-set(url('foo.png') 1x, url('bar.png') 2x) to the modern form.

@foolip
Copy link
Member

foolip commented May 12, 2021

An alias would be great if it's web compatible. Is the prefixed form more limited in what syntax it accepts? If so, if (almost) all allowed syntax for the unprefixed form work with the prefixed one, then this should be pretty safe.

Or is it the other way around?

@emilio
Copy link
Collaborator Author

emilio commented May 12, 2021

Yeah, prefixed syntax is more limited, and a strict subset of the modern one.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 12, 2021
The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1709415
gecko-commit: 01bca848cb09b426c0c1c5f34d32dbe3fa5b9aa7
gecko-reviewers: karlcow, twisniewski
@foolip
Copy link
Member

foolip commented May 12, 2021

That's great! I guess the only risk then is use of the prefixed variant that's currently rejected but would start working and have an unintended effect. It's not super common but not a theoretical problem. This could be answered with httparchive or use counters probably.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-images] Consider making -webkit-image-set a parse-time alias to image-set, and agreed to the following:

  • RESOLVED: Make -webkit-image-set a parse time alias to image-set
The full IRC log of that discussion <dael> Topic: [css-images] Consider making -webkit-image-set a parse-time alias to image-set
<dael> github: https://github.com//issues/6285
<dael> emilio: There's enough usage in the wild of -webkit-image-set it's worth supporting
<dael> emilio: Chromium engineers skeptical of getting rid of prefix. I think alias the 2 functions is easiest. webkit does some aliasing. -webkit-image-set serialized as unprefixed
<dael> emilio: I think right now wk limits the syntax for -webkit-image-set but they could just support whole thing. I think that's easiest way to spec it
<dael> TabAtkins: So long as it's reasonable to Chrome and WK people to accept full set I'm happy to put this in spec
<dael> smfr: Fine with that
<dael> rune: Without looking it much detail, sounds good
<dael> astearns: Prop: Make -webkit-image-set a parse time alias to image-set
<dael> astearns: Objections?
<dael> RESOLVED: Make -webkit-image-set a parse time alias to image-set
<dael> astearns: foolip mentions might be a problem with accepting full syntax, but can check
<dael> emilio: I think it's pretyt unlikely that [missed] but we can check
<dael> astearns: Might be possibility. Usage would be tiny and not sure side effect is awful

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 12, 2021
…e-set(). r=karlcow,twisniewski

The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 12, 2021
The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1709415
gecko-commit: 01bca848cb09b426c0c1c5f34d32dbe3fa5b9aa7
gecko-reviewers: karlcow, twisniewski
@tabatkins tabatkins self-assigned this May 13, 2021
@emilio
Copy link
Collaborator Author

emilio commented May 14, 2021

@tabatkins fyi tests for this are in web-platform-tests/wpt@6ea1045

@tabatkins
Copy link
Member

Oh nice, thanks!

@yisibl
Copy link
Contributor

yisibl commented Jul 16, 2021

I found that the implementation of -webkit-image-set() in Firefox is completely a standard image-set() mapping, and some new syntaxes are not supported in WebKit/Blink. This seems to be confusing. Does Firefox need to fully follow the existing syntax in WebKit/Blink?

For example, the following code is valid in Firefox 90 but invalid in WebKit/Blink.

.foo {
  background: -webkit-image-set('../img/image-1x.jpg' 1x, '../img/image-2x.jpg' 2x) /* WebKit/Blink invalid */
  background: -webkit-image-set('../img/image-1x.jpg' 1x, linear-gradient(black, white) 2x) /* WebKit/Blink invalid */
  background-image: -webkit-image-set('../img/image-1x.jpg' 1dppx, '../img/image-2x.jpg' 2dppx); /* WebKit/Blink invalid */
}

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1709415
Demo: https://hongkiat.github.io/css-image-set/

@emilio
Copy link
Collaborator Author

emilio commented Jul 16, 2021

Once Chrome and Webkit update to the new syntax and use the alias all browsers should agree here.

@emilio
Copy link
Collaborator Author

emilio commented Jul 16, 2021

I think this issue can be closed given we have a resolution, spec edits and tests, but lmk if you disagree.

@emilio emilio closed this as completed Jul 16, 2021
@gsnedders
Copy link
Contributor

That's great! I guess the only risk then is use of the prefixed variant that's currently rejected but would start working and have an unintended effect. It's not super common but not a theoretical problem.

WebKit did actually start treating the prefixed form identically when (unprefixed) image-set() was first implemented, but this was reverted in https://bugs.webkit.org/show_bug.cgi?id=206909 due to Gmail breakage. Arguably, though, the underlying cause of that is https://bugs.webkit.org/show_bug.cgi?id=231078 (invalid <image> renders as a broken image in WebKit).

Loirooriol added a commit to Loirooriol/servo that referenced this issue May 22, 2023
The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 24, 2023
The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912
Loirooriol pushed a commit to Loirooriol/servo that referenced this issue May 24, 2023
The webkit syntax is an strict subset of the modern one, so this should
be doable, and is the simplest.

If my reading of the WebKit code is correct it should also be the way
WebKit deals with this (except they restrict -webkit-image-set() syntax
artificially).

 * w3c/csswg-drafts#6285
 * whatwg/compat#144

Differential Revision: https://phabricator.services.mozilla.com/D114912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants