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

Is it okay to just alias -webkit-mask-composite to mask-composite? #153

Open
saschanaz opened this issue Aug 1, 2021 · 17 comments
Open

Comments

@saschanaz
Copy link
Member

saschanaz commented Aug 1, 2021

d1ac0cf just added all the mask things as aliases, but per MDN the syntax of this specific one is quite different. Why is it safe to just alias it?

@miketaylr
Copy link
Member

It could just be a bug due to me not understanding it well.

@miketaylr
Copy link
Member

See also:

https://searchfox.org/mozilla-central/rev/4b88e0b8cca115009e82fdd65e5bf5812ff99128/devtools/shared/css/generated/properties-db.js#2110-2126

  "-webkit-mask-composite": {
    "isInherited": false,
    "subproperties": [
      "mask-composite"
    ],
    "supports": [],
    "values": [
      "add",
      "exclude",
      "inherit",
      "initial",
      "intersect",
      "revert",
      "subtract",
      "unset"
    ]
  },

@miketaylr
Copy link
Member

Looking at https://www.w3.org/TR/css-masking-1/#the-mask-composite

<compositing-operator> = add | subtract | intersect | exclude

That seems to match what is in properties-db.js. So yeah, this isn't a "simple alias", but an alias that only supports the subset of values that match the unprefixed values.

@saschanaz
Copy link
Member Author

saschanaz commented Aug 2, 2021

It seems that list conflicts with webkit and blink implementations, it seems they don't overlap and MDN is right about it?

  '-webkit-mask-composite': {
    values: [
      'clear',
      'copy',
      'source-over',
      'source-in',
      'source-out',
      'source-atop',
      'destination-over',
      'destination-in',
      'destination-out',
      'destination-atop',
      'xor',
      'plus-lighter',
    ],
  },

@miketaylr
Copy link
Member

We could do one of two things: document what Gecko does, or keep this as a simple alias. I suspect it's not worth the effort for Gecko to implement all the values, without any evidence that this breaks sites. What do you think @saschanaz?

@saschanaz
Copy link
Member Author

document what Gecko does, or keep this as a simple alias

I think those two options are same given that Gecko implements it as a simple alias. There at least should be a note that the aliased -webkit-mask-composite is incompatible with webkit/blink implementation (although then I'm not sure why the alias should even exist).

@miketaylr
Copy link
Member

I just noticed simple alias isn't even defined, and we should fix that.

There at least should be a note that the aliased -webkit-mask-composite is incompatible with webkit/blink implementation (although then I'm not sure why the alias should even exist).

I think this largely depends on what values are important for web compatibility. If no important sites use destination-atop, but they all use add, then it seems useful to have. But if Gecko wants to unship it, we can remove it from the spec.

@saschanaz
Copy link
Member Author

saschanaz commented Aug 2, 2021

My comments are largely based on the MDN data and spec, for actual implementation I guess @emilio might have an opinion.

@domenic
Copy link
Member

domenic commented Aug 2, 2021

We could do one of two things: document what Gecko does, or keep this as a simple alias

Shouldn't we try to get all three browsers to converge on a single interoperable behavior, and have the spec reflect whatever that is?

@miketaylr
Copy link
Member

Shouldn't we try to get all three browsers to converge on a single interoperable behavior, and have the spec reflect whatever that is?

Yeah, that's ideal. Unfortunately I don't recall the circumstances surrounding the addition of -webkit-mask-composite - if Gecko had bugs requiring it, or it was initially added by EdgeHTML and Gecko followed. (note: there is some history at #14).

We could do the research to determine which values are important for compat, beyond add, substract, intersect, exclude. As of right now, those are the interoperable values between webkit, gecko, and blink. In theory if the others aren't used, or usage is low-enough to justify removal from the other engines, that would be a nice outcome.

(I guess that's the entire point of this issue being opened and I was too quick to reply. 🙈 )

@saschanaz
Copy link
Member Author

We could do the research to determine which values are important for compat, beyond add, substract, intersect, exclude. As of right now, those are the interoperable values between webkit, gecko, and blink.

Really? CSS.supports("-webkit-mask-composite: add") returns false on Chrome but true on Firefox. I think the implementations of Firefox and Chrome are totally incompatible with each other.

@emilio
Copy link

emilio commented Aug 2, 2021

Yeah, so what we implement is just an alias for mask-composite: https://searchfox.org/mozilla-central/rev/7b1de5e29d878cc163dec7beaf9b57a2f0f41aaa/servo/components/style/properties/longhands/svg.mako.rs#169

Blaming it, it goes back to the original implementation of the property in https://bugzilla.mozilla.org/show_bug.cgi?id=686281: https://hg.mozilla.org/mozilla-central/rev/f6eba571ce59.

So this is a bit messy. It was probably just added for consistency with other mask aliases which do share syntax per https://bugzilla.mozilla.org/show_bug.cgi?id=686281#c167.

cc @dbaron in case he remembers more context.

@dbaron
Copy link
Member

dbaron commented Aug 2, 2021

I don't remember, other than that that bug went through quite a few cycles of review and it was hard to keep track of at the time. I don't think I was aware there was a syntax difference.

@gsnedders
Copy link
Member

It seems that list conflicts with webkit and blink implementations, it seems they don't overlap and MDN is right about it?

Indeed. If I'm understanding correctly:

-webkit-mask-composite mask-composite
clear
copy
source-over add
source-in intersect
source-out subtract
source-atop
destination-over
destination-in
destination-out
destination-atop
xor exclude
plus-lighter

w3c/fxtf-drafts@bb5f277 seems to be the spec change which changed away from what Blink/WebKit implement, though it doesn't link to any resolution or issues, hence I don't know how broad support for the simplification was at the time.

Note that Blink & WebKit only support the prefixed property, so even if the totally different Gecko implementation is some level of evidence that sites don't rely on the prefixed property, it may only be because they additionally support the unprefixed property.

On the face of it, the options are:

  1. Define -webkit-mask-composite as <composite-mode>, per WebKit and Blink today, potentially ensuring something is defined as to how this is combined with the mask-composite property in the cascade.
  2. Drop -webkit-mask-composite entirely (though for both WebKit and Blink this is presumably blocked on supporting mask-composite), given Gecko's implementation is okay for web compat seemingly.
  3. Define -webkit-mask-composite as a legacy shorthand with only four values (which depends on WebKit and Blink being okay with dropping the rest).

@karlcow
Copy link
Member

karlcow commented Aug 3, 2021

Diving in my mail archives.

Log

September 2011

January 2016

February 2016

October 2016

Blink implemented the no-repeat (-webkit-)mask-repeat default value in https://bugs.chromium.org/p/chromium/issues/detail?id=628968, but it was reverted according to comment 11 of that bug due to https://bugs.chromium.org/p/chromium/issues/detail?id=645000. in https://bugzilla.mozilla.org/show_bug.cgi?id=1305697#c5

Unrelated

🚨 The important thing probably is that most of the webcompat issues I could browse, find about mask were NOT about mask-composite.

@karlcow
Copy link
Member

karlcow commented Aug 3, 2021

Current usage as defined by chrome is 0.186841%
https://www.chromestatus.com/metrics/css/popularity#webkit-mask-composite

it would be interesting to have the stats for the values in fact.
https://github.com/search?l=CSS&q=%22-webkit-mask-composite%22&type=Code

not a lot of matches but some patterns (⭐ most common)

  • destination-in
  • destination-out
  • exclude
  • intersect
  • source-in
  • source-out
  • source-over
  • unset
  • xor

Examples:

exclude as a fallback for unset

https://github.com/Ejiro-Oke/To-Do-App/blob/577639e17ba9048eacf97838eeabb57c2dc3ebde/ToDo.css#L116-L129

exclude as a fallback for source-out

https://github.com/VincEnterprise/wavio/blob/be111d73a6bfbb86026bff95985156bfef654e26/src/styles/main.css#L143-L144

exclude as a fallback for destination-out

https://github.com/mailkaze/portafolio-leonidas/blob/be958c0dcd6fe541d985c55e9bd4d708253395b8/css/testimonials.css#L28-L32

intersect as a fallback for destination-in

https://github.com/NgTheLuan/Job-Listing/blob/faf4bafe5fa433f1fd66efd64f85705bf3dc0a45/client/src/features/LoadingImg/styles.css#L9-L13

intersect as a fallback for source-in

https://github.com/MatiasLN/whisky/blob/407f7414a25634ed806b8c26bce18ee423a10a4d/src/comps/Map/Map.css#L62-L63

add as a fallback for add

https://github.com/u4i-admin2/IPA/blob/5ca3b26d7414c6a32626a6192345efd158f81128/css-modules/general.module.css#L33-L34

Weird

https://github.com/isratkorobi/Css-Hover-Effect/blob/caac58474fe49f14afc74b99a4155681db98a303/style.css#L35-L36

https://github.com/biglinux/big-store/blob/8490b80203e8626764cb965ba6e45c45c3aee034/big-store/usr/share/bigbashview/bcc/apps/big-store/css/appstream.css#L793

no fallback

https://github.com/boo-boom/web-issue/blob/f9e5b7751c745ab2caf489b976930a4fdc19a0e6/%E5%AE%9E%E4%BE%8B/23-CSS%E5%AE%9E%E7%8E%B0%E4%BC%98%E6%83%A0%E5%88%B8/style.css#L61-L68

@miketaylr
Copy link
Member

Thanks for the digging, @karlcow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants