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

Add feComposite support for operator attribute's lighter value #9074

Merged
merged 13 commits into from
Mar 3, 2021

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Feb 12, 2021

FF86 introduces support for the "lighter" filter attribute - see bug 1518099.

The current BCD on feComposite is a bit of a mess - for example it says that this is already supported in FF and is not supported in Chrome, neither of which is true.

Bug info for main platforms here:

Following on from those bugs it is pretty clear that feComposite must be supported in Blink and Webkit derived browsers. Have marked as true. For Blink have marked as version version where Blink was supported.

Docs bug is mdn/content#1727

This may also affect SVGFECompositeElement but still checking.

@github-actions github-actions bot added the data:svg 🖌️ Compat data for SVG features. https://developer.mozilla.org/docs/Web/SVG label Feb 12, 2021
@@ -24,16 +24,16 @@
"version_added": null
},
"opera": {
"version_added": false
"version_added": true
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, these must all be true/supported because you can't have support for "lighter" if the parent item is not supported (as per bugs). I am not sure what versions.

If you believe chrome then this was in from release 1 (webkit and from then in Blink). Should I leave these alone or assume that support came in earliest matching version of Blink/Webkit for the other browsers? That would mean, for example, that Opera would change to version 15 and I'd assume there was no support in Presto. Thoughts?

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 16, 2021
@@ -51,40 +51,52 @@
"__compat": {
"support": {
"chrome": {
"version_added": "5"
"version_added": "5",
"notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)."
Copy link
Collaborator Author

@hamishwillee hamishwillee Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI added all these "BackgroundImage/BackgroundAlpha notes to mirror feComposite (which this API does). Only thing that supports it is Edge before v79

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts there:

  1. If I understand correctly, this note means that neither BackgroundImage nor BackgroundAlpha are accepted as values to the in1 or in2 attributes?
  2. Ordinarily, I'd say that when we repeat the same note again and again, it's often a sign that the information needs to be expressed as a subfeature or something else odd is going on. Except, in this case…
  3. Apparently no browser supports these values, so there's no actual compat story to worry about, right?

Unless there's a reason for web developers to expect these values to work, I'm inclined to suggest dropping all of these identical notes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, I get it now: Edge before Edge 79 is the oddity; no current browser supports it, but there was somewhat recent support in Edge. Nobody else has implemented this. Reading the linked bugs, it looks like nobody else will implement it.

Here's my new take:

  • Drop all the notes on the SVGFECompositeElement data about accepted attributes. in1 etc. are reflected attributes, so we don't cover supported values in the interface. I get into more detail about this on the lighterForError feature, since it's a bigger issue there.
  • On the feComposite data, reduce this to one and only one note, for Edge (see suggestion below).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially yes. You can specify those of course (they are "valid" options) but the only thing they they work on is pre-Blink-Edge. So "no current browser".

My read of the defect reports is because this is very hard to do, not because it isn't a good idea. Even the example in the SVG spec uses these. I have just removed that example from the docs. Also yes, that some browsers won't ever fix this.

But my concern is that without the note everyone will think all the spec values for in1 and in2 will work. I think you're saying "that doesn't matter". I will take your direction and update appropriately :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. To clear up a point of confusion: reflected attributes set and get the source element's attributes. For example, given page source <feComposite in1="somevalue">, SVGFECompositeElement.in1 returns "somevalue" and SVGFECompositeElement.in1 = "NEW" sets the element to <feComposite in1="NEW">. In terms of behavior, reflected attributes provide access to underlying element but don't really add anything more.

    Consequently, to avoid duplication, we document support for attribute values on the element itself, not the interface to that element. We figure that people don't really learn about, for example, the attributes of HTML elements by reading the HTML*Element interface docs.

  2. my concern is that without the note everyone will think all the spec values for in1 and in2 will work

    I didn't realize the spec used these (unsupported) values as examples and I appreciate that you're trying to guard against misunderstanding here. In that case, I think the right way to handle this is to have subfeatures for in1 and in2 (on the svg.elements.feComposite.{in1,in2} features) that represent each of the values for it and provide complete compat data for each, instead of notes. I think that gives readers a clearer idea of what is and is not supported.

    There's one additional bit of nuance here, however: because spec authors write ahead of implementation, there are often features which are in specs and never implemented. BCD can be a source of additional confusion, as folks get the idea that a feature in BCD means that it's a live concern. This is why we avoid all false features, where possible.

    In this case, the BackgroundImage and BackgroundAlpha values were once supported by at least one browser, so that deserves an entry.

  3. I was trying to avoid repetitious notes. Repetitious notes are hard to follow. When every entry has a note and the text is very similar between them, then it's hard to know what's actually noteworthy. That's why I was leaning toward writing notes for the exceptional, not usual, case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional thoughts here:

  • I missed that IE supported BackgroundImage and BackgroundAlpha as well. I think my general point still stands, however, that these are interesting because they're different from the others.
  • I said that we should "have subfeatures for in1 and in2 (on the svg.elements.feComposite.{in1,in2} features) that represent each of the values". I am not asking you to add features for every value yourself as part of this PR (that's optional). I just meant that svg.elements.feComposite.{in1,in2}.BackgroundImage and svg.elements.feComposite.{in1,in2}.BackgroundAlpha would be germane here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddbeck You didn't miss anything about IE. I'm an idiot - those notes were supposed to be on Edge. Fixed.

But my feeling is that this is probably worth merging now because it is "not wrong" and adds a lot of corrected information about supported versions.

There are ongoing discussions like #9074 (comment) and this one on possibly adding subfeatures, but I'd happily do those as a post process. Up to you.

@@ -329,6 +351,54 @@
}
}
},
"lighterForError": {
Copy link
Collaborator Author

@hamishwillee hamishwillee Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robert (domain expert) confirmed lighterForError (what a weird key name for operator supporting the lighter value!) should be added to SVGFECompositeElement too. I have matched the feComposite versions exactly except for Safari.

For Safari the operator was not added until version 6, so I pushed the implementation of operator value lighter to be that same version. That is not "rock solid" (i.e. no bug confirmation) but very likely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but we should drop this feature entirely. For reflected attributes, we typically just track whether the attribute exists as part of the API. This is us mostly following the convention of specifications for these things: the IDL usually specifies the reflection, while the specification for the element defines valid values.

(Plus—and this is definitely not your fault, since you're following the structure from the element data—the structure is wrong. This ought to be a subfeature of operator, not a peer to operator.)

Copy link
Collaborator Author

@hamishwillee hamishwillee Feb 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddbeck So to summarize, I just remove this addition altogether? - we simply don't track things supported at this level in the BCD. Sounds nuts, but there you go.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should only have data on the element, not the interface. I got into this more in #9074 (comment). But let me know if you have more questions—it's not an obvious distinction.

Copy link
Collaborator Author

@hamishwillee hamishwillee Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddbeck What I think you're saying is that we don't document the bits of an SVG element that reflects a javascript API - we'd just document the javascript behaviour. Right?

So I understand why you wanted me to delete the lighterForError from the reflected API svg/elements/feComposite.json , but THIS is api/SVGFECompositeElement - the API from which we reflect. Which makes me think that it should have stayed.

Or did you mean that we document on the element but not the javascript API? Either way, doesn't one of them need comments about the features? The comments here said delete this from both /api and /svg

What am I missing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or did you mean that we document on the element but not the javascript API? Either way, doesn't one of them need comments about the features? The comments here said delete this from both /api and /svg

I meant on the element, not the API. Sorry, I didn't mean to suggest deleting all of them everywhere. I think the various threads on this PR made it hard to keep track of which thing was which. I'll post a standalone review that summarizes things that need to change to get this merged.

"firefox_android": {
"version_added": "86"
},
"ie": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, IE doesn't work support lighter for feComposite, so set false in that and here too (verified by testing).

@hamishwillee
Copy link
Collaborator Author

@evilpie @ddbeck This should now be ready for review. I've added notes throughout on what I've done. General things to note to make review easier are that according to Robert, the Moz domain expert:

  • if feComposite is supported, then in, in2 and all the k values are supported by everything. So these are all moved from null to at least true (if version not known).
  • feComposite and SVGFECompositeElement mirror each other in terms of functionality. That's why I updated that to have lighter and BackgroundImage. Versions might not completely match up though.
  • BackgroundImage/BackgroundAlpha only supported by Edge prior to Blink support.

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a complicated one! Thanks for digging into this, @hamishwillee. I've got a bunch of suggestions, but it took me a while to understand the whole story here, so it might be a bit confused in places. If there's any questions about my comments, please say something. Thank you!

@@ -51,40 +51,52 @@
"__compat": {
"support": {
"chrome": {
"version_added": "5"
"version_added": "5",
"notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts there:

  1. If I understand correctly, this note means that neither BackgroundImage nor BackgroundAlpha are accepted as values to the in1 or in2 attributes?
  2. Ordinarily, I'd say that when we repeat the same note again and again, it's often a sign that the information needs to be expressed as a subfeature or something else odd is going on. Except, in this case…
  3. Apparently no browser supports these values, so there's no actual compat story to worry about, right?

Unless there's a reason for web developers to expect these values to work, I'm inclined to suggest dropping all of these identical notes.

svg/elements/feComposite.json Outdated Show resolved Hide resolved
svg/elements/feComposite.json Outdated Show resolved Hide resolved
svg/elements/feComposite.json Outdated Show resolved Hide resolved
svg/elements/feComposite.json Outdated Show resolved Hide resolved
@@ -51,40 +51,52 @@
"__compat": {
"support": {
"chrome": {
"version_added": "5"
"version_added": "5",
"notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, I get it now: Edge before Edge 79 is the oddity; no current browser supports it, but there was somewhat recent support in Edge. Nobody else has implemented this. Reading the linked bugs, it looks like nobody else will implement it.

Here's my new take:

  • Drop all the notes on the SVGFECompositeElement data about accepted attributes. in1 etc. are reflected attributes, so we don't cover supported values in the interface. I get into more detail about this on the lighterForError feature, since it's a bigger issue there.
  • On the feComposite data, reduce this to one and only one note, for Edge (see suggestion below).

svg/elements/feComposite.json Outdated Show resolved Hide resolved
@@ -329,6 +351,54 @@
}
}
},
"lighterForError": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but we should drop this feature entirely. For reflected attributes, we typically just track whether the attribute exists as part of the API. This is us mostly following the convention of specifications for these things: the IDL usually specifies the reflection, while the specification for the element defines valid values.

(Plus—and this is definitely not your fault, since you're following the structure from the element data—the structure is wrong. This ought to be a subfeature of operator, not a peer to operator.)

svg/elements/feComposite.json Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

@ddbeck Thanks for the review. I think I have now done the fixes. Hope I got it right.

@hamishwillee
Copy link
Collaborator Author

PS Partially the response confused me because I don't know what a reflective property is.

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a summary of what needs to change before we merge this:

API features

  • Remove the notes (new in this diff) from the api features
  • Rename in1 to in (to match the name of the element's attributes and the spec—this is new and I only just noticed it)

SVG features

  • Reinstate lighterForError (SVG only, not API)
  • Move lighterForError one level deeper, as a child to operator
  • Rename lighterForError to lighter and rewrite the description to <code>lighter</code> value

@@ -57,7 +57,8 @@
"version_added": "18"
},
"edge": {
"version_added": "12"
"version_added": "12",
"notes": "Before Edge 79, <code>BackgroundImage</code> and <code>BackgroundAlpha</code> were supported."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this note (the one on svg.elements.feComposite.in covers it)

svg/elements/feComposite.json Show resolved Hide resolved
api/SVGFECompositeElement.json Outdated Show resolved Hide resolved
api/SVGFECompositeElement.json Outdated Show resolved Hide resolved
@hamishwillee
Copy link
Collaborator Author

Thanks for the clear direction @ddbeck . I'm pretty sure this captures it.

@chrisdavidmills
Copy link
Collaborator

@ddbeck this one is mentioned in the 86 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/86#svg

Can you have a quick look at @hamishwillee 's latest fixes and get it merged if you are happy? Thanks!

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 🎉

@ddbeck ddbeck merged commit 40af4a0 into mdn:master Mar 3, 2021
@ddbeck ddbeck changed the title feComposite support for operator lighter property Add feComposite support for operator attribute's lighter value Mar 3, 2021
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Mar 3, 2021
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Mar 3, 2021
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Mar 4, 2021
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Mar 4, 2021
@hamishwillee hamishwillee deleted the feComposite branch March 8, 2021 05:47
foolip added a commit to foolip/browser-compat-data that referenced this pull request May 13, 2022
Original source: mdn#9074

This demo was used to test:
https://yari-demos.prod.mdn.mozit.cloud/en-US/docs/Web/SVG/Element/feComposite/_sample_.example.html

The following support was confirmed with BrowserStack:
 - Chrome 41 (not in 40)
 - Edge 79 (not in 18)
 - Firefox 86 (not in 85)
 - Safari 10.1 (not in 9.1)

The precise Safari version was determined by source:
https://trac.webkit.org/changeset/195745/webkit
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Configurations/Version.xcconfig?rev=195745

WebKit trunk version 602.1.18 maps to Safari 10 (using WebKit 602.1.50).

Part of mdn#7844.
@foolip
Copy link
Collaborator

foolip commented May 13, 2022

Sorry to revive an old PR, but just a note that I found the Chrome, Edge and Safari versions here to be incorrect and fixed them in #16278. For Chromium and WebKit, it appears that commits were mapped to versions in an incorrect way. The Chrome mistake is probably due to #7844, but I don't know what happened for WebKit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:svg 🖌️ Compat data for SVG features. https://developer.mozilla.org/docs/Web/SVG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants