Address autofocus issue #7711 #7712
Conversation
mapitman
force-pushed
the
autofocus
branch
2 times, most recently
from
9f054e4 to
684a6f5
Compare
|
@mapitman Thanks for the PR (and issue ticket). I do think that moving control of the prop to the top-level Auth component is a good starting point. However, I think there should be a way to provide a priority list of preferred components (keys?) to set an autofocus on first, and then skip for the rest of components if autofocus is set. I know you mentioned that you are not as familiar with React, so perhaps someone else is willing and able to further develop this PR if you are unable to. |
|
@tim-lai I'd be willing to take another go at this. Would you be willing to give me some guidance on how this would work? I'm not quite understanding the use case. Is it that someone may want to set focus to the second textbox in the modal for example? |
|
@mapitman My interpretation of the earlier PR #6483 was that the PR simply added the autofocus to any input elements that could be present (per various individual use cases), without considering if those input elements could appear together. In the case(s) where multiple input elements are present, it seems reasonable that the autofocus should be on the first input element (this PR). So my request is that any proposed solution does NOT hardcode which input element to autofocus, but that it finds the first element rendered to which autofocus should be applied. Let me if know that helps clarify my request. Thanks! |
Find the first input element in the div with class name modal-ux-content and set focus to it.
|
@tim-lai Thanks for the reply. I've made a slight change because I noticed I was using using a class name specific to my use of Swagger UI. I've also rebased my changes on current master. The code will now find the first input element in the modal div (found by using the class name I believe this addresses your point above:
|
I didn't realize this project does not use semi-colons.
| } | ||
|
|
||
| componentDidMount() { | ||
| var inputs = document.getElementsByClassName("modal-ux-content")[0].getElementsByTagName("input") |
There was a problem hiding this comment.
As a vanilla JS solution, this looks good to me. However, thinking about this more, we really should "React-ify" this to not directly change the DOM with the .focus. E.g. use "refs"
I'm thinking would could try adding a ref to className="auth-container" here that would contain multiple current refs. Most likely then also need to add creation of refs within the two maps that generate React element. e.g. something similar to ref={el => inputRef.current[x] = el} .
More info on refs here, and we'll want to be compatible with the possible future use of React hooks.
There was a problem hiding this comment.
"React-ifying" is the part where I'm out of my depth. I'll take a look at the link you provided and see if I can learn just enough to do this. 😸
2 participants
Description
Remove the hard-coded
autoFocusattributes from the auth-related jsx files. Replace with JavaScript to set the focus to the firstinputelement in the auth modal.Motivation and Context
Fixes #7711
How Has This Been Tested?
I tested it with using the swagger.json linked to in the issue. Also the automated test that was added to test the autoFocus still passes after this change.
Screenshots (if appropriate):
Checklist
My PR contains...
src/is unmodified: changes to documentation, CI, metadata, etc.)package.json)My changes...
Documentation
Automated tests
There is an existing test for this.