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 WebSpeech events #3420

Merged
merged 2 commits into from
Feb 21, 2019
Merged

Add WebSpeech events #3420

merged 2 commits into from
Feb 21, 2019

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Feb 8, 2019

This is a proposal to add the events compat data for the Web Speech API. The docs have been moved and refactored, now we also need to see where we want to install the compat data.
See mdn/sprints#915 (comment) for the discussion.

In here I'm proposing install events at "targetInterface.events.eventname".

Much like the new MDN page slugs, the events are installed under "targetInterface.eventname_event"

The compat data itself is assumed to be the same as the already existing data for Web Speech.

@Elchi3 Elchi3 added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 8, 2019
@Elchi3 Elchi3 requested a review from wbamberg February 8, 2019 14:08
@Elchi3 Elchi3 mentioned this pull request Feb 8, 2019
19 tasks
@jpmedley
Copy link
Collaborator

jpmedley commented Feb 8, 2019

LGTM.

Are events always going to be at the bottom?

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 11, 2019

Are events always going to be at the bottom?

Good question. Could also make this alphabetically, so that tooling can more easily enforce sorting, like proposed in #3427 (I'm not yet sure if it is a good idea to give up control and make everything alphabetically, but maybe it is best for consistency and maintainability).

@jpmedley
Copy link
Collaborator

In long lists people will scroll to the bottom then if there are no events scroll back up to the Es just to make sure they didn't miss it. I don't like friction.

I understand why you might be scratching your head on this one. When I asked the question, I favored keeping events at the bottom. Now I don't.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 11, 2019

My two cents: I prefer the idea of relying on alphabetical sorting. It's easier to get right when we can automatically fix things. Also, I'd prefer to be in the habit of using Ctrl-F to find something in a JSON file than relying on the relative order of support statements.

I figure the order has no bearing on how it's shown on MDN.

@jpmedley
Copy link
Collaborator

Well, currently it does. If you look at RTCPeerConnection before the next deployment you'll see it's browser data is in BCD order. If we enforce order on BCD, that's one less thing renderers need to do.

@ddbeck
Copy link
Collaborator

ddbeck commented Feb 11, 2019

Yeah, you're right about that. Really I should've said, we shouldn't rely on the order in the source to control the appearance on MDN.

@wbamberg
Copy link
Collaborator

wbamberg commented Feb 14, 2019

I guess these tables will appear in (at least) 2 places:

  • the event page, which will just show data for this event
  • the interface page, for which (I assume) we would like to include these items

Embedding data using {{Compat("api.SpeechRecognition")}} gives me a table like:

screen shot 2019-02-13 at 16 07 20

... that is, it's missing the events entirely. I guess perhaps the macro doesn't by default explore into items, like events here, that don't have __compat but that are just containers for subfeatures?

It's a bit unconventional AFAIK for BCD to use hierarchy to group a collection of "things of a particular kind" - for instance we don't do this for properties or methods, so it looks a bit odd to do it for events.

We do need something though, because event names can clash with the names of properties and methods (e.g. IDBDatabase.close() and the close event).

Of course we face the same problem for MDN slugs, and there we decided against using hierarchy, and instead use "eventname_event" for the slug. Perhaps we should do the same here, with "eventname_event" as the key value and a description like <code>eventname</code> event?

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 15, 2019

I think Will has excellent points, and I'm going to update this PR to use the flat hierarchy. I had data consumers in mind who might want to filter out events (or get them specifically) but they can still do so by looking for the "eventname_event" key value. For sorting, I guess, we will also sort just alphabetically as we also don't sort methods and properties separately. for instance. I also like that this is on par with the MDN slugs then.

@wbamberg
Copy link
Collaborator

I had data consumers in mind who might want to filter out events

Indeed that's a thing that the hierarchical organization facilitates. Is there a particular reason why you think this should be supported? Because I'd have a couple of thoughts in relation to that: (1) is there a reason to do this for events but not for methods, properties etc and (2) hierarchy is a rather inflexible way to do this, if it's a thing we want to do.

(I mean, this is a big topic, the whole "consumers want a particular view of the data" thing, that extends into issues like https://discourse.mozilla.org/t/browser-compatibility-tables-for-api-overviews/33832. Maybe it is a thing we would want to attack some time, but we shouldn't underestimate it :).

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 18, 2019

(1) is there a reason to do this for events but not for methods, properties etc and (2) hierarchy is a rather inflexible way to do this, if it's a thing we want to do.

You're on point here! I think we should look at this more holistically eventually. I think this will come up again as we see consumers actually using the api/ compat data. I agree we shouldn't underestimate this work.


I've now updated this PR to install events under "targetInterface.eventname_event"

@Elchi3 Elchi3 mentioned this pull request Feb 18, 2019
@jpmedley
Copy link
Collaborator

Instead of doing "*eventname*_event" for the slug, might it be better to have an attribute hanging off of it that specifies its type (event, property, method, etc..). (This would also replace the description now used for constructors.) Then in every case, not just events, I could write code like the following, which would return every property from a given interface:

function getProperties() {
  return bcd.api.Widget.filter(branch => {
    return branch.type === 'property';
  }
}

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 20, 2019

Thanks, Joe! I think this kind of classification is interesting to make the data more machine-readable (much like we're also looking into this for CSS compat data). I think it deserves an own issue or project, though. I don't want to block getting event data in here for now as it also affects more than events.

@jpmedley
Copy link
Collaborator

I understand the concern, but making the change later rather than sooner will potentially mean breaking third-party code.

Copy link
Collaborator

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

I am happy with this data as is. R+.

I did a bit of testing, and it is still accurate to say that Opera/Opera Mobile doesn't support recognition and Opera Mobile doesn't support synthesis.

@Elchi3 Elchi3 removed the request for review from wbamberg February 21, 2019 11:15
@Elchi3
Copy link
Member Author

Elchi3 commented Feb 21, 2019

Thanks, Chris!

@Elchi3 Elchi3 merged commit ceb53ea into master Feb 21, 2019
@Elchi3 Elchi3 deleted the webspeech-events branch February 21, 2019 11:16
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 this pull request may close these issues.

None yet

5 participants