screen-share / captured-surface-control

Web API allowing capturing applications limited control over captured surfaces.
https://screen-share.github.io/captured-surface-control/
8 stars 2 forks source link

Limit setZoomLevel() to the local user #14

Open eladalon1983 opened 1 month ago

eladalon1983 commented 1 month ago

When discussing the APIs proposed by this spec, Mozilla and Apple raised concerns over the possibility for an app to delegate control to a remote user. The approach of captureWheel was generally seen as appropriate to mitigate these concerns. I suggest we adopt a similar approach to mitigate the same concerns for zoom-control.

Consider:

  CaptureController.addSetZoomHandler(element, selector, handler);

Then specify that when the event indicated by selector (e.g. "click") happens on element, if it is a trusted event, then:

  1. Queue a task to open a window of opportunity to call setZoomLevel().
  2. Queue a task to invoke handler.
  3. Queue a task to close the window of opportunity to call setZoomLevel().

Effectively, this allows changing the zoom level only from within a handler, that is only invoked in response to immediate activity by the local user (cf. isTrusted). But it doesn't specify what that activity is - it allows it to be click, sliding a slider, etc.

Example:

  const controller = new CaptureController();
  const element = document.getElementById('zoomIncreaseButton');
  const handler = (event) => {
    const levels = CaptureController.getSupportedZoomLevels();
    const index = levels.indexOf(controller.getZoomLevel());
    const newZoomLevel = levels[Math.min(index + 1, levels.length - 1)];
    controller.setZoomLevel(newZoomLevel);
  }
  controller.addSetZoomHandler(element, 'onclick', handler);

Wdyt, @youennf and @jan-ivar?

eladalon1983 commented 1 week ago

Here's a simpler approach: setZoomLevel() should only be called from the context of an event handler, where the event being handled is trusted.

That's it.

If anyone is aware of a way to specify that in spec language, I think that's sufficient. However, I could not yet find a way to specify this formally. I consulted a colleague who has faced a similar challenge, and he says that they went with the following work-around:

Clarifications:

A JSFiddle sanity-checking these claims is here: https://jsfiddle.net/kxa01sh6/

The resulting API shape is:

partial interface CaptureController {
  Promise<undefined> setZoomLevel(long zoomLevel, Event event);
};

The former usage sample now reads:

  const controller = new CaptureController();
  const zoomIncreaseButton = document.getElementById('zoomIncreaseButton');
  zoomIncreaseButton.addEventListener('click', event => {
    const levels = CaptureController.getSupportedZoomLevels();
    const index = levels.indexOf(controller.getZoomLevel());
    const newZoomLevel = levels[Math.min(index + 1, levels.length - 1)];
    controller.setZoomLevel(newZoomLevel, event);
  });

Note how this is a perfectly normal click-handler, with the only unusual portion being how the event is fed back into the setZoomLevel() method.

jan-ivar commented 1 week ago

Wouldn't this let you call the api from a mousemove event?

eladalon1983 commented 1 week ago

Yes, it would allow that. (As did the alternative approach we discussed during TPAC, which got a positive response.)

I don't think that's a problem, but in the interest of starting out conservative, I am open to adding the following:

Would that dispel your concerns?

eladalon1983 commented 1 week ago

Getting back to the issue of specifying this, here is another option:

When invoked, the user agent MUST run the following steps:

  1. Let currentEvent be Window.event, which represents the Window's current event.
  2. If currentEvent is undefined, return a promise rejected with ...
  3. If currentEvent.isTrusted is false, return a promise rejected with ...

A supposed problem is that this field is marked as "legacy", but I don't think this is critical here. I'll be presenting both options at the WebRTC WG tonight.

jan-ivar commented 1 week ago
controller.setZoomLevel(newZoomLevel, event);

Our DOM team says the UA probably doesn't need the event input here to look up its call stack to know where it's called from. Spec writers are lower on the priority of constituencies list than web developers, which suggests we should aim for:

controller.setZoomLevel(newZoomLevel);

or maybe even

controller.zoomLevel = newZoomLevel;

But let's ensure we have agreement about desired behavior first, to avoid surprises like mousemove.

Would that dispel your concerns?

If we step up a level, yes. It sounds like we want zoom controls to work through intentional and primary user interactions with <button> (click), <input type="number"> (input, change), and maybe <input type="range"> (input, change)?

That sounds fine to me. Might it work to start by just writing those requirements in prose, and leave the UA to figure out how?

eladalon1983 commented 1 week ago

Our DOM team says the UA probably doesn't need the event input here to look up its call stack to know where it's called from. Spec writers are lower on the priority of constituencies list than web developers, which suggests we should aim for

Thank you for checking with them. We are in agreement on this topic.

or maybe even

controller.zoomLevel = newZoomLevel;

As I was drafting my response to this possibility - that we need a Promise for the permission prompt - issue #27 came in, indicating that you are aware of this consideration. Shall we relegate this discussion to that issue, then?

But let's ensure we have agreement about desired behavior first, to avoid surprises like mousemove.

I believe we have consensus here, as indicated by my previous comment and my addition of permitted event types to the spec. Do we agree?

It sounds like we want zoom controls to work through intentional and primary user interactions with

That's right.

Might it work to start by just writing those requirements in prose, and leave the UA to figure out how?

Could you please advise which normative parts of the spec you propose to remove, and which non-normative prose you suggest to add?