Fix ConfirmationModal to focus on positive confirm button#945 #949
Conversation
| <div | ||
| className="p-confirmation-modal" | ||
| ref={modalRef as RefObject<HTMLDivElement>} | ||
| > | ||
| {children} | ||
| </Modal> | ||
| <Modal |
There was a problem hiding this comment.
The additional div wrapper with the unused p-confirmation-modal class is unnecessary: we can set the ref to the Modal directly - which is a div itself.
There was a problem hiding this comment.
Do you mean passing/forwarding the current ref to Modal? Since Modal already has a ref on itsdiv.
There was a problem hiding this comment.
Yes, forwarding the ref should work in this case - but now that you mention it, I wonder whether it would make more sense to apply the changes proposed in this PR to the Modal itself, since we already have the ref there, which we're already using for focusing purposes.
However, this proposed change requires UX input, as I'm not sure that focusing the positive button when we have one is what we want to do. As a general guideline, I've been told that we want to keep the least destructive option focused by default, which would still be the "Cancel" button then.
There was a problem hiding this comment.
I agree that applying these changes in Modal would make more sense. Since this change requires UX input and may not be the least destructive option, should I set these changes on pause then?
There was a problem hiding this comment.
Sure, let's wait some feedback from them first.
|
@britneywwc Thanks for the suggestion. Can you explain a bit why you think such change is an improvement? Our current focus rules with Modal component we are following the accessibility recommendations from https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/: 
When opening modal the first focusable element is focused by default, and it's recommended for it to be a button that closes the modal (so it's easy to go back if it was opened by mistake). It's even more important if the action on the modal is destructive. And with ConfirmationModal it seems by definition we ask user for confirmation because the action they are about to perform is hard or impossible to revert. In such case it's recommended for default focus NOT to be the main action, but cancel action, so it's easier to cancel than perform the action by mistake. So from accessibility point of view, I don't think we want to focus on primary button by default. Of course, these are not strict rules and there may be situations when focusing on something else will not have bad consequences. So it may be worth considering updating the Modal component, so that the default focused can be changed, but it should be used with caution and I don't know if we have currently a use case for such feature. |
|
@bartaz Thanks for your input. Initially I thought this change would be necessary due to users' expectations when they are prompted with a "positive" Confirm button. Although after reading the information you have shared, I agree that it is a much safer option to stay focused on a non-destructive option. If there is currently no use case for such feature then I will close this PR for now. |
|
Thanks @britneywwc, it's always good to have such discussions, so if you have any other ideas for improvements feel free to propose them as issues or PRs! |
Done
QA
Storybook
To see rendered examples of all react-components, run:
QA in your project
from
react-componentsrun:Install the resulting tarball in your project with:
QA steps
confirmButtonAppearancetopositive/negative/neutral. Click enter to verify which buttons are clicked.Fixes
Fixes: #945