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

Limit setZoomLevel() to the local user #14

Open
eladalon1983 opened this issue Sep 12, 2024 · 6 comments
Open

Limit setZoomLevel() to the local user #14

eladalon1983 opened this issue Sep 12, 2024 · 6 comments

Comments

@eladalon1983
Copy link
Member

eladalon1983 commented Sep 12, 2024

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
Copy link
Member Author

eladalon1983 commented Oct 14, 2024

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:

  • Let func(arg_1, ..., arg_n) be the function you are trying to gate on trusted events.
  • Add one more argument, event.
  • Callers are expected to "manually" pass in the event from the event-handler in whose context they are executing.
  • The spec checks that the event's dispatch flag is set.
  • The spec checks that event.isTrusted is true.

Clarifications:

  • The check of the dispatch flag ensures the event passed is not an old cached event; it can only be the event from whose context we are presently executing.
  • The check of isTrusted ensures that the event was produced in response to genuine user interaction.

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
Copy link
Member

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

@eladalon1983
Copy link
Member Author

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:

  • Let permittedEventTypes be ['click', 'input', 'change'].
  • If event.type is not in permittedEventTypes, reject.

Would that dispel your concerns?

@eladalon1983
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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 (click), (input, change), and maybe (input, change)?

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?

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

2 participants