Closed lanitochka17 closed 2 weeks ago
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@rayane-djouah Sorry for the delay here as I was OoO. I've added this to Wave Collect and will update once we have a priority there.
Cool it Melvin π
Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.
^^ Dammit, the sidepane jumped as I was trying to click the project. π
Thank you for the update
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
I imagine a "Polish" doesn't need to be increased, but curious your thoughts @trjExpensify π
@tgolen @OfstadC @rayane-djouah this issue is now 4 weeks old, please consider:
Thanks!
Started a discussion in #expensify-open-source
I imagine a "Polish" doesn't need to be increased, but curious your thoughts @trjExpensify π
Agree.
Tabs disappear when changing from Scan to Distance via SHIFT+TAB
The OnyxTabNavigator will render all 3 tab screens, but the focused tab screen will be visible, and other tab screen will be moved to the left/right depending on the position, being hidden to the user
In IOURequestStartPage
, we are wrapping a ScreenWrapper
with focus trap enabled. Now based on how the focus-trap
library works, the following happens:
All tabbable elements from 3 tabs are included in the focus trap, along side with the header back button. And all of them are in 1 single container group.
Looking into the focus-trap implementation here and here: if the element is in the rear end of a group, focus-trap
lib will move the focus programmatically (see this and this, it basically move the focus to the first/last element of the next/prev group depending on whether we are TAB-ing forward or SHIFT+TAB-ing backward), but if there is no group jumping (we are from one focusable element to the next in the same container group), the focus-trap
will depend on the browser native TAB/SHIFT+TAB
behavior to move the focus for us, unless we explicitly
configured a different key event for moving focus in focus trap (check this)
Hence, if we try tab
, the focus will be on the header back button (the first focusable element in the focus trap), and tab
-ing further won't move the focus anywhere else, because we have already disabled the default TAB/SHIFT TAB
behavior here.
If we try SHIFT+TAB
, the focus will be on the last element in the focus trap, i.e: the Next
button of the third tab Distance
. And as the focus-trap
do this programmatically, it will call the node.focus({preventScroll: false})
here, this will force the Next
button in the Distance
tab, which is currently out of view, scroll into visible port and shifting the UI to the left. Hence, the tabs seem to disappear in this case.
We have 3 options:
We should add a props in ScreenWrapper
to allow disabling the focus trap. I'm not sure if it is intended to have the focus trap in IOURequestStartPage
, but if we don't need it then we should disable it to fix this issue.
We can add a props focusTrapOptionsPassthrough
in ScreenWrapper
, and in IOURequestStartPage
, set
<ScreenWrapper focusTrapOptionsPassThrough={{ preventScroll: false }} />
this will prevents the focus node to scroll into view if it is out of view, making UI shift like described.
IOURequestStartPage
, but we don't want to include focusable element from inactive tabs, we can consider render only the active tab. It can be done by adding a prop to 3 component IOURequestStepAmount
, IOURequestStepScan
, IOURequestStepDistance
:type Props {
onlyRenderWhenFocused?: boolean
}
....
const isFocused = useIsFocused()
if(!isFocused && onlyRenderWhenFocused) return null;
And in here, add onlyRenderWhenFocused=true
for 3 components.
Same idea with option 3 above, but we can initiate the focus trap so that only elements under the active tab are added to the focus trap accordingly.
We can utilize the `containerElements` prop of the focus-trap-react. Check [here](https://github.com/focus-trap/focus-trap-react?tab=readme-ov-file#containerelements). The idea is that we should get the root `View`s of each tab component `IOURequestStepAmount`, `IOURequestStepScan`, `IOURequestStepDistance`, then based on the `selectedTab` to register the `View` as `containerElements` of the focus trap. We should also consider the HeaderWithBackButton and the TabSelector component to be one of the containers.
We can achieve that by following change:
- Define a `useFocusTrapContainers` hook
```
import { useCallback, useState } from "react";
import { UseFocusTrapContainers } from "./type";
const useFocusTrapContainers: UseFocusTrapContainers = () => {
const [containers, setContainers] = useState
@tgolen, @OfstadC, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@dominictb - Thank you for explaining the RCA in such detail! It makes sense to me and is the most clearly explained so far. I think the most accurate solution for this bug, based on what you suggested, is what you described here (if we want to keep tab navigation disabled on this page):
- We can add a props
focusTrapOptionsPassthrough
inScreenWrapper
, and inIOURequestStartPage
, set<ScreenWrapper focusTrapOptionsPassThrough={{ preventScroll: true }} /> // I think you've made a typo here (should be true instead of false)
this will prevents the focus node to scroll into view if it is out of view, making UI shift like described.
However, I still want to explore solutions that exclude focusable elements from inactive tabs to re-enable proper tab navigation on this page. The most relevant solution for now involves using the CSS visibility property, which I think is not optimal (kind of a workaround). Is there a possibility to use focus trap properties for this instead? Have you explored other solutions in this context?
@rayane-djouah I'll explore it today and let you know!
I think I figured out the way to do this, but I'll need some time to test this out. Will get back by tomorrow at most.
https://github.com/Expensify/App/issues/43719#issuecomment-2168110266
@rayane-djouah Updated the below sections:
What changes do you think we should make in order to solve the problem? What alternative solutions did you explore? (Optional)
Is there a possibility to use focus trap properties for this instead?
I'll update this first thing tomorrow.
Is there a possibility to use focus trap properties for this instead? Have you explored other solutions in this context? @rayane-djouah
We can utilize the containerElements
prop of the focus-trap-react. Check here. The idea is that we should get the root View
s of each tab component IOURequestStepAmount
, IOURequestStepScan
, IOURequestStepDistance
, then based on the selectedTab
to register the View
as containerElements
of the focus trap. We should also consider the HeaderWithBackButton and the TabSelector component to be one of the containers.
We can achieve that by following change:
useFocusTrapContainers
hook
import { useCallback, useState } from "react";
import { UseFocusTrapContainers } from "./type";
const useFocusTrapContainers: UseFocusTrapContainers = () => { const [containers, setContainers] = useState<HTMLElement[]>([]);
const addContainer = useCallback((container: HTMLElement) => { const removeContainer = () => setContainers((prevContainers) => prevContainers.filter((c) => c !== container)); setContainers((prevContainers) => prevContainers.includes(container) ? prevContainers : [...prevContainers, container]);
return removeContainer;
}, [])
return [containers, addContainer] }
export default useFocusTrapContainers;
Note: It is important we allow the container to be removed if the respective component is unmounted.
We will call the hooks 3 times in `IOURequestStartPage` to create 3 functions to register container for each tab. We can have 1 more hook call to have a function to register containers for the other components like `HeaderWithBackButton` and `TabSelector`
const [manualTabContainers, addManualTabContainer] = useFocusTrapContainers(); const [scanTabContainers, addScanTabContainer] = useFocusTrapContainers(); const [distanceTabContainers, addDistanceTabContainer] = useFocusTrapContainers();
Now for each tab component, like `IOURequestTabManual`, we need to drill down the prop `registerFocusTrapContainer=addManualTabContainer` down the tree, until we meet the first root `View`. For `IOURequestTabManual`, the prop needs to pass down from `IOURequestTabManual` > `MoneyRequestAmountForm` > [`ScrollView`](https://github.com/Expensify/App/blob/f392cd369131e80b09e1ada8afd239d461257b24/src/pages/iou/MoneyRequestAmountForm.tsx#L254). And in that `ScrollView`, we'll need to provide a ref callback to register that View as focus trap container
```typescript
<ScrollView ref={(scrollViewNode) => {
if(scrollViewNode) {
const unregister = props.registerFocusTrapContainer?.(scrollViewNode as unknown as HTMLElement);
return () => unregister?.();
}
}} contentContainerStyle={styles.flexGrow1}>
We should drill the props for IOURequestStepScan
and IOURequestStepDistance
as well.
Finally, we should calculate the list of container elements in IOURequestStartPage
:
const focusTrapContainers = [
...(selectedTab === CONST.TAB_REQUEST.MANUAL ? manualTabContainers : []),
...(selectedTab === CONST.TAB_REQUEST.SCAN ? scanTabContainers : []),
...(selectedTab === CONST.TAB_REQUEST.DISTANCE ? distanceTabContainers : []),
]
And then pass down to ScreenWrapper
> FocusTrapForScreen
to initialize the focus trap.
In order to avoid the prop drilling we can try to use React Context instead (note that we have to avoid the case: if a View
is registered as a focus trap container, all components in the child subtree cannot be registered to avoid redundancy), but that's more of a generic solution and I would love to have the team's input before working on this.
@rayane-djouah for a simpler solution, you can simply don't render anything if the tab isn't focus. Check out option 3 of my original proposal. We can even lift the logic up to IOURequestStepScan
<Tab.Screen>
{() => {
if(selectedTab !== 'scan') return null
return <IOURequestStepScan .../>
}}
</Tab.Screen>
Reviewing, will update today
Good timing @rayane-djouah - I was just typing a bump π - Thanks you for updating! π
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@dominictb - Can you please share a test branch of this solution? also, please include the solution in your original proposal.
Issue not reproducible during KI retests. (First week)
@rayane-djouah https://github.com/Expensify/App/compare/main...dominictb:epsf-app:fix/43719-shift-tab-test
This roughly the idea. The detailed implementation can be refined later.
@rayane-djouah for a simpler solution, you can simply don't render anything if the tab isn't focus. Check out option 3 of my original proposal. We can even lift the logic up to
IOURequestStepScan
<Tab.Screen> {() => { if(selectedTab !== 'scan') return null return <IOURequestStepScan .../> }} </Tab.Screen>
As I mentioned here: https://github.com/Expensify/App/issues/43719#issuecomment-2178614112 This solution will cause a performance issue (re-render delays when tab switching, especially for the map in the distance tab)
@tgolen, Summary:
Tab navigation was disabled on the initial page of the IOU flow as per this PR to address this issue. Subsequently, the implementation of a focus trap in this PR caused this regression, as explained in the RCA within this proposal. To fix this bug from root, it is recommended to set the preventScroll
option to true
as outlined in the second option of the solution section in the proposal. If re-enabling tab navigation on this page is desired, we can utilize the containerElements
prop from focus-trap-react
, as suggested here in the alternative solution section of the proposal. I defer the decision to you on whether we should re-enable tab navigation on this page, especially considering our recent focus on improving tab navigation in the app, as seen in https://github.com/Expensify/App/issues/36476.
@dominictb's proposal looks good to me.
:ribbon::eyes::ribbon: C+ reviewed
Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
I would like to re-assist the solutions proposed as both the solutions proposed such as hiding the in-active tabs and the use of containerElements
prop in the focus-trap-react were proposed initially by me.
@rayane-djouah could you please confirm the above statement?
cc: @OfstadC
I think we should try to have tab navigation anywhere we can, so if it's possible to re-enable tab navigation (it sounds like it), then I think we should do it. I'll wait for ^ question to be resolved before doing anything here though.
On Sun, Jul 21, 2024 at 2:34β―AM vishnu-karuppusamy @.***> wrote:
I would like to re-assist the solutions proposed as both the solutions proposed such as hiding the in-active tabs and the use of containerElements prop in the focus-trap-react were proposed initially by me.
@rayane-djouah https://github.com/rayane-djouah could you please confirm the above statement?
cc: @OfstadC https://github.com/OfstadC
β Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/43719#issuecomment-2241527627, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJMABZ5WK76ZMN3YCTKTDLZNNXAJAVCNFSM6AAAAABJI5OZFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGUZDONRSG4 . You are receiving this because you were mentioned.Message ID: @.***>
I would like to re-assist the solutions proposed as both the solutions proposed such as hiding the in-active tabs and the use of
containerElements
prop in the focus-trap-react were proposed initially by me.@rayane-djouah could you please confirm the above statement?
@vishnu-karuppusamy - Thank you for your initial suggestion regarding the use of the containerElements
prop. I appreciate your contribution to this discussion. I noticed, however, that your proposal might benefit from a more detailed explanation, particularly concerning the root cause analysis of this bug and a fully elaborated working solution.
I tried implementing the solution you suggested, as seen in the test branch, but unfortunately, it did not resolve the bug. In striving for the clearest and highest-quality proposal, with a fully working solution, I've decided to proceed with @dominictb's proposal that meets these criteria more fully.
Thank you again for your efforts and understanding.
@tgolen - Thank you for confirming the way to move forward! I agree that enabling tab navigation wherever possible will improve usability. Let's go ahead with @dominictb's solution.
π£ @rayane-djouah π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @dominictb π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
Any update here @dominictb @rayane-djouah ? π
PR under review
β οΈ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
This ^ is a false positive https://github.com/Expensify/App/issues/47381#issuecomment-2288264744
[!NOTE] The production deploy automation failed: This should be on [HOLD for Payment 2024-08-22] according to https://github.com/Expensify/App/issues/47356 prod deploy checklist, confirmed in https://github.com/Expensify/App/pull/45982#issuecomment-2294129588.
cc @OfstadC
Thanks @rayane-djouah - since the automation failed. I'll posted the checklist below π
Regression Test Proposal
Do we agree π or π
cc @OfstadC
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.83-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4626993 Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Nothing will happen
Actual Result:
RHP changes to Distance flow, and tabs (Manual, Scan, Distance) are missing
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/d86b3e9c-7d57-4722-b6ae-547be8e690f1
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @rayane-djouah