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

input type="file" hidden #61

Closed
arturi opened this issue Dec 26, 2017 · 7 comments
Closed

input type="file" hidden #61

arturi opened this issue Dec 26, 2017 · 7 comments

Comments

@arturi
Copy link

arturi commented Dec 26, 2017

Hi! Thank you for the awesome project!

In my project I’m using hidden file inputs that are triggered when a button is clicked. File input element looks like this: <input class="uppy-Dashboard-input" hidden="true" aria-hidden="true" tabindex="-1" type="file" name="files[]" multiple="true" onchange={this.props.handleInputChange} ref={(input) => { this.input = input }} />, and it was included to the list of focusable nodes by micromodal. I fixed it by changing the input selector to omit hidden:

'input:not([disabled]):not([type="hidden"]):not([hidden])'

I’d like to send a PR, but thought I’d ask first, and also maybe there’s something else that needs to be changed. Add aria-hidden too? My input does have a tabindex="-1" and aria-hidden="true", but still gets treated as focusable, because it’s an input and not type="hidden".

@ghosh
Copy link
Owner

ghosh commented Dec 27, 2017

Hi @arturi - Thanks for reporting this. Here are my thoughts on this:-

  • aria-hidden="true" prevents screen readers from reading the element.
  • tabindex="-1" prevents the node from being focusable
  • hidden="true" is an html5 attribute which should visually hide your element.

Which browser and version are you using? Normally any element with hidden or type=hidden should be hidden by the browser. Micromodal cannot focus on hidden elements.

Now that you mention it, I am not sure if there is any need of :not([type="hidden"]):not([hidden]) in the focusable list since hiding the elements should be onus of the browser.

Additionally, a simple [hidden] { display: none;} should do the trick if the browser does not support this natively.

@arturi @kalpeshsingh thoughts?

@arturi
Copy link
Author

arturi commented Dec 27, 2017

Okay, I did some tests and it turns out the bug occurs when this input I mention is the last “focusable element that should not be focusable“ in the modal.

Demo: https://codepen.io/anon/pen/jYyvMe, try tabbing a few times, notice how focus breaks out of the modal.

If you look in the console, the NodeList that gets logged there includes all the inputs that have hidden and tabindex="-1" in them, and that screws up the math, I think.

@ghosh
Copy link
Owner

ghosh commented Dec 28, 2017

Yup, seems like aria-hidden="true" needs to explicitly excluded from being selected.

'input:not(hidden):not(disabled):not([aria-hidden])'

Problem is this would need to be set for all selectable elements. Seems like globally omiting aria-hidden="true" does not work.

@arturi
Copy link
Author

arturi commented Dec 28, 2017

Shall I send a PR with :not([aria-hidden] in every selectable element? Or shall we think on some other solutions? Could also loop through nodes after querySelectorAll to remove those with hidden attributes, but that might be slower / more verbose code.

@kalpeshsingh
Copy link
Collaborator

In my opinion, adding :not([aria-hidden] in every selectable element should be last option. I will check on this if we can get something clean to fix this problem.

@ghosh ghosh closed this as completed in 4b81bec Jan 13, 2018
@ghosh
Copy link
Owner

ghosh commented Jan 13, 2018

This should be fixed in 0.3.0

@arturi - Would have asked you for a PR, but it was a very small change. So I changed it myself.

@kalpeshsingh There were actually just 4 elements which I needed to add it too. If you find a better way feel free to raise a PR

@arturi
Copy link
Author

arturi commented Jan 17, 2018

Thank you!

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

3 participants