-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 modal focus #6278
Fix modal focus #6278
Conversation
@@ -445,10 +444,9 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> = | |||
|
|||
const baseModalStyle = { ...style, ...modalStyle }; | |||
|
|||
// Sets `display` always block when `animation` is false | |||
if (!animation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not spot the difference in term of animation. I don't know why display was not always set to block.
You can slow down the animation with this (3s instead of 150ms):
.fade {
transition: opacity 3s linear;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the logic was supposed to follow upstream bootstrap, although theirs appear to set display block after the animation finishes. I think the issue may be that we're running the autoFocus
code in @restart/ui
too early? Maybe we should be running it during one of the transition callbacks when animation is enabled.
cc @jquense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the algorithm they are using:
// https://github.com/twbs/bootstrap/blob/v5.0.0/js/src/modal.js#L125-L129
scrollBarHide();
document.body.classList.add('modal-open');
///
// https://github.com/twbs/bootstrap/blob/v5.0.0/js/src/modal.js#L144
// https://github.com/twbs/bootstrap/blob/v5.0.0/js/src/util/backdrop.js#L46-L54
reflow(modalBackdrop);
modalBackdrop.classList.add('show');
await emulateAnimation(modalBackdrop);
///
// https://github.com/twbs/bootstrap/blob/v5.0.0/js/src/modal.js#L144
// https://github.com/twbs/bootstrap/blob/v5.0.0/js/src/modal.js#L243-L261
==> modal.style.display = 'block';
reflow(modal);
==> modal.classList.add('show');
enforceFocus();
///
I think line modal.style.display = 'block'
does not change anything because of
// https://github.com/twbs/bootstrap/blob/v5.1.3/scss/_transitions.scss#L4-L6
.fade:not(.show) {
opacity: 0;
}
Line modal.classList.add('show')
cancels opacity: 0
.
I think they use display: none
because
<div class="modal fade"><div class="modal-content">...</div></div>
is already inside their DOM even if the modal is not displayed.
In React, <div class="modal fade">...</div>
is added to the DOM only when the modal is displayed with prop show={true}
via createPortal()
.
Check https://getbootstrap.com/docs/5.1/components/modal/ and remove display: none
from .modal { ...; display: none; ...; }
, you will see that you are unable to click anywhere on the page because of
.modal-dialog {
...;
pointer-events: none;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading https://developer.mozilla.org/en-US/docs/Web/CSS/display:
display: none
Turns off the display of an element so that it has no effect on layout (the document is rendered as though the element did not exist). All descendant elements also have their display turned off. To have an element take up the space that it would normally take, but without actually rendering anything, use the visibility property instead.
That's why display: none
is not compatible with autoFocus
on an input inside children.
So the issue may be that we're running the autoFocus code in @restart/ui too early? in my opinion won't fix this issue.
Check https://bugzilla.mozilla.org/show_bug.cgi?id=569955: elements with display: none; are not focusable. (they even have added a test for this :-) Add a test to make sure that autofocus on input elements does not work if they don't have a frame)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related Bootstrap issues: twbs/bootstrap#12525, twbs/bootstrap#22402
<Form.Control | ||
type="email" | ||
placeholder="name@example.com" | ||
autoFocus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoFocus
attribute on an input to demo the behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems ok. honestly i don't remember why we needed this style override originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks alright to me
btw, each time I modify Modal.tsx, the documentation refreshes (good) but it freezes my Chrome tab: I have to open a new tab each time 😱 |
Thanks for making this fix @tkrotoff |
Fix for #5102