Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

fix: Remove auto focus from Modal Content #128

Merged
merged 5 commits into from
Aug 4, 2020

Conversation

Pr1ncek
Copy link
Contributor

@Pr1ncek Pr1ncek commented Jun 29, 2020

📃 Summary

  1. When the Modal opens, it automatically sets focus to the first element within the ModalContent.
    This behavior can be unwanted sometimes. See discussion within the Asana ticket for more context.

Solution

  1. We add a tabindex to the Modal component, so it does not set focus to the first element within the modal until the user presses tab.

Important Note
If you have a form within a modal and you want the first input to auto focus when the modal opens, then you have to pass the auto focus prop to the Input itself.
As shown here. https://github.com/heydoctor/molekule/blob/master/src/Modal/Modal.example.js#L39

🏷 Asana Task

https://app.asana.com/0/1172482803331797/1176612299695449/f

QA

  1. Run Molekule story book locally.
  2. Check out the No Auto Focus example within the story book

Expect
When you open this modal, none of the elements should have focus until you press tab.

I will remove this example before merging because I don't really think this needs to be an entirely separate example in our story book.

📈 Risk Analysis

  • Does this affect patients?
  • Does this affect clinical?
  • Does this mutate data?
  • Does it touch PHI?
  • Does it touch payments?
  • Does it update Node packages?

@Pr1ncek Pr1ncek requested review from erikshestopal and gerrymi June 29, 2020 23:23
@Pr1ncek Pr1ncek marked this pull request as draft June 29, 2020 23:45
2. Use this prop to auto focus the first element within the modal
@Pr1ncek Pr1ncek marked this pull request as ready for review June 30, 2020 00:26
@Pr1ncek Pr1ncek marked this pull request as draft July 2, 2020 19:38
@Pr1ncek Pr1ncek marked this pull request as ready for review July 2, 2020 19:39
Copy link
Contributor

@kylealwyn kylealwyn left a comment

Choose a reason for hiding this comment

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

Could we add another Modal story with no input and some buttons to verify?

transitionState={state}
onClick={handleContentClick}
aria-modal="true"
tabIndex={!autofocus && 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need this. The input in the Modal stories still autofocuses without setting to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pr1ncek Pr1ncek marked this pull request as draft July 9, 2020 16:51
@Pr1ncek Pr1ncek marked this pull request as ready for review July 22, 2020 23:05
@Pr1ncek
Copy link
Contributor Author

Pr1ncek commented Jul 22, 2020

Could we add another Modal story with no input and some buttons to verify?

I have added an example with an input included to show that even if you have an input, the modal won't auto focus it.
unless you specifically write <Input autofocus />
https://github.com/heydoctor/molekule/blob/master/src/Modal/Modal.example.js#L39

@Pr1ncek Pr1ncek requested review from kylealwyn, schlegz and a team July 22, 2020 23:08
Copy link
Contributor

@kylealwyn kylealwyn left a comment

Choose a reason for hiding this comment

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

Looks great @Pr1ncek! Could we remove the outline on ModalContent?

CleanShot 2020-07-23 at 09 19 52@2x

@Pr1ncek
Copy link
Contributor Author

Pr1ncek commented Jul 24, 2020

Looks great @Pr1ncek! Could we remove the outline on ModalContent?

CleanShot 2020-07-23 at 09 19 52@2x

Behavior of the Basic Modal has an outline already. So it takes four tabs to cycle through the elements. 1 input, 2 buttons, and the Modal itself.
modal-basic

There are two approaches we can take here.

  1. We can set outline: none, but the modal will still be focusable. you just wont see an outline. So it will still take 4 tabs to cycle through the elements.

  2. Better approach I think is to set tabIndex to -1, so the modal is not a focusable element. Only the input, and the 2 button are focusable.
    Looks like this
    chrome-capture -1 tab index

@kylealwyn
Copy link
Contributor

@Pr1ncek Will approach #2 still support the core requirement? If so, seems ideal.

@Pr1ncek
Copy link
Contributor Author

Pr1ncek commented Jul 24, 2020

@Pr1ncek Will approach #2 still support the core requirement? If so, seems ideal.

Had to go with approach 1, outline: none
Setting tab index to -1, automatically focuses the first element in the modal and that is the original issue we are trying to solve.

@Pr1ncek Pr1ncek requested a review from kylealwyn August 3, 2020 23:58
Copy link
Contributor

@kylealwyn kylealwyn left a comment

Choose a reason for hiding this comment

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

works great!

@Pr1ncek Pr1ncek merged commit 322c054 into master Aug 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/remove-unwanted-focus-from-modal-content branch August 4, 2020 21:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants