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 8 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.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.
Let's move this to
libs/focusWithDelay
replacing setTimeout approach. And this will fix edit composer 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.
Actually
libs/focusWithDelay
has an small issue, mobile web also needs delay (especially mobile Safari). If moving the above code tofocusWithDelay
, I plan to remove thedisableDelay
variable.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.
sure, go ahead
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.
We should keep original focus state before opening popover.
We should not change current focus behavior. Please compare this branch build with main or staging.
Only the difference here should be to fix keyboard flash. Other behaviors should keep the same on all platforms.
Screen.Recording.2023-08-16.at.6.03.06.PM.mov
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.
I understand what you mean😄. The current focus behavior is based on the previous discussion and PR , it's not an accidental change. If we ultimately decide to preserve state, the the conditions for determining focus in the solution will certainly also be different 🙂.
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.
@trjExpensify can you please confirm this is the new expected behavior we landed?
I see that composer is always focused upon modal close no matter keyboard was shown or not before opening 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.
Just FYI, for now the focus behavior of attachment modal and emoji modal is consistent. Also we have discussed composer's refocusing behavior in many places, e.g., issue #9252 is also interested in preserving state, issue #15992 is looking for a more overall design. (actually, I personally also prefer preserving state, and have implemented it before, however as I mentioned in the proposal, it will affect many places, so handling it in other issues may be more appropriate 🙂).
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.
To clarify what I was talking about here in the linked comment referenced is the scenario in which we were addressing in the issue.
I agree that should be what we're shooting for here.
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.
@trjExpensify, hi, does this mean that if the keyboard was not shown before opening the attachment modal, the keyboard also should not be shown after closing the modal? If so, I will modify the focus condition later. : )
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.
Correct!
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.
Oh, I see. I originally thought the focus behavior in the PR #15337 was as expected 😂.
Just modified. Please feel free to let me know if there are any differences from expectations. : )
cc @situchan