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

Putting the event target behind a permission #30

Closed
jakearchibald opened this issue Sep 14, 2020 · 38 comments
Closed

Putting the event target behind a permission #30

jakearchibald opened this issue Sep 14, 2020 · 38 comments
Assignees

Comments

@jakearchibald
Copy link

Ugh I'm kinda being 'that guy' with a "what if this looked entirely different?" issue, so feel free to ignore me.

The current design has some weirdness:

  • isMultiScreen and getScreens are async due to the potential need for permissions.
  • A screenschange event sits on the window object. It isn't clear how this interacts with permissions. It's weird for it to start/stop depending on permissions. It's equally weird for it to fire when permission isn't given.
  • Once you get the event, you need to use an async method to get the actual data, which is unusual and racey.
  • Once isMultiScreen resolves to true, getScreens may still return one item, due to race conditions.

To avoid this, an object representing screen state could be behind an async method:

const screens = await getScreens();
console.log(screens.length);
// It has an indexed property getter
console.log(screens[0]);
// It's also an iterator, so it has .entries, .forEach etc etc.
// It has a change event:
screens.addEventListener('change', () => {
  console.log(screens); // Data has changed
});

You might also want an event for when the permission is revoked, at which point the object stops being live.

This would solve #28 and #12.

If part of the API doesn't require permissions, eg change information on the primary screen, or the presence of multiple screens, that could go on Screen:

window.screen.addEventListener('change', () => {
  console.log('The primary screen has changed in some way');
});
console.log(screen.isOnlyScreen); // If you want permissionless isMultiScreen
@inexorabletash
Copy link
Member

Ugh I'm kinda being 'that guy'...

Dude, I did a happy dance when I saw you had cycles to drop by and give your always valuable feedback. Please keep being "that guy"

@jakearchibald
Copy link
Author

jakearchibald commented Sep 14, 2020

Here's a sketch that keeps the array separate, and has a 'basic' permission that just exposes isMultiScreen:

const basicScreenDetails = await getScreenDetails();
basicScreenDetails.hasMultilpleScreens; // boolean.
basicScreenDetails.screens[0]; // Primary ScreenInfo.
basicScreenDetails.screens[1]; // Undefined, even if there are multiple screens.
basicScreenDetails.addEventListener('change', () => {
  // hasMultilpleScreens or screens[0] has changed.
});

const screenDetails = await getScreenDetails({ allScreens: true });
screenDetails.hasMultilpleScreens; // boolean.
screenDetails.screens[0]; // Primary ScreenInfo.
screenDetails.screens[1]; // ScreenInfo for second screen, or undefined.
screenDetails.addEventListener('change', () => {
  // One or many of the entries in screenDetails.screens has changed.
});

@michaelwasserman
Copy link
Member

I'm very happy to get your API design input; thanks! I hope we'll get to chat; some initial qs:

  1. Why gate access to the EventTarget with a permission, beyond just sending events to execution contexts with requisite permission? We do plan to add data to the events (e.g. event.isMultiScreen and event.screens, as permitted); does that help?
  2. Would we need a separate EventTarget to support optional permission-gating of isMultiScreen and its change events? It might be nicer than adding a sync boolean and EventHandler to the existing Screen interface.
  3. Did you mean screenObserver.addEventListener(...) in your last comment?
  4. Why might combining indexable and event target characteristics be problematic?
  5. Do we need notification of permission revocation beyond proposed permission API integration? ie.
    let permission = await navigator.permissions.query({name:'window-placement'});
    permission.addEventListener('change', ... );

I really appreciate the feedback, and do think a new interface/namespace may be warranted.

@michaelwasserman michaelwasserman self-assigned this Sep 15, 2020
@jakearchibald
Copy link
Author

jakearchibald commented Sep 15, 2020

Why gate access to the EventTarget with a permission, beyond just sending events to execution contexts with requisite permission?

Mostly because I couldn't think of any other part of the platform that behaves that way. Although, now I think about it, that's how device orientation works, although it's really badly spec'd – it mentions permissions, but not of the event-dispatching parts of the spec check for the permission. I think maybe permissions were kinda stuck on afterwards there.

Most APIs seem to follow a pattern where the developer asks for a thing, and if they're allowed the thing they're given the thing, and that thing is the thing they didn't have before.

If we decide it's ok to keep the event on the global, then the information should be on the global too. The weirdness right now is that some parts of the API are behind a promise solely for permission purposes, but other parts aren't. It should be one or the other.

@kenchris I'm interested in your take here. It didn't come up in TAG review, but it seems like an important part of platform consistency.

We do plan to add data to the events (e.g. event.isMultiScreen and event.screens, as permitted); does that help?

Right now, the problem is that the event fires, but the information isn't available. However, I don't think the solution is to add that information to the event. In fact, the TAG recommends against it.

I think the information should be available synchronously once permission is granted.

I would only subclass the event if you want to provide information beyond current state. For example, it might be appropriate for the event to have information around what changed vs the previous state, but I don't think that's urgent.

Would we need a separate EventTarget to support optional permission-gating of isMultiScreen and its change events? It might be nicer than adding a sync boolean and EventHandler to the existing Screen interface.

If it's a seperate permission, I guess you would. It seems bad for interop to have some browsers put that behind a permission and others not. If it was one way or the other, the design problem goes away.

I guess you could have something like:

const basicScreenDetails = await getScreenDetails();
const allScreenDetails = await getScreenDetails({ allScreens: true });

Where basic screen details would have less info.

Did you mean screenObserver.addEventListener(...) in your last comment?

Yes 😄 sorry, I've updated it.

Why might combining indexable and event target characteristics be problematic?

I think we try to avoid having not-quite-arrays these days, in favour of real arrays.

Do we need notification of permission revocation beyond proposed permission API integration? ie.

If re-adding the permission brings all of the live objects back to life, then I guess the permission API is fine.

@jakearchibald
Copy link
Author

I've updated #30 (comment) to cater for two levels of permission.

@kenchris
Copy link

Here's a sketch that keeps the array separate, and has a 'basic' permission that just exposes isMultiScreen:

We have discussed before in the TAG, that we really don't want methods to sometimes return arrays and others times objects - this came up with native file system as well

@jakearchibald
Copy link
Author

@kenchris I agree, but none of the proposals here do that

@kenchris
Copy link

  • A screenschange event sits on the window object. It isn't clear how this interacts with permissions. It's weird for it to start/stop depending on permissions. It's equally weird for it to fire when permission isn't given.

This indeed sounds weird. I also think that a "change" event on what is returned from the method makes more sense. In that case we need to return an object and that would then have the array associated.

In many ways this is not that different from something like say the OrientationSensor (it is constructed and can throw, but it could have been returned by an async getter instead). That has the values attached as properties and an event that fires when they change.

interface ScreenInfo : EventTarget {
  readonly attribute FrozenArray<Screen> screens;
  attribute EventHandler onchange;
};
```

@kenchris
Copy link

@kenchris I agree, but none of the proposals here do that

Your example in this comment #30 (comment) did, depending on objects given to the method

@jakearchibald
Copy link
Author

In many ways this is not that different from something like say the OrientationSensor

Agreed. I was looking at those specs for inspiration. Any idea what happened with device orientation, which does seem to be a global event gated behind a permission? I assume it's just a legacy mess.

@kenchris I agree, but none of the proposals here do that

Your example in this comment #30 (comment) did, depending on objects given to the method

🤔 I don't think it did. It's gone though a few edits, but it always returned an object that has a screens property, which is an array. The very first version of that comment had an incorrect variable name which may have been misleading, but that was before I added the arguments to the method. Anyway, rest assured that I'm not proposing a method that returns different types depending on the args.

@kenchris
Copy link

kenchris commented Sep 15, 2020

OK then I misunderstood

basicScreenDetails.primary; // ScreenInfo for the primary screen.
basicScreenDetails.screens; // undefined.

That to me indicated that the basicScreenDetails represented the first screen (it even has properties like primary and screens is undefined). That seems messy. So the objects represents a screen itself, or sometimes it instead holds an array of screens.

In this case screens should just be an array of size 1 like we suggested for native file system

@kenchris
Copy link

Agreed. I was looking at those specs for inspiration. Any idea what happened with device orientation, which does seem to be a global event gated behind a permission? I assume it's just a legacy mess.

If you don't have permission the constructors will fail, if I recall correctly

@kenchris
Copy link

The Generic Sensors kind of have the difference in that you need to call "start" before they check the permission and then you get an error if that is the case. OrientationSensor is based on that https://www.w3.org/TR/orientation-sensor/

image

@jakearchibald
Copy link
Author

jakearchibald commented Sep 15, 2020

basicScreenDetails.primary; // ScreenInfo for the primary screen.
basicScreenDetails.screens; // undefined.

That to me indicated that the basicScreenDetails represented the first screen (it even has properties like primary and screens is undefined). That seems messy. So the objects represents a screen itself, or sometimes it instead holds an array of screens.

In this case screens should just be an array of size 1 like we suggested for native file system

Ohh, good call. I've updated the sketch.

Agreed. I was looking at those specs for inspiration. Any idea what happened with device orientation, which does seem to be a global event gated behind a permission? I assume it's just a legacy mess.

If you don't have permission the constructors will fail, if I recall correctly

That spec doesn't have any constructors (aside from the event constructors). You might be thinking of orientation sensor rather than device orientation.

The Generic Sensors kind of have the difference in that you need to call "start" before they check the permission and then you get an error if that is the case.

Yeah, I don't think the constructor model makes sense for this API, because there aren't advantages to start/stop. You're just getting info that's readily available, but behind a permission. As in, there's no battery impact to observing this stuff, unlike a gyroscope.

@kenchris
Copy link

basicScreenDetails.hasMultipleScreens; should be the same as checking .screens.length > 1 so not sure how useful it is, but apart from that, I like your suggestion.

@jakearchibald
Copy link
Author

basicScreenDetails.hasMultipleScreens; should be the same as checking .screens.length > 1 so not sure how useful it is

The idea is that a basic permission would tell you there are multiple screens, but wouldn't give you the details. If hasMultipleScreens is false, then there's no point asking for further permission.

It does create a weird situation where basicScreenDetails.hasMultilpleScreens can be true, but basicScreenDetails.screens[1] will be undefined, since you don't have permission to see additional screen details. Maybe having basicScreenDetails.screens be undefined in that case is better after all?

@kenchris
Copy link

That seems very confusing.

We are not getting of window.screen any time soon, so maybe just do one permission for getting access to additional info requestDetailedScreenInfo() or so.

@jakearchibald
Copy link
Author

Which part is confusing?

I think the idea is for some browsers in some situations to provide hasMultilpleScreens without requesting user permission, so you don't put the user through a permission request just to find out that window.screen has all the details you needed anyway.

@kenchris
Copy link

I think the idea is for some browsers in some situations to provide hasMultilpleScreens without requesting user permission, so you don't put the user through a permission request just to find out that window.screen has all the details you needed anyway.

But that is kind of a trap, because people often change configuration and add and remove screens, like when plugging/unplugging laptop. And this is also adding additional entropy to fingerprinting (I am sure that will come up in privacy reviews).

@jakearchibald
Copy link
Author

But that is kind of a trap,

What's the trap?

because people often change configuration and add and remove screens, like when plugging/unplugging laptop. And this is also adding additional entropy to fingerprinting (I am sure that will come up in privacy reviews).

I guess that's why it's potentially behind a permission. It isn't clear to me yet what the conditions are for this being auto-granted.

@kenchris
Copy link

What's the trap?

As it can change, the site would have to listen to a kind of "screen change" event and act when "isMultipleScreens" becomes true, but you want to guard the event behind the permission, so...

@jakearchibald
Copy link
Author

What's the trap?

As it can change, the site would have to listen to a kind of "screen change" event and act when "isMultipleScreens" becomes true

The event is already described in the sketch:

const basicScreenDetails = await getScreenDetails();
basicScreenDetails.hasMultilpleScreens; // boolean.
basicScreenDetails.screens[0]; // Primary ScreenInfo.
basicScreenDetails.screens[1]; // Undefined, even if there are multiple screens.
basicScreenDetails.addEventListener('change', () => {
  // hasMultilpleScreens or screens[0] has changed.
});

For example, the values on the sensors API can change, and you need an event to know when that happens, and I wouldn't call that a 'trap'.

@kenchris
Copy link

I guess that I don't get the point about the permissions. As I read, you want to avoid permission and still know whether there are more than one screen connected as well as listen to a change event. But in order to actually get the info about multiple screens you would need a permission and call the exact same API. To me that feels a bit counter intuitive and developer hostile, but maybe I need to think more about this

@kenchris
Copy link

I also don't think that the name "hasMultipleScreens" is that fitting. It is not always "has" because it is not always permanent as they are most likely external displays.

For dual screen devices so far we consider them one screen and one viewport, and changing that whether the browser/PWA/window spans across both physical screens is probably going to be quite confusing - @zouhir @darktears

We really need to figure out how to handle this properly. Currently I see a dual screen as one virtual screen (and one viewport) where apps might be able to layout at one part. If apps really want different windows per screen instead, we need to figure out how to handle that.

On the other hand I have seen new laptops that have an actual screen in the trackpad, but in order for developers to actually use that they would need to know that it is a track pad screen and not just another external display.

@jakearchibald
Copy link
Author

There are two levels of permission that gain you different amounts of detail:

  1. Whether there is more than one screen connected. And when that changes.
  2. Details about all auxiliary screens. And when those details change.

In particular trusted situations, it seems like 1 will be auto granted. In fact, there are cases where we already expose this. If two windows from the same origin are given different window.screen details at the same time, hasMultilpleScreens can be inferred.

If we're pushing this discussion as far as 'hostility', it seems bad to put the user through a permission prompt only to gain details you already know. If I was to point to an element of counter-intuitivity here, it would be that.

If the developer already has access to hasMultilpleScreens, which has a lower bar than full detailed screen information, they can avoid that permission prompt, because they know the result will not provide anything useful.

@jakearchibald
Copy link
Author

also don't think that the name "hasMultipleScreens" is that fitting. It is not always "has" because it is not always permanent as they are most likely external displays.

'has' doesn't infer immutability. I have 5 apples. I give 5 apples away. I no longer have 5 apples.

For dual screen devices so far we consider them one screen and one viewport, and changing that whether the browser/PWA/window spans across both physical screens is probably going to be quite confusing

That isn't what this spec is about. I recommend reading the explainer. One example I'm personally interested in: When I give a talk I have a notes window. I have to manually drag this across to the secondary screen and maximize it. This is a real pain as the secondary screen is often behind me. This API would allow window placement on an auxiliary screen.

Currently I see a dual screen as one virtual screen (and one viewport) where apps might be able to layout at one part.

That isn't typically how operating systems behave.

If apps really want different windows per screen instead, we need to figure out how to handle that.

This spec appears to have figured a lot of that out and has been reviewed by the TAG. I'm just filing issues to clear up some of the details.

On the other hand I have seen new laptops that have an actual screen in the trackpad, but in order for developers to actually use that they would need to know that it is a track pad screen and not just another external display.

Is window placement typically possible in those situations?

@kenchris
Copy link

Good point that some of this can already be inferred today, thought maybe not in a reliable way as it does require the user to have windows from the same origin on different screens, which is not always the case.

I really would like to better understand the privacy and security issues associated with exposing this info, as well as what is already exposed today - or which can be inferred in certain cases.

I also think that there is a difference between screens being internal or external

@jakearchibald
Copy link
Author

There is a bit for that https://webscreens.github.io/window-placement/#dom-screeninfo-internal, but doesn't seem documented.

@kenchris
Copy link

That isn't what this spec is about.

That might be, but it has been one of the feedback we have given and what we have heard from elsewhere - this spec and the foldables specs needs to be able to work together in harmony.

These things are changing rapidly. Laptops are getting multiple screens (many models in the pipeline for the coming years) and even external ones are still being connected.

That isn't typically how operating systems behave.

Those things are changing :-) I have insights that I unfortunately cannot share here. But a public example is the Surface Duo that works exactly that way, as well as the postponed Surface Neo.

Is window placement typically possible in those situations?

As you can attack external displays and projectors, yes it is still very relevant.

@jakearchibald
Copy link
Author

jakearchibald commented Sep 15, 2020

Is window placement typically possible in those situations?

As you can attack external displays and projectors, yes it is still very relevant.

I meant, can you place a browser window on your trackpad?

@kenchris
Copy link

I meant, can you place a Chrome window on your trackpad?

To be honest, I am not sure at this point, but it might very well be possible or at least in the future. @darktears might know

@jakearchibald
Copy link
Author

This spec is about aiding placement of browser windows on auxiliary screens.

@kenchris
Copy link

kenchris commented Sep 15, 2020

I meant, can you place a Chrome window on your trackpad?

Just checked, you can indeed. It can be used as an auxiliary screen - the new ASUS laptops allow that

@jakearchibald
Copy link
Author

Nice! If general window placement is possible there, then it seems like this spec has it covered.

@kenchris
Copy link

kenchris commented Sep 15, 2020

Microsoft proposed the Window Segmentation API (@zouhir), but it is actually more a screen segmentation API, so I wonder whether a screen could have a "hasSegments" property and a getter for those. We might not consider that external displays have folds and segments right now, as on macOS you can use an iPad as external display, so I could imagine a future where you would use your Surface Duo (or Neo) as an external display for your main PC/laptop.

OK, filed an issue for that

@rainke
Copy link

rainke commented Nov 23, 2020

can I know if the screen is mirroring

@michaelwasserman
Copy link
Member

@rainke: Currently, only the source screen is exposed and there's no property indicating that it's mirrored.
This is mentioned briefly in Additional Explorations, but we could more clearly define this behavior in the explainer/spec.
Feel free to file a separate issue, with your use cases, if exposing more mirroring information would be valuable.

@michaelwasserman
Copy link
Member

Our updated proposal in EXPLAINER.md should address the concerns raised here, and CHANGES.md describes most changes from the earlier proposal; please take a look. I'm going to close this issue, but feel free to reopen it or file other issues as appropriate. Thank you for your advice and patience!

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

5 participants