Add feComposite support for operator attribute's lighter value #9074
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,40 +51,52 @@ | |
| "__compat": { | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": "5", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "chrome_android": { | ||
| "version_added": "18", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "edge": { | ||
| "version_added": "12", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported from version 79 (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "firefox": { | ||
| "version_added": "3", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://bugzil.la/437554'>bug 437554</a>)." | ||
| }, | ||
| "firefox_android": { | ||
| "version_added": "4", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://bugzil.la/437554'>bug 437554</a>)." | ||
| }, | ||
| "ie": { | ||
| "version_added": "10", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported." | ||
| }, | ||
| "opera": { | ||
| "version_added": "15", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "opera_android": { | ||
| "version_added": "14", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "safari": { | ||
| "version_added": "6", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://webkit.org/b/6022'>Webkit bug 137230</a>)." | ||
| }, | ||
| "safari_ios": { | ||
| "version_added": "6", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://webkit.org/b/6022'>Webkit bug 137230</a>)." | ||
| }, | ||
| "samsunginternet_android": { | ||
| "version_added": "1.0", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "webview_android": { | ||
| "version_added": "≤37", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| } | ||
| }, | ||
| "status": { | ||
|
|
@@ -98,13 +110,16 @@ | |
| "__compat": { | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": "5", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "chrome_android": { | ||
| "version_added": "18", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "edge": { | ||
| "version_added": "12", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported from version 79 (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "firefox": { | ||
| "version_added": "3" | ||
|
|
@@ -113,25 +128,32 @@ | |
| "version_added": "4" | ||
| }, | ||
| "ie": { | ||
| "version_added": "10", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported." | ||
| }, | ||
| "opera": { | ||
| "version_added": "15", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "opera_android": { | ||
| "version_added": "14", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "safari": { | ||
| "version_added": "6", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://webkit.org/b/6022'>Webkit bug 137230</a>)." | ||
| }, | ||
| "safari_ios": { | ||
| "version_added": "6", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://webkit.org/b/6022'>Webkit bug 137230</a>)." | ||
| }, | ||
| "samsunginternet_android": { | ||
| "version_added": "1.0", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| }, | ||
| "webview_android": { | ||
| "version_added": "≤37", | ||
| "notes": "BackgroundImage/BackgroundAlpha are not supported (see <a href='https://crbug.com/137230'>Chromium bug 137230</a>)." | ||
| } | ||
| }, | ||
| "status": { | ||
|
|
@@ -329,6 +351,54 @@ | |
| } | ||
| } | ||
| }, | ||
| "lighterForError": { | ||
|
There was a problem hiding this comment. Robert (domain expert) confirmed For Safari the There was a problem hiding this comment. 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 There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. There was a problem hiding this comment. @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 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? There was a problem hiding this comment.
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. |
||
| "__compat": { | ||
| "description": "<code>operator</code> value: <code>lighter</code>", | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": "45" | ||
| }, | ||
| "chrome_android": { | ||
| "version_added": "45" | ||
| }, | ||
| "edge": { | ||
| "version_added": "≤18" | ||
| }, | ||
| "firefox": { | ||
| "version_added": "86" | ||
| }, | ||
| "firefox_android": { | ||
| "version_added": "86" | ||
| }, | ||
| "ie": { | ||
|
There was a problem hiding this comment. Note, IE doesn't work support lighter for feComposite, so set false in that and here too (verified by testing). |
||
| "version_added": false | ||
| }, | ||
| "opera": { | ||
| "version_added": "32" | ||
| }, | ||
| "opera_android": { | ||
| "version_added": "32" | ||
| }, | ||
| "safari": { | ||
| "version_added": "6" | ||
| }, | ||
| "safari_ios": { | ||
| "version_added": "6" | ||
| }, | ||
| "samsunginternet_android": { | ||
| "version_added": "5.0" | ||
| }, | ||
| "webview_android": { | ||
| "version_added": "45" | ||
| } | ||
| }, | ||
| "status": { | ||
| "experimental": false, | ||
| "standard_track": true, | ||
| "deprecated": false | ||
| } | ||
| } | ||
| }, | ||
| "operator": { | ||
| "__compat": { | ||
| "support": { | ||
|
|
||
There was a problem hiding this comment.
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 v79There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts there:
BackgroundImagenorBackgroundAlphaare accepted as values to thein1orin2attributes?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.There was a problem hiding this comment.
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:
SVGFECompositeElementdata about accepted attributes.in1etc. are reflected attributes, so we don't cover supported values in the interface. I get into more detail about this on thelighterForErrorfeature, since it's a bigger issue there.feCompositedata, reduce this to one and only one note, for Edge (see suggestion below).There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.in1returns"somevalue"andSVGFECompositeElement.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*Elementinterface docs.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
in1andin2(on thesvg.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
falsefeatures, where possible.In this case, the
BackgroundImageandBackgroundAlphavalues were once supported by at least one browser, so that deserves an entry.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two additional thoughts here:
in1andin2(on thesvg.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 thatsvg.elements.feComposite.{in1,in2}.BackgroundImageandsvg.elements.feComposite.{in1,in2}.BackgroundAlphawould be germane here.There was a problem hiding this comment.
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.