-
Notifications
You must be signed in to change notification settings - Fork 3k
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
prevent initial focus trap while has a focused element. #44932
prevent initial focus trap while has a focused element. #44932
Conversation
delayInitialFocus: CONST.ANIMATED_TRANSITION, | ||
// We don’t want to override autofocus while there is a focused element. | ||
initialFocus: (containers) => { | ||
const hasFocusedElement = containers?.some((container) => container.contains(document.activeElement)); |
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.
const hasFocusedElement = containers?.some((container) => container.contains(document.activeElement)); | |
const isFocusedElementWithinFocusTrapContainer = containers?.some((container) => container.contains(document.activeElement)); |
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've updated
patches/focus-trap+7.5.4.patch
Outdated
-var delay = function delay(fn) { | ||
- return setTimeout(fn, 0); | ||
+var delay = function delay(fn, delayTime = 0) { | ||
+ return setTimeout(fn, delayTime); |
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 think we need to keep the extra setTimeout here and add our timeout. After testing I found out that document.activeElement does not return the correct focused element.
+ return setTimeout(fn, delayTime); | |
+ return setTimeout(() => setTimeout(fn, delayTime), 0); |
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.
After the component unmounts, the focus trap will deactivate, so the default active element becomes the document. So I don't think we need to add another timeout inside the existing timeout
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.
Can you please elaborate what does this has to do with focus deactivation? What I am referring to is the fact that if you log document.activeElement
you will not get the correct focused element as we are doing this too early
Screen.Recording.2024-07-08.at.4.42.36.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.
We have a missing delayInitialFocus
time; it should equal CONST.ANIMATED_TRANSITION + CONST.ANIMATION_IN_TIMING
. I’ll update it with the timing animation in accordingly.
Screen.Recording.2024-07-08.at.23.18.32.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.
Updated timing 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.
The correct timing is 300ms (CONST.ANIMATED_TRANSITION
). CONST.ANIMATION_IN_TIMING
is not related to the screen transition.
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.
The correct timing is 300ms (CONST.ANIMATED_TRANSITION). CONST.ANIMATION_IN_TIMING is not related to the screen transition.
About this change, we need to extend ANIMATION_IN_TIMING. This is because useAutoFocus also runs after ANIMATED_TRANSITION. In that case, input focusing and getting initial focus from the focus trap will run simultaneously, so sometimes the focused element is not 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.
Given that this already had it's own setTimeout
even when no timeout is actually required, we should just add another setTimeout
. The former is to fix whatever the library was trying to fix and the later is for our actual wanted delay.
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'll try your suggestion
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 have updated
patches/focus-trap+7.5.4.patch
Outdated
-var delay = function delay(fn) { | ||
- return setTimeout(fn, 0); | ||
+var delay = function delay(fn, delayTime = 0) { | ||
+ return setTimeout(fn, delayTime); |
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.
Can you please elaborate what does this has to do with focus deactivation? What I am referring to is the fact that if you log document.activeElement
you will not get the correct focused element as we are doing this too early
Screen.Recording.2024-07-08.at.4.42.36.PM.mov
@mountiny Yes it will fix that issue |
} | ||
return undefined; | ||
}, | ||
setReturnFocus: (element) => { |
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 not return focus to the initial screen if we already have a focused element, e.g. in ReportScreen after closing RHP the Composer should remain focused
Screen.Recording.2024-07-09.at.12.17.06.AM.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.
@s77rt I’m still working on setReturnFocus
. Do you mean your suggestion also checks if there’s a focused element to skip setReturnFocus
?
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.
Yes!
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.
When a component using focus-trap-react
unmounts, it calls deactivateTrap, which triggers focusTrap.deactivate. This subsequently calls getReturnFocusNode in focus-trap
. Therefore, we can’t get the containers for the next screen because when Screen B unmounts (after navigating from Screen A to Screen B), it sets the return focus node from Screen B’s context.
As a result, we can’t retrieve the containers for Screen A when Screen B unmounts. Instead, we can check if the active element is the default (document body). If it is, we can return false to ensure the focus trap does not interfere with the focus transition to Screen A. Otherwise, we keep the current focused element.
I have updated the code, and it covers the issue you mentioned above.
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.
Works for me but let's also check if document.activeElement
is null
. In that case we want to return the focus as well
patches/focus-trap+7.5.4.patch
Outdated
-var delay = function delay(fn) { | ||
- return setTimeout(fn, 0); | ||
+var delay = function delay(fn, delayTime = 0) { | ||
+ return setTimeout(fn, delayTime); |
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.
The correct timing is 300ms (CONST.ANIMATED_TRANSITION
). CONST.ANIMATION_IN_TIMING
is not related to the screen transition.
// We don't want to ovverride autofocus on these screens. | ||
initialFocus: () => { | ||
if (screensWithAutofocus.includes(activeRouteName)) { | ||
delayInitialFocus: CONST.ANIMATED_TRANSITION + CONST.ANIMATION_IN_TIMING, |
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.
delayInitialFocus: CONST.ANIMATED_TRANSITION + CONST.ANIMATION_IN_TIMING, | |
delayInitialFocus: CONST.ANIMATED_TRANSITION, |
buttonRef={(ref) => { | ||
inputCallbackRef(ref); | ||
}} |
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.
buttonRef={(ref) => { | |
inputCallbackRef(ref); | |
}} | |
buttonRef={inputCallbackRef} |
src/hooks/useAutoFocusInput.ts
Outdated
import {InteractionManager} from 'react-native'; | ||
import CONST from '@src/CONST'; | ||
import * as Expensify from '@src/Expensify'; | ||
|
||
type FocusRef = View | TextInput | null; |
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.
type FocusRef = View | TextInput | null; | |
type FocusElement = View | TextInput | null; |
NAB
if (screensWithAutofocus.includes(activeRouteName)) { | ||
delayInitialFocus: CONST.ANIMATED_TRANSITION, | ||
initialFocus: (focusTrapContainers) => { | ||
const hasFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement)); |
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.
const hasFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement)); | |
const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement)); |
NAB
Bug: After pressing Enter selecting the participant, the back button is focused initially Screen.Recording.2024-07-14.at.7.23.33.PM.mov |
@s77rt At the |
@suneox We can't use |
@s77rt If we don't extend checkInitialFocus: (focusTrapContainers) => {
return new Promise((resolve) => {
InteractionManager.runAfterInteractions(() => {
const hasFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
resolve(!hasFocusedElementInsideContainer);
});
});
} |
@suneox How this works with the search input? Let's follow the same approach there Screen.Recording.2024-07-15.at.8.53.16.PM.mov |
@s77rt The text input from BaseSelectionList also focus after timeout App/src/components/SelectionList/BaseSelectionList.tsx Lines 540 to 552 in fb9ce79
So, do you agree to implement my alternative solution to create a promise function and revert the current change, or should we just extend the delay time for the current code and leave a note explaining the reason for it? |
@suneox What I meant is to use the same focus approach used in the participants selection and use it in the confirmation page (do not use useAutoFocusInput) |
I got it, that is the reason my initial change use |
Reviewer Checklist
Screenshots/VideosAndroid: NativeBuild issue Android: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.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.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.0.8-1 🚀
|
Hey @s77rt @suneox @chiragsalian, I think this PR might have introduced this issue (focus is skipping the login input field on the homepage). It's not a blocker, but could you please take a look? Thanks |
@Julesssss I'd like to confirm issue #45571 occurs by this PR because the input on |
🚀 Cherry-picked to staging by https://github.com/Beamanator in version: 9.0.8-3 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.9-5 🚀
|
This PR caused a regression. The button has the |
Details
Fixed Issues
$ #43662
$ #44956
$ #44745
PROPOSAL: #43662 (comment)
Tests
Same QA Steps
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
MacOS: Chrome / Safari
Issue 43662:
Screen.Recording.2024-07-06.at.15.04.51.mov
Issue 44745:
Screen.Recording.2024-07-11.at.21.28.23.mov
MacOS: Desktop
Screen.Recording.2024-07-06.at.14.50.31.mov
Android: Native
Screen.Recording.2024-07-06.at.14.56.49.mov
Android: mWeb Chrome
Screen.Recording.2024-07-06.at.14.55.29.mov
iOS: Native
Screen.Recording.2024-07-06.at.14.52.27.mov
iOS: mWeb Safari
Screen.Recording.2024-07-06.at.14.49.23.mov