Closed m-natarajan closed 3 months ago
Triggered auto assignment to @garrettmknight (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
Comment not deleted when delete button in red and hit enter key
Needs focus trap
After this PR merged https://github.com/Expensify/App/pull/39520
We need to use focus trap to set the focus inside the delete popup and to set the focus to the delete button
To set focus on the button when the component loads, we can use the useRef
hook to create a reference to the button and then use the useEffect
hook to focus it once the component has mounted. Here's how you can modify the ConfirmContent
component to achieve this:
useRef
and useEffect
hooks from React.useRef
.useEffect
to set the focus when the component loads.Here's the updated code:
const confirmButtonRef = useRef(null); // Create a ref for the confirm button
useEffect(() => { if (confirmButtonRef.current) { confirmButtonRef.current.focus(); // Focus the confirm button when the component loads } }, []);
<Button ref={confirmButtonRef} //..
In this updated code, the confirmButtonRef
is used to create a reference to the confirm button, and useEffect
is used to set the focus on the button when the component loads.
Pressing the Enter key when opening the delete confirmation modal doesn't delete the comment/message.
When the delete confirmation modal is shown, the focus trap from the rn-web Modal focuses on the Cancel button, so pressing Enter will press the Cancel button. We set pressOnEnter
shortcut to the Delete button, but it will be disabled if the current focus is focused on an element with an accessibility role, except text.
https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/components/Button/index.tsx#L131
Now, the question is why the cancel button is focused first when we open the confirmation modal. In the confirmation modal, there are 3 focusable elements, the overlay, delete button, and cancel button. The logic of the focus trap is here. It will focus on the first focusable element first and keep the focus when tabbing within the focus trapped element.
Here is how it behaves in this GH test case. First, it will focus on the first focusable descendant of the modal, in our case, it's the modal overlay. The modal overlay is saved as the last focused element. At this point, the edit composer is blurred. If the edit composer is blurred, it will set SHOULD_SHOW_COMPOSE_INPUT to true. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/pages/home/report/ReportActionItemMessageEdit.tsx#L427
SHOULD_SHOW_COMPOSE_INPUT hides the main composer when an edit composer is focused, on small screens only. Besides the SHOULD_SHOW_COMPOSE_INPUT controls the main composer visibility, it also controls the autoFocus behavior. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L288-L289
Because SHOULD_SHOW_COMPOSE_INPUT is now true, autoFocus now becomes true too. Before, it was false because an edit composer was focused which set SHOULD_SHOW_COMPOSE_INPUT to false.
Now, here's the problem. autoFocus is supposed to work only once when the text input is mounted. However, the live markdown input will focus on the input every time the autoFocus is changed to true. https://github.com/Expensify/react-native-live-markdown/blob/main/src/MarkdownTextInput.web.tsx#L584-L588
The input focus then triggers the focus trap logic again which first focuses on the first focusable descendant, that is the overlay. Then it checks whether the last (prev) focused element is the same as the current focused element. If it's the same, it will start focusing on the last focusable descendant element.
In our case, the last focused element is the overlay and the first focusable descendant is also the overlay, so the focus trap will focus on the last focusable descendant element, that is the cancel button.
We can:
but I would like to address the root cause of the issue, that is the input gets focused every time autoFocus is changed which doesn't happen with the react native TextInput.
To fix the issue, we should remove autoFocus from the effect deps here https://github.com/Expensify/react-native-live-markdown/blob/377ab57fa02c598b624eb91e4faf4fd2693596a2/src/MarkdownTextInput.web.tsx#L584-L588
This way, it will work as pointed out by the comment, that is to auto-focus only when the component is mounted, not when autoFocus is changed.
This happens after this react native live markdown PR which adds the react hooks eslint rule and adds the autoFocus as the deps to fix the lint, but we shouldn't do that for autoFocus case.
This will also fix an issue where the main composer is focused every time the edit composer is blurred.
https://github.com/Expensify/App/assets/50919443/2773f351-bb92-41fd-a3c5-ee5830febb2d
After this change, the overlay will be the one that is focused when opening the confirmation modal, which is what happens with other confirmation modals. Pressing Enter on the overlay will trigger the pressOnEnter
shortcut because the overlay doesn't have any role.
Job added to Upwork: https://www.upwork.com/jobs/~01b70ac798adfe8d66
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External
)
Thanks for the proposal everyone.
Proposal from @bernhardoj looks good to me.
πππ C+ reviewed
Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @bernhardoj π 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 π
The live markdown PR is here
The live markdown PR is merged and released in 0.7.78, but we can't update it yet because 0.7.76 caused a regression, so we need to wait for a new version that fixes the issue.
Still holding on https://github.com/Expensify/App/issues/42766
β οΈ 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.
Still holding on https://github.com/Expensify/App/issues/42766
@bernhardoj The react-native-live-markdown version in bumped. Can you please retest and verify that the issue is resolved.
Tested on staging and i'ts fixed.
https://github.com/Expensify/App/assets/50919443/efc6bd02-6348-4003-bfc6-a1a578f2abee
Sweet! @sobitneupane does this mean we can finish with pay out for this issue and close it?
cc: @garrettmknight
@garrettmknight @blimpich @sobitneupane @bernhardoj this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
I can confirm that the issue is resolved.
https://github.com/Expensify/App/assets/25876548/6c859368-6a4f-4735-a69b-92a33b4cd992
does this mean we can finish with pay out for this issue and close it?
Yup.
Nice one!
Payment Summary
@sobitneupane is any of the checklist necessary on this one?
Regression Test Proposal
Do we agree π or π
Dropping to weekly for you to request @sobitneupane
Just awating the reqeust
@garrettmknight @blimpich @sobitneupane @bernhardoj this issue is now 4 weeks old, please consider:
Thanks!
Still awaiting payment request
@garrettmknight, @blimpich, @sobitneupane, @bernhardoj Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Still awaiting the payment request
Still awaiting the payment request
@sobitneupane I'm gonna close this one, but submit the payment request when you get a chance.
$250 approved for @sobitneupane
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.76-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @iwiznia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1716823498553919
Action Performed:
Expected Result:
Message/comment deleted
Actual Result:
Message/comment is not deleted because the focus is on the
cancel
button (even though delete is in red)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/38435837/ab09a4e9-a12d-4d66-943b-4e1affbb40c7
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @sobitneupane