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

"oncapturedmousechange" vs addEventListener "mousemove"/"mouseclick"/etc #1

Closed
happylinks opened this issue Mar 2, 2023 · 14 comments
Closed

Comments

@happylinks
Copy link

I was curious why the proposal was made to have a callback "oncapturedmousechange" instead of a more common API like addEventListener. I could see us wanting to support multiple types of events, maybe even keyboard events (out of scope for this proposal I know :)), and a more general event listener approach could make sense for that?

For example:

const controller = new CaptureController();
const stream = await navigator.mediaDevices.getDisplayMedia({ controller });

// In scope
controller.addEventListener("mousemove", event => console.log("mouse moved", event.clientX, event.clientY));

// Potentially soon?
controller.addEventListener("mouseclick", event => console.log("mouse clicked", event.clientX, event.clientY));

// Probably further out of scope because of security implications
controller.addEventListener("keydown", event => console.log("keydown", event.clientX, event.clientY));

Would like to better understand the choice for the callback in the proposal, thanks.

@happylinks happylinks changed the title "oncapturedmousechange" vs addEventListener "mousemove"/"click"/etc "oncapturedmousechange" vs addEventListener "mousemove"/"mouseclick"/etc Mar 2, 2023
@eladalon1983
Copy link
Contributor

Defining oncapturedmousechange implicitly allows use of addEventListener(). See for example the canonical getDisplayMedia example page, where addEventListener is used, despite not being explicitly called out by the gDM spec nor the gUM spec. Specifically, here.

@happylinks
Copy link
Author

Ahh, didn't realise that! So the event name would be "capturedmousechange"? Similar question then, any benefit in aligning it more with the existing mouse events like mousemove, mouseclick?
Want me to open a new issue for potentially adding mouse clicks as well?

@eladalon1983
Copy link
Contributor

Could you clarify what you mean by aligning it more closely with existing mouse events? It is currently modelled after this event. Note that it could then be easily extended to include additional information (modifier keys, buttons) if we get consensus that it's safe.

@happylinks
Copy link
Author

Mostly calling it "mousechange" vs "mousemove" I guess. Or am I missing an existing mousechange event somewhere. Other than that I think it's good.
Oh and surfaceX vs screenX, what's the reason behind that?

@eladalon1983
Copy link
Contributor

Mostly calling it "mousechange" vs "mousemove" I guess. Or am I missing an existing mousechange event somewhere.

I'm flexible with names. Whatever the consensus happens to be is fine by me. Generally speaking.

Oh and surfaceX vs screenX, what's the reason behind that?

What's the alternative?

@happylinks
Copy link
Author

happylinks commented Mar 2, 2023

I'm flexible with names. Whatever the consensus happens to be is fine by me. Generally speaking.

Cool, then my vote goes for naming it after the existing events like (captured)mousemove, (captured)mouseclick, etc. Devs looking for the events will have an easier time finding it if it's something they already used in other places.

What's the alternative?

Using "screen" instead of "surface", since screen is already used by mousemove: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/screenX

@eladalon1983
Copy link
Contributor

Cool, then my vote goes for naming it after the existing events like (captured)mousemove, (captured)mouseclick, etc. Devs looking for the events will have an easier time finding it if it's something they already use in other places.

Would you like 5-10 minutes to present about this in the next SCCG meeting?

Using "screen" instead of "surface"

What if the surface being captured is a tab? Or a window? "Screen" would be misleading then.

@happylinks
Copy link
Author

Would you like 5-10 minutes to present about this in the next SCCG meeting?

Sure, I can put some options on a slide and discuss :)

What if the surface being captured is a tab? Or a window? "Screen" would be misleading then.

Ok hadn't thought about that. I see that getDisplayMedia also calls it "surface" everywhere, in that case it makes sense.

@fred-wang
Copy link
Contributor

Regarding support for addEventListener() etc, this is done in Media Capture and Streams by making the interfaces inherit from https://dom.spec.whatwg.org/#eventtarget ; see issue #9

@fred-wang
Copy link
Contributor

Fixing #9 provided the addEventListener/removeEventListener/dispatchEvent with event type "capturedmousechange"

Not sure if the discussions around "mousemove"/"mouseclick" is addressed though.

@eladalon1983
Copy link
Contributor

I think an event that delivers the entire new state, allows firing a single event even when two events happen at the same time. It also allows the browser the flexibility to employ optimizations that buffer and batch events. I therefore prefer a single "x-changed" over a set of ("x-moved", "x-clicked", "x-scrolled").

(The option of presenting a different view is always available either here or in the CG meetings.)

@fred-wang
Copy link
Contributor

So reading this issue again, the problems raised were:

  1. Allow addEventListener/removeEventListener. This is addressed now that we made CaptureController an EventTarget and introduced a capturedmousechange event type.
  2. Naming for event members surfaceX/surfaceY. This was explained.
  3. Naming for event types
    3.1 "captured" prefix. (is it an issue?)
    3.2 single "x-change" VS multiple "x-move", "x-click".

So only 3. remains. As I commented #2 (comment) I would suggest either to open a separate issue or to change the title, so it's easier to understand what the issue is about when reviewing the list of open issues. If we decide this is something for the future, I'd also use the discussed approach with labels so we can forget about it from the time being.

I guess we can probably first discuss this in the CG. Personally as an implementer I agree with what Elad said in his last comment and also that allows to minimize the number of event types / interfaces. But I also guess it won't be too difficult to make CaptureController convert its received "change" event into multiple "move", "click" etc if that's something web developers want.

@happylinks
Copy link
Author

happylinks commented Jul 7, 2023

We discussed naming in a CG meeting a while back, I think we didn't really feel strongly about picking one vs another so I think we can start with "capturedmousechange".
Surface-x is indeed clear, and great that addEventListener works now!
So yeah, I think this can be closed. @eladalon1983 anything else from your side? Else I'll close it.

@eladalon1983
Copy link
Contributor

So yeah, I think this can be closed. @eladalon1983 anything else from your side? Else I'll close it.

I'm glad we've reached consensus. :-)

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

No branches or pull requests

3 participants