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

api.EventTarget.addEventListener.options - Missing feature for signal option #9555

Closed
ilogico opened this issue Mar 17, 2021 · 11 comments · Fixed by #9562
Closed

api.EventTarget.addEventListener.options - Missing feature for signal option #9555

ilogico opened this issue Mar 17, 2021 · 11 comments · Fixed by #9562
Assignees
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API

Comments

@ilogico
Copy link

ilogico commented Mar 17, 2021

MDN URL: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

What information was incorrect, unhelpful, or incomplete?

The browser compatibility table.

Specific section or headline?

What did you expect to see?

Information about the signal option.

Did you test this? If so, how?

MDN Content page report details
@hamishwillee hamishwillee self-assigned this Mar 19, 2021
@hamishwillee
Copy link
Collaborator

hamishwillee commented Mar 19, 2021

@ddbeck The BCD for does not mention compatibility for the option signal.

I was going to answer with "we only document compatibility issues where the compatibility differs - so here the options "signal" would be assumed to "match" that of addEventListener itself. But then I realised it that were true then, if I add a new option "Fred" tomorrow, the assumption would be that "Fred" was added with addEventListener (at least until we get around to documenting it.

Can you confirm the process - ie. is it "missing" or not expected. If it is missing, I will update BCD with all missing options etc.

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 19, 2021

@hamishwillee In this case, the table does have entries for capture, once, and passive already and the compat data varies between those entries. I wouldn't be surprised if its support is also different from the others (since AbortSignal itself appears to be newer than the other options), too. Taken together, I think it's reasonable to expect that signal is represented.

if I add a new option "Fred" tomorrow, the assumption would be that "Fred" was added with addEventListener (at least until we get around to documenting it.

Yeah, this stuff happens. Older APIs tend to have a clunkier history. If some API originally shipped with a set of options, I would not add the individual options to BCD until the new option ("Fred") came along. If someone were to open a PR adding "Fred" alone, I'd be happy to merge it without adding features for the other options (on the presumption that the others were added at the same time as their parent), but I'd also accept a full set of features (there's a bit of authorial judgement happening, after all).

This does make a false impression in between the time that "Fred" ships and the "Fred" feature is added, but BCD is generally lagging in this way because we don't add features speculatively—otherwise we'd have tables littered with rows of all false data for features that never got beyond the proposal stage.

Does that help?

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 19, 2021

Also, if you wanted to move this issue to mdn/browser-compat-data, that would also be good (unless your planning some content work as well).

@hamishwillee
Copy link
Collaborator

@ddbeck I don't believe I have the magic power to move this item to bcd (and I would like to - do not plan on updating the docs).

Thanks for the explanation. I think you are saying that, if a parent is documented as present then (even if BCD doesn't say anything about something) you can assume that that subfeatures probably are. No certainty though, and you have lesser certaintly of the version in which they were added. This is to avoid documenting a million false cases that would have to disappear later. Feels flakey.

In this case I think you are saying that, given AbortSignal was implemented later, there must be compatibility differences associated with signal? I ran the docs example and it is definitely supported on chrome (and friends) and Firefox. Though this bug indicates maybe support is a bit flakey - https://bugzilla.mozilla.org/show_bug.cgi?id=1680530

Is there any way I can use AbortSignal versions to seed the minimum level of support in some way? i.e. say "Supported but must be >= 55 and might be even higher". I am guessing thta this is a case where I just have to hunt through bugs right?

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 22, 2021

I don't believe I have the magic power to move this item to bcd (and I would like to - do not plan on updating the docs)

OK, I'll do this shortly. 👍

I think you are saying that, if a parent is documented as present then (even if BCD doesn't say anything about something) you can assume that that subfeatures probably are.

Yes, though I'll add that this is basically pragmatic. For starters, it's how most people tend to react to feature data; fighting an interpretation is hard (see: what does in version_added strings mean). But also, it'd get terribly fractal if we supposed otherwise. Every method argument would need a feature and every one of those would need a subfeature for each of its supported types and on and on.

In this case I think you are saying that, given AbortSignal was implemented later, there must be compatibility differences associated with signal?

Yes, exactly. To spell this out more clearly:

  1. The type of signal is an AbortSignal object.
  2. AbortSignal was introduced after addEventListener (for any set of allowed arguments), at least for those browsers which are old enough to have captured the whole history.
  3. Therefore, support for addEventListener and signal cannot be the same version. In other words, we know that version_added for signal must be equal to or greater than the value of AbortSignal.

Is there any way I can use AbortSignal versions to seed the minimum level of support in some way? i.e. say "Supported but must be >= 55 and might be even higher". I am guessing thta this is a case where I just have to hunt through bugs right?

Yeah, you're on the right track here. I'll sometimes search for bugs, but if something doesn't turn up quickly, I'll test my way to an exact version. To actually do that, I'd need a test case.*

I ran the docs example and it is definitely supported on chrome (and friends) and Firefox.

And you found one! I'd probably copy and paste this into a file (maybe dress it up the output a bit to speed things along) and try it a half-dozen times in BrowserStack to figure out which versions it first appeared in. If you (or someone else) came along with such a test case, I'd be more than happy to do the actual testing. You don't need to know everything to start an issue or PR on BCD; someone bringing in a bit of knowledge and expertise around a specific API is a huge boon, because I know the ins-and-outs of BCD itself, to get it over the finish line in short order.

* Also, I should note that, for simple existence checks for APIs, there exist tools such as https://mdn-bcd-collector.appspot.com/ which obviate the need to write or find a test from scratch.

@ddbeck ddbeck transferred this issue from mdn/content Mar 22, 2021
@ddbeck ddbeck added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Mar 22, 2021
@ddbeck ddbeck changed the title Issue with "EventTarget.addEventListener()": (short summary here please) api.EventTarget.addEventListener - Missing feature for "signal" option Mar 22, 2021
@ddbeck ddbeck changed the title api.EventTarget.addEventListener - Missing feature for "signal" option api.EventTarget.addEventListener.options - Missing feature for "signal" option Mar 22, 2021
@ddbeck ddbeck changed the title api.EventTarget.addEventListener.options - Missing feature for "signal" option api.EventTarget.addEventListener.options - Missing feature for signal option Mar 22, 2021
@ddbeck
Copy link
Collaborator

ddbeck commented Mar 22, 2021

@hamishwillee alright, once I had a test case this all came together! Want to do the honors of opening a PR? 😃

OK, I tested this one out, using this test (modified from the example on MDN). The results are:

  • Chrome: 90
  • Firefox: 86
  • Safari: false

And we can mirror or presume false for the other browsers.

I kinda didn't trust something so new (addEventListener is basically primordial, right?) but I was able to find some interesting bugs (thanks to a lucky search against the gecko mirror) to back this up:

@hamishwillee
Copy link
Collaborator

hamishwillee commented Mar 23, 2021

Thanks very much @ddbeck - saved me lots of time, so I guess I will take the BCD task: #9562 :-)

I get a pass with your test code on Chrome and edge 89. I also got a pass on ,OPera 74 - which corresponds to Chrome 88. My guess would therefore be that this actually went in on Chrome 88 ... but I don't have a version to test that with. I have guessed at chrome 89 just because I can't test. But let's continue that conversation on the PR

PS Thanks for the details above. Knowing additional places to look for tests and about things like BrowserTest very useful (though it isn't clear how to run a test file like yours on that site - and the docs example works fine back quite a few versions, which it shouldn't). I am very happy to provide lots of info and have you do the real work. On the other hand, I learn new things every day doing this.

@hamishwillee
Copy link
Collaborator

@chrisdavidmills We seem to have missed this bug in FF86 https://bugzilla.mozilla.org/show_bug.cgi?id=1679204#c4 , which is fixed by a BCD update. Do you want me to add a release note on this? Either way, can you please remove dev-doc-needed because I can't.

@chrisdavidmills
Copy link
Collaborator

@hamishwillee yes please.

I've updated DDN -> DDC

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 23, 2021

I figured out the discrepancy for Chrome 88/90. See #9562 (comment).

though it isn't clear how to run a test file like yours on that site

OK, yes, I skipped over that detail. If I have a test locally that I want to try in BrowserStack or Sauce Labs, then I'll use ngrok to serve the file, which creates a publicly-reachable URL. Though sometimes I'll put something into CodePen or the like. It's almost as if I should write some documentation about how to test BCD stuff. 😬

@hamishwillee
Copy link
Collaborator

@chrisdavidmills Relnote for this in mdn/content#3516

@ddbeck Yes, some docs on this would be lovely. I'm bookmarking all these resources whether you do or not though :-)

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants