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

[Popup] multiple popups colliding? #433

Closed
getify opened this issue Jan 22, 2021 · 8 comments
Closed

[Popup] multiple popups colliding? #433

getify opened this issue Jan 22, 2021 · 8 comments
Assignees

Comments

@getify
Copy link

getify commented Jan 22, 2021

The explainer suggests that only one popup can be visible at a time.

What happens when one popup is currently showing and the show() method is called on another popup element/anchor? Does the first one automatically close, or is the second one ignored?

If the first one is auto-closed, does this count as "light dismissal" (even though it might be purely by script and not user interaction) in terms of the hide event firing? Or if the second one is just ignored, is there a way to detect that your popup didn't actually open?

Speaking of that, are there any other cases/conditions where you can try to open a popup and it might fail? Can you detect those failures if so, like with a true or false being returned from the show() call?


I'm assuming the former, that the first popup is closed when the second one starts to open. That seems most natural.

However, this concerns me in the potential for DOS-like behavior. Can't "malicious" code (like ads, browser extensions, etc) interfere with, or even completely suppress, a site's normal popups (like select boxes, for example) by trying to popup a hidden (or even hijacking/clickjacking) popup and thus replace the site's intended popup?

Would it be useful to have default behavior be that a popup will not allow itself to be closed by another popup trying to open... but that you could opt your popup into "auto-closing" behavior with an additional attribute, in case you prefer that on your site? Or the reverse: default to auto-closing but allow opting out of that with an attribute?


Also, is show() sync or async? Like, could my script immediately detect/manipulate the popup after show() was called, or do you have to wait briefly (or poll) to be sure? If async, can show() return a promise that's fulfilled when it's completely opened (and potentially rejected if there's any cases where popup opening may fail)?

@mfreed7
Copy link

mfreed7 commented Mar 29, 2021

Thanks for the comments here. So the current explainer (and Chromium implementation) has this behavior:

  1. The first popup is shown.
  2. The second popup has .show() called. At this point:
    1. Any open popups (#1 in this case) are closed, in LIFO order, synchronously.
    2. The new popup is made visible, synchronously.
    3. 'hide' events from any closed popups are fired at microtask timing.

You are correct that malicious iframes could indeed close visible popups this way, or spoof a site's popups with their own. I'm not sure of a good mitigation strategy here. @melanierichards @gregwhitworth

P.S. Nice profile picture.

@BoCupp-Microsoft
Copy link
Contributor

You are correct that malicious iframes could indeed close visible popups this way, or spoof a site's popups with their own. I'm not sure of a good mitigation strategy here.

The top layer stack is per document, so I don't think actions taken in an iframe need to impact another document's top layer UX.

Light dismissal includes loss of focus, so I wouldn't expect a document that isn't active to be able to display a popup. I'm thinking that helps ensure correct UX so that transient UX like a popup wouldn't be displayed in multiple documents at once.

We have some related language in the explainer about this here, but we could be more explicit about these types of cross document interactions.

Does that sufficiently mitigate the "iframe popup DOS"?

@domenic
Copy link

domenic commented Mar 29, 2021

You are correct that malicious iframes could indeed close visible popups this way, or spoof a site's popups with their own. I'm not sure of a good mitigation strategy here.

Wait, what is the X in "one popup per X"? I was assuming it'd be Document, but if iframes can interfere, then is it... the entire browser? The top-level browsing context? The browsing context group?

If it's Document, then I think the concern is largely mitigated, as malicious code which has access to your DOM (to insert and show a <popup>) can already do all sorts of terrible things.

Light dismissal includes loss of focus, so I wouldn't expect a document that isn't active to be able to display a popup. I'm thinking that helps ensure correct UX so that transient UX like a popup wouldn't be displayed in multiple documents at once.

Note that all documents have indepdent "currently focused area"s. That is, document.activeElement and someElement.focus() work for all documents, not just the one the user most-recently interacted with. (They even work for documents in bfcache!) So I'm not sure focus is the most relevant thing here. Personally I'd expect any document to be able to display a popup, independently, just like (as you note) their top layers are independent.

@BoCupp-Microsoft
Copy link
Contributor

Wait, what is the X in "one popup per X"? I was assuming it'd be Document.

Yes it is document.

Note that all documents have independent "currently focused area"s. That is, document.activeElement and someElement.focus() work for all documents, not just the one the user most-recently interacted with.

There's only one active document though. If focus leaves that active document, because the browser or tab or document lost focus, then I expect its popups to be dismissed. So the active document seems relevant in that we should answer the question what happens when show() is called if you aren't the active document. I'm thinking nothing happens.

Let me know if you disagree with any of that thinking.

@domenic
Copy link

domenic commented Mar 29, 2021

There's only one active document though.

No, that's not correct. There is one active document per browsing context, but there are many many browsing contexts. (One for each window, and one for each iframe.) The inactive documents are just the documents in bfcache.

@domenic
Copy link

domenic commented Mar 29, 2021

In general any kind of way in which documents can affect each other is pretty unusual on the web platform, and poses some security and privacy issues. I'd definitely suggest sticking to everything being document-scoped, and not giving any particular document special treatment.

@mfreed7
Copy link

mfreed7 commented Mar 29, 2021

Sorry, and thanks @BoCupp-Microsoft and @domenic . I'm not sure where my head was at when I responded above, I had Shadow DOM on my brain at the moment. You are of course correct that <iframe>s will get their own popup stack, and will only be able to display popups within their frame bounding rect. So no problems there.

There will be the possibility that multiple components with separate Shadow DOM can step on each other's popups, but as @domenic pointed out, they already have DOM access so anything is already possible.

There's only one active document though. If focus leaves that active document, because the browser or tab or document lost focus, then I expect its popups to be dismissed. So the active document seems relevant in that we should answer the question what happens when show() is called if you aren't the active document. I'm thinking nothing happens.

This is an interesting point. I agree that if the browser (tab/document/etc) loses focus, its popups should close. That feels like "light dismiss". Given @domenic's points above, this might be moot. But it would be more surprising if non-focused documents (e.g. a non-focused <iframe> within a focused main document) can't show popups. From the user's point of view, there are no iframes, so this might feel mysterious. Sounds again like this part is moot though, so probably no issue.

@melanierichards
Copy link
Contributor

Thanks everyone for the discussion!

I've opened #318 and #319 on Open UI, and will close this issue as a result. Please feel free to chime in on those issues, or to open anything in the Open UI repo that I might've missed and which feels unresolved. Thanks!

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

6 participants