Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 keyboard flashing while clicking "Add attachment" #23994
fix keyboard flashing while clicking "Add attachment" #23994
Changes from all commits
11717c5
e076446
4ebbd77
7ac72b5
87699e3
98a5c4a
47c05fe
4d0afeb
887427a
a021e0f
bd2b6ad
3dfcf11
767b239
4d165ce
09baf7b
e272a1c
67d3228
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
what's this condition for?
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.
Ensure that the pending promise can also be settled when
coverScreen
isfalse
, becauseonDismiss
will not be emitted in this case. (It only exists in the RNModal
component.)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.
Hey @ntdiary, I know this thread is kinda old, but would appreciate your input:
Why was it necessary to put the
ComposerFocusManager.setReadyToFocus()
logic into bothonDismiss
andonModalHide
(which callshideModal
)?Was there any drawback to just calling it in
hideModal
regardless of thefullscreen
condition? This would mean removing theReactNativeModal.onDismiss
prop below.I'm asking because we've recently discovered another case when the
onDismiss
is not called, this time for a full-screen Attachments modal.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.
@paultsimura, when using the
Modal
component, the invocation ofonModalHide
happens earlier than the destruction of the focus trap, which will cause the composer not gaining focus correctly after the modal is dismissed. :)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.
Thanks @ntdiary. Do you have any particular
Modal
scenario in mind that might break?I tried with only the
hideModal
: emoji picker, attachments modal, the side drawer – everything focuses the composer correctlyThere 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.
@paultsimura, I'm not exactly sure about the new code in the main branch, maybe you can verify the emoji picker in mobile chrome. Additionally, I'm refactoring the modal's refocusing behavior (#29199), so there might be a chance to review the related code again tomorrow. :)
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.
Cool, would your change cover the modal being closed on clicking the browser's "Back" button as well?
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.
Yeah, I added a new
ModalContent
, I feel it should fix that issue.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.
Do we need this logic on web at all?
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.
Yeah, the
Modal
component inreact-native-web
has focus trap. This code, together withInteractionManager.runAfterInteractions
, can also ensure that thefocus
is called safely.This file was deleted.
This file was deleted.
This file was deleted.
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.
This caused a regression #26489. After setting this value to true we had to reset it to false as soon as a menu item is selected otherwise we will focus the composer for a brief period while opening the RHP.