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

fix: token warning modal stealing focus across frame boundaries #1205

Conversation

moontools-hyperion
Copy link
Contributor

The token warning modal in the Uniswap interface, when embedded in an iframe on a parent page, steals focus from the parent window. This prevents users from interacting with input elements in the parent window.

Simple example of issue:
https://jsfiddle.net/26cms93e/

Steps to reproduce:

  1. Go to https://app.moontools.io/pairs/0x02f14c27037bd30f18a6578590fd40fafd3376ff
  2. Click on the "Swap" tab (contains an iframe pointing to Uniswap interface) and wait for token warning modal to load
  3. Notice that you cannot interact with the "Filter by address" input field above the transaction log. If you close the token warning modal, the input becomes focusable again.

This issue is also present on other DEX data explorer sites. This PR fixes the issue described above.

@vercel
Copy link

vercel bot commented Nov 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/uniswap/uniswap-interface/nahjfj863
✅ Preview: https://uniswap-interface-git-fix-modals-stealing-focus.uniswap.vercel.app

@moodysalem
Copy link
Contributor

moodysalem commented Nov 10, 2020

This is very interesting--is it better fixed in the browser though? Should a child iframe be able to steal focus or blur inputs on the parent page? It seems like a good thing you can't tab outside of the modal while on the Uniswap interface.

style={props}
onDismiss={onDismiss}
initialFocusRef={initialFocusRef}
dangerouslyBypassFocusLock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs on this explicitly say that you should have some replacement focus lock if you enable this, but I don't see any replacement in this PR.

@MicahZoltu
Copy link
Contributor

@MicahZoltu
Copy link
Contributor

IIUC, the browser solution is to disallow the iframe from being able to programmatically set focus at all. Essentially this means that Uniswap should not depend on its ability to control focus for disabling page content. Instead it should disable fields in the app and/or use an overlay.

@moontools-hyperion
Copy link
Contributor Author

moontools-hyperion commented Nov 11, 2020

Should a child iframe be able to steal focus or blur inputs on the parent page?

IMO it should not, but evidently it is possible in some browsers. Afaik there isn't a way to prevent this from the parent page.

Probably need to do what @MicahZoltu suggested i.e. disabling all inputs in uniswap-interface when the modal is open. Is this approach okay? @moodysalem

@moodysalem
Copy link
Contributor

moodysalem commented Nov 25, 2020

IIUC, the browser solution is to disallow the iframe from being able to programmatically set focus at all. Essentially this means that Uniswap should not depend on its ability to control focus for disabling page content. Instead it should disable fields in the app and/or use an overlay.

Wdym by an overlay? I do not like the idea of disabling all other inputs on the page when a modal is open. It will either be mutating the dom of many components outside of react and is likely to break, or require changing tons of components

The browser solution breaks the reach dialog's focus solution to tabbing outside the modal, and that's fine with me--it's not required. I am pretty sure the browser should fix it

Copy link

@luanluvthu luanluvthu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@MicahZoltu
Copy link
Contributor

I do not like the idea of disabling all other inputs on the page when a modal is open.

I'm not a huge fan either, but I don't think there is another option if you want the iframe widget to work correctly and not inappropriately interfere with the hosting page. If you do nothing, people using the iframe have two choices:

  1. Their page breaks when Uniswap has a modal open.
  2. Uniswap modals don't work correctly within the iframe.

It will either be mutating the dom of many components outside of react and is likely to break, or require changing tons of components

I agree that it increases complexity quite a bit. I would not be worried about DOM updates since most (all) of the changes won't trigger a re-layout, just some non resizing style changes. Component updates aren't that expensive (on this order of magnitude) as long as they don't trigger a relayout. I suspect you can mitigate the complexity quite a bit with a good framework (e.g., React).

I am pretty sure the browser should fix it

From what I have seen, no browser considers this a bug. I can appreciate one disagreeing on this, but I think Uniswap should work in the real world, not in a hypothetical future that doesn't align with current trajectory for reality.

@moodysalem
Copy link
Contributor

We need to set crossFrame to false on the underlying focus lock that reach dialog is rendering
theKashey/react-focus-lock#104 (comment)

@moodysalem
Copy link
Contributor

moodysalem commented Dec 2, 2020

@moontools-hyperion are you able to add a replacement focus lock to the Modal component that has crossFrame set to false?

@sharife3
Copy link

sharife3 commented Jan 5, 2021

Any updates on this ?

@moodysalem
Copy link
Contributor

Any updates on this ?

The better fix is described in the above comment (disable only cross frame focus lock) and we would accept any pull request that implements it

Base automatically changed from master to main February 3, 2021 18:54
@moontools-hyperion
Copy link
Contributor Author

Hi @moodysalem, I've created a PR for this at #1326

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

Successfully merging this pull request may close these issues.

5 participants