Open lanitochka17 opened 1 month ago
Triggered auto assignment to @srikarparsi (DeployBlockerCash
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
We think that this bug might be related to #vip-vsp
Issue is not reproducible on Production:
https://github.com/user-attachments/assets/80605f58-1968-445c-aa67-9a1b290d0f6b
Possibly related to https://github.com/Expensify/App/pull/45743 (?) Anyway, doesn't look like something worth blocking for – going to demote.
Job added to Upwork: https://www.upwork.com/jobs/~011ced2d499965abe3
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
Hi @tsa321 @hoangzinh, if you have a second, do you think you could check if this issue is related to https://github.com/Expensify/App/pull/45743
@srikarparsi, I have tried reverting my PR, but this issue is still reproducible.
Edited by proposal-police: This proposal was edited at 2024-08-13 12:48:11 UTC.
Blue border appears on + button when user open report by url
We use focusTrap to maintain tab navigation. When a user visits a page via URL, focusTrap initially focuses on the tabbable element, causing the blue border to appear. This issue is not limited to the report page; it occurs on several other pages as well, such as the About page, Profile page, Workspace List page, and briefly in the display name field, etc
We can disable the blue border using CSS styling when the page loads. We can then enable the blue border only when the user presses the Tab key. If any other key is pressed, we should disable the blue border again. The code could be:
let sheet;
const mangaeFocusedElementBlueBorder = () => {
if (!document?.body) {
return;
}
sheet = document.createElement('style');
sheet.innerHTML = '[tabindex="0"]:focus{box-shadow: none; outline: none;}'
document.body.appendChild(sheet);
window.addEventListener('keydown', (e) => {
if (e.key === 'Tab' || e.key === 'Shift') {
if (e.key === 'Tab' && !!sheet.parentNode) {
sheet.parentNode.removeChild(sheet);
}
} else if (!sheet.parentNode) {
document.body.appendChild(sheet);
}
}, true);
}
We could place it in the Accessbility library
For the css selector, we could use *:focus
instead of [tabindex="0"]:focus
In the FocusTrapForScreen
option:
we could add an onActivate
property with the function: () => document?.activeElement?.blur()
. Then, in initialFocus
:
we should always return false
there.
The focus trap already listens for the focus event and will check whether the focused element is inside the element to which it is attached (in this case, inside the screen) and will automatically move the focus inside the screen if it is outside.
To also fix this in strict mode, we could add activeElementRef
and include onActivate
and onDeactivate
options in focusTrapOptions
. In onActivate
, we store the current active element in the ref, then blur it; in onDeactivate
, we focus that element or if don't want to return the we don't need the onDeactivate callback. We must also set initialFocus
and setReturnFocus
to always return false
. I have created a test branch to highlight the code changes.
Thanks for your proposal @tsa321
Can you please elaborate the root cause section of your proposal?
When a user visits a page via URL, focusTrap initially focuses on the tabbable element, causing the blue border to appear.
Can you please link the code which implements above feature? Let's try to investigate more on the root cause.
Why is the issue limited to some pages only?
@sobitneupane,
Here:
In the initialFocus
of focusTrap
, if the return value is false
, the focusTrap
will not automatically focus on a tabbable element inside the screen. However, if the return value is undefined
, the focusTrap
will focus on an element inside it.
(Assuming the return value of initialFocus
is undefined
.) When a user opens a page by clicking a button, the blue border won't appear. However, when the user refreshes the page or opens it by pressing a keyboard key (for example, pressing Enter on a button), the blue border will appear. This is the default behavior of web browsers for tab navigation.
@sobitneupane, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Hi @sobitneupane, any updates here?
There is a bug in #46075 that seems to have the same cause as this one. Let's fix them together in this one.
Thanks for the update @tsa321
This PR adds the focus trap in the app.
Will your proposal resolve https://github.com/Expensify/App/issues/46075 issue as well? What is RCA for https://github.com/Expensify/App/issues/46075?
Are there any other ways to show the outline only if interacted through keyboard? If I am not wrong, focus-visible
outlines only if interaction is made through keyboard.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@sobitneupane
Will your proposal resolve the issue at https://github.com/Expensify/App/issues/46075 as well? What is the root cause analysis (RCA) for https://github.com/Expensify/App/issues/46075?
Yes, my proposal will address that issue as well. The root cause analysis (RCA) for #46075 indicates that the blue border only appears when something is typed in the search box, but it won't appear if you immediately click on the concierge chat without typing. This issue could be related to browser behavior involving typing.
Are there any other ways to show the outline only if interacted with through the keyboard? If I’m not mistaken,
focus-visible
outlines only appear with keyboard interaction.
Currently, browser behavior for focus is similar to focus-visible
. Before focus-visible
, the browser use focus
behavior, the browser would show the blue border even on mouse clicks. However, with focus-visible
, the blue border only appears if the event prior to the focus is from keyboard interaction or other criteria (which is not from mouse click). If the event is from a mouse click or other criteria, the blue border will not be displayed.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Hey @sobitneupane, @jnowakow who was the main creator of PR for Focus Trap is currently OOO. From my point of view, I can only agree that we should avoid workarounds, but I currently have no alternative.
@sobitneupane, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Thanks @adamgrzybowski
@sobitneupane, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!
Waiting for proposal.
updated on alternative solution.
@sobitneupane In the FocusTrapForScreen
option:
we could add an onActivate
property with the function: () => document?.activeElement?.blur()
. Then, in initialFocus
:
we should always return false
there.
The focus trap already listens for the focus event and will check whether the focused element is outside the element to which it is attached (in this case, inside the screen) and will automatically move the focus inside the screen if it is outside.
Thanks for the update @tsa321
The change you suggested might recreate some issue, https://github.com/Expensify/App/pull/39520 PR is trying to solve. For example: Focus remains on the report when navigated to RHP with keyboard.
https://github.com/user-attachments/assets/612a6320-02f6-424a-bb8e-691a4c45823d
@sobitneupane by adding onActivate
property to in focusTrap option: () => document?.activeElement?.blur()
the active element will be document body.
the blue border won't show up in the report
@tsa321, I tried but It didn't work for me. I might have missed something. Is it working on your end?
@sobitneupane it is working as expected for me:
https://github.com/user-attachments/assets/05680e08-1c6e-461c-80c9-3bb565082ac0
The code change is :
in FocusTrapForScreen/index.web.tsx
:
focusTrapOptions={{
+ onActivate: () => {
+ document?.activeElement?.blur();
+ },
trapStack: sharedTrapStack,
allowOutsideClick: true,
fallbackFocus: document.body,
delayInitialFocus: CONST.ANIMATED_TRANSITION,
initialFocus: (focusTrapContainers) => {
- if (!canFocusInputOnScreenFocus()) {
- return false;
- }
- const isFocusedElementInsideContainer = focusTrapContainers?.some((container) => container.contains(document.activeElement));
- if (isFocusedElementInsideContainer) {
- return false;
- }
- return undefined;
+ return false;
},
- setReturnFocus: (element) => {
- if (document.activeElement && document.activeElement !== document.body) {
- return false;
- }
- return element;
- },
...(focusTrapSettings?.focusTrapOptions ?? {}),
}}
The removal of setReturnFocus
is to make the focus return to last focused element in previous screen, it is optional.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@tsa321 I can still reproduce the issue.
https://github.com/user-attachments/assets/e4b642e5-0e54-414d-bb32-28b991a35405
@sobitneupane I forgot to mention: to test in the development environment, you must add mode: 'legacy'
below this line:
@tsa321 If I am not wrong, enabling the legacy mode will disable the concurrent mode. It makes sense that the issue is not reproducible in the legacy mode. But shouldn't it work in concurrent mode as well?
@sobitneupane I have a solution to fix this issue in strict mode and have created a test branch to highlight the code changes.
@sobitneupane, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!
Thanks for the update @tsa321 Overall the alternative solution 2 looks good.
Can you please add the reasoning for the suggested changes especially in onDeactivate
and setReturnFocus
?
In onActivate, we store the current active element in the ref, then blur it; in onDeactivate, we focus that element. We must also set initialFocus and setReturnFocus to always return false.
@sobitneupane Here is the log when I open the profile page from the tab navigation on the report screen:
In strict mode, the component is mounted, unmounted, and then mounted again (mounted twice). The onActivate
and onDeactivate
functions are triggered immediately and in the correct order. However, the setReturnFocus
of the unmounted screen is triggered once after the second onActivate
, which causes the blue border to keep appearing on the report screen because it returns the focus to the previous active element. That is why I use onActivate and onDeactivate instead of setReturnFocus.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Thanks for the update @tsa321
Why is onActivate
triggered twice? Is it an existing bug in focus-trap? Are there any issues related to it in the focus-trap repo? I am not sure if it is causing our issue.
@sobitneupane No, it is characteristic of strict mode in development environment: All component are mounted twice.
Issue not reproducible during KI retests. (First week)
@sobitneupane, @srikarparsi Huh... This is 4 days overdue. Who can take care of this?
Thanks for the update @tsa321
Alternative solution 2 in your proposal will probably solve the issue. If @jnowakow (author of the PR which adds the focus trap) is back, I'd like to hear his thoughts on the proposal.
🎀 👀 🎀 C+ reviewed
cc: @adamgrzybowski
Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@sobitneupane, @srikarparsi Eep! 4 days overdue now. Issues have feelings too...
@sobitneupane, @srikarparsi 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
Asked in the swm channel since I'm not too familiar with focus trap.
@sobitneupane, @srikarparsi 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!
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: 9.0.11-2 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Chat opens without blue frame around + button
Actual Result:
Chat opens with blue frame around + button
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/369832a2-8e5f-4180-b1c0-e23e7f9d8299
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @sobitneupane