fix: enable tab on IOU request start page #45982
Conversation
melvin-bot
bot
requested review from
rayane-djouah
and removed request for
a team
|
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-08-07.at.12.30.58.PM.movAndroid: mWeb ChromeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-07.at.12.23.12.mp4iOS: NativeUntitled.mp4iOS: mWeb SafariScreen.Recording.2024-08-07.at.12.23.25.PM.movMacOS: Chrome / SafariScreen.Recording.2024-08-07.at.12.11.05.PM.movMacOS: DesktopScreen.Recording.2024-08-07.at.12.20.01.PM.mov |
|
@dominictb - We need to fix the bug in the start chat page and in all places where we use the tab selector. perhaps we can extract common code |
|
@rayane-djouah I'll check and get back to you on that. |
@rayane-djouah do you think we need to get someone's opinion on this: If we tab around like this there's blurry blue border around that tab item? |
I think this is already the behavior in the start chat page in the production app and should be out of scope for this PR 
|
|
@rayane-djouah sorry, but one final question: In |
|
@dominictb - I think we should make it consistent with the IOU start page. Therefore, it should be included. |
|
working on this now! |
|
got some issue while updating the |
|
|
||
| const addContainer = useCallback((container: FocusTrapContainerElement) => { | ||
| const containerAsHTMLElement = container as unknown as HTMLElement; | ||
| const removeContainer = () => setContainers((prevContainers) => prevContainers.filter((c) => c !== container)); |
There was a problem hiding this comment.
We might not need this. I will delete this later once this PR is finished reviewing.
|
@rayane-djouah pls re-check. I'll update the evidence later. |
|
TS checks are failing |
|
Checking now. @rayane-djouah I think you can still continue the review, I'll update the TS error in a bit. |
src/components/FocusTrap/FocusTrapContainerElement/FocusTrapContainerElementProps.ts
Outdated
Show resolved
Hide resolved
| if (typeof r === 'function') { | ||
| r(node); | ||
| } else if (r) { | ||
| // eslint-disable |
There was a problem hiding this comment.
| // eslint-disable |
| @@ -253,6 +253,7 @@ function WorkspaceNewRoomPage({policies, reports, formState, session, activePoli | |||
| includePaddingTop={false} | |||
| shouldEnablePickerAvoiding={false} | |||
| testID={WorkspaceNewRoomPage.displayName} | |||
| focusTrapSettings={{active: false}} | |||
There was a problem hiding this comment.
In /new/(chat|room) page, we have 3 focus traps: The parent focus trap in here and 2 child focus traps in each NewChatPage and WorkspaceNewRoomPage, and all three are activated. So in this case, the parent focus trap will be ignored, i.e: The header button + tab selector items are not in the focus trap.
Since the expected result is to include both the header button and tab bar items in the focus trap, We'll need to deactivate 2 child focus traps in respective component NewChatPage and WorkspaceNewRoomPage
There was a problem hiding this comment.
Please add that context to the code so the explanation is apparent for who sees it next.
About this one, it is because when the tab selector item is active, they have no role, hence the |
|
On it now! |
move tab bar with focus trap inclusion inside onyx tab navigator
|
@rayane-djouah done! |
There was a problem hiding this comment.
I've long been confused with the tab navigator and found it to be a very difficult component to use, modify, and maintain. My hats off to you for being brave enough to work on it. Now, with the inclusion of the focus traps, I think it is even more difficult to understand and work on, which has me worried.
Instead of us moving toward something more simple, we are diving deeper into complexity, and I would be hard pressed to come along and continue to work on this component.
At the very least... Would you consider adding a MD file to /contributingGuides that explains the focus trap components, how they should be used, and most importantly why they should be used?
If you were to rewrite the tab selector from scratch today, I'm curious what ideas you have to make something that is more simple and easier to maintain.
| return ( | ||
| <View | ||
| ref={(node) => { | ||
| const r = ref; |
There was a problem hiding this comment.
Can ref be referenced directly? If so, please remove this unnecessary variable.
There was a problem hiding this comment.
Since we have ref.current = node, we'll need to assign to a local variable to avoid eslint error
| /** Callback to register the focus trap container elements of the current active tab */ | ||
| onActiveTabFocusTrapContainerElementChanged?: (containerElement: HTMLElement | null) => void; | ||
|
|
||
| /** Callback to register the focus trap container elements of the tab bar */ |
There was a problem hiding this comment.
Would you please add a little more detail to these comments? Instead of only describing what the callback are, I like to include when they are triggered and why someone would want to use them. For example:
This callback is triggered when the container element receives or loses focus. Use this when...
| export const TopTab = createMaterialTopTabNavigator(); | ||
| const TopTab = createMaterialTopTabNavigator(); | ||
|
|
||
| const TabFocusTrapContext = React.createContext<(tabName: string, containerElement: HTMLElement | null) => void>(() => {}); |
There was a problem hiding this comment.
Please add a comment to describe this variable and why it is there.
| }: OnyxTabNavigatorProps) { | ||
| const [focusTrapContainerElementMapping, setFocusTrapContainerElementMapping] = useState<Record<string, HTMLElement>>({}); | ||
|
|
||
| const setTabFocusTrapContainerElement = useCallback((tabName: string, containerElement: HTMLElement | null) => { |
There was a problem hiding this comment.
What is the purpose of this callback?
| ); | ||
|
|
||
| useEffect(() => { | ||
| onActiveTabFocusTrapContainerElementChanged?.(selectedTab ? focusTrapContainerElementMapping[selectedTab] : null); |
There was a problem hiding this comment.
Please add a comment to describe why this is necessary.
| const [tabBarContainerElement, setTabBarContainerElement] = useState<HTMLElement | null>(null); | ||
| const [activeTabContainerElement, setActiveTabContainerElement] = useState<HTMLElement | null>(null); | ||
|
|
||
| const containerElements = useMemo(() => { |
There was a problem hiding this comment.
Please add a comment explaining why both of these callbacks are necessary.
| @@ -253,6 +253,7 @@ function WorkspaceNewRoomPage({policies, reports, formState, session, activePoli | |||
| includePaddingTop={false} | |||
| shouldEnablePickerAvoiding={false} | |||
| testID={WorkspaceNewRoomPage.displayName} | |||
| focusTrapSettings={{active: false}} | |||
There was a problem hiding this comment.
Please add that context to the code so the explanation is apparent for who sees it next.
|
I discussed this PR internally with a few folks (like @AndrewGable and @marcaaron) and I think we're all a bit concerned at the amount of changes in this PR to fix the reported bug. @dominictb I think you had some other proposal ideas that were a more simple solution that might be worth exploring. I think one of them was only rendering the active tab. @rayane-djouah found that this leads to a bit of a delay as you navigate between tabs due to the re-rendering (it's mostly notable on the map view).
|
|
The app uses a sliding transition when you long press and swipe the pages, a gesture commonly used on mobile devices. Here is a video of the animation on staging: Screen.Recording.2024-08-09.at.12.45.08.AM.movIf we render only the active tab or use the CSS visibility property to hide other tabs, we will disrupt this animation. Additionally, there will be a delay when navigating between tabs. Here is the result using the CSS visibility solution: Screen.Recording.2024-08-09.at.12.44.04.AM.movAnd the result of rendering only the active tab: Screen.Recording.2024-08-09.at.12.50.15.AM.movHowever, with the changes from this PR, there are no delays and the animation still works correctly. |
|
@rayane-djouah, maybe we can remove the CSS visibility hidden when the dragging starts? |
|
Sorry it's quite a lot of information for me to digest. Give me some time to get back to you. TY! |
|
@tgolen sorry for late reply, but here's my take on this issue:
I would try to give my best to explain what I introduced in the PR. We can discuss more if you have any further concerns about the approach, implementation,... here or on Slack. TY! |
There was a problem hiding this comment.
Thank you for your comments in the code an further explanation.
I did some research about the focus traps and I understand them better now, though I'm not really sure they are the best thing for accessibility. It feels to me like focus traps in general break the tab navigation behavior of a browser in such a way that someone who is blind or using non-standard mouse/keyboard inputs might have difficulty. (eg. once you have focus inside the tabs, there is no way to escape out of the focus trap so they cannot navigate to other areas of the page)
Since this is a completely separate concern than the bug you're fixing, I'll go ahead and merge this for now, but just record my thoughts for historical purposes.
|
✋ 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/tgolen in version: 9.0.19-2 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.19-4 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.19-5 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.20-0 🚀
|
|
This PR is failing because of issue #47381 |
|
FYI I believe this was deployed to prod yesterday, from this checklist - #47356 |
Details
Fixed Issues
$ #43719
PROPOSAL: #43719 (comment)
Tests
Tab/Shift TabaroundVerify that the focus jumps around the IOU start page but there's no UI shifting
Offline tests
QA Steps
Tab/Shift TabaroundVerify that the focus jumps around the IOU start page but there's no UI shifting
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
We shouldn't have issue on native app as focus trap is web-only feature
ios-8MB.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web-8MB.mp4
MacOS: Desktop