Open lanitochka17 opened 3 weeks ago
Triggered auto assignment to @isabelastisser (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #vip-vsp
Using up and down arrow keys doesn't change the button focus on confirm modal.
We don't support up and down arrow keys functionality in ConfirmContent.tsx
.
Handle up and down keyboard keys like this:
const confirmButtonRef = useRef<HTMLButtonElement>(null);
const cancelButtonRef = useRef<HTMLButtonElement>(null);
useEffect(() => {
const handleKeyDown = (event: KeyboardEvent) => {
if (event.key === 'ArrowUp') {
if (confirmButtonRef.current) {
confirmButtonRef.current.focus();
}
} else if (event.key === 'ArrowDown') {
if (cancelButtonRef.current) {
cancelButtonRef.current.focus();
}
}
};
document.addEventListener('keydown', handleKeyDown);
return () => {
document.removeEventListener('keydown', handleKeyDown);
};
}, []);
Then, pass the refs to the buttons.
Job added to Upwork: https://www.upwork.com/jobs/~0125e80c6474a3886e
Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External
)
Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key
There no logic to handle keyboard input in ConfirmContent.tsx
shouldStackButtons
props is true then stacked button should be focus and same for non stacked buttons.Additionally
Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key
The issue happens because we set setShouldShowComposeInputKeyboardAware
to true when the edit input gets blurred but we shouldn't call setShouldShowComposeInputKeyboardAware
when we have delete modal open.
https://github.com/Expensify/App/blob/dd30d137b485482bb8e4c61700b58d2e03683124/src/pages/home/report/ReportActionItemMessageEdit.tsx#L456
We should create a ref isDeleteModalActive
and set it to true when before we call input blur and showDeleteModal
and in onBlur
callback we should check if showDeleteModal.current
is true or not, if it is we will return without calling setShouldShowComposeInputKeyboardAware
.
We can use isPopoverVisible
state from ReportActionContextMenu
.
Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key
We don't have any event handlers for up and down arrow keys. That's why doesn't have focus change when we press up and down arrow keys.
We can make use of useKeyboardShortcut()
and useArrowKeyFocusManager()
hooks that have been used for the similar purpose (handling focus with cursors).
This involves three steps -
useArrowKeyFocusManager()
to track the focused index.
const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager(
{
maxIndex: 1,
initialFocusedIndex: 0,
}
)
ref={(ref) => {
if (!ref) {
return;
}
buttonRefs.current.push(ref);
}}
useKeyboardshortcut()
to listen to - arrow up and and arrow down shortcuts as below -
useKeyboardShortcut(
CONST.KEYBOARD_SHORTCUTS.ARROW_UP,
() => {
if (!(focusedIndex > 0)) {
return;
}
setFocusedIndex(focusedIndex - 1);
}
);
useKeyboardShortcut( CONST.KEYBOARD_SHORTCUTS.ARROW_DOWN, () => { if (focusedIndex === buttonRefs.current.length - 1) { return; } setFocusedIndex(focusedIndex + 1); } );
Sure we can refactor conditions. With help of `useKeyboardshortcut()` we can avoid managing event listeners since it will do heavy lifting and and it's well tested.
https://github.com/Expensify/App/assets/27822551/73444e4b-1079-466d-8e39-d71e302b5046
### What alternative solutions did you explore? (Optional)
let hasFocused = focusFirstDescendant(trapElementRef.current);
shouldAutoFocus
is true
at the first time because !modal?.isVisible
is true
. That leads to the main composer being focused for a while. I do not know why the main composer is focused if shouldAutoFocus
is true
but if you try removing this line, the bug is gone.focus
event, so when the above main composer is focused, that event handle is triggered, and the logic to focus on the last descendant is call:
hasFocused = focusLastDescendant(trapElementRef.current);
willModalBecomeVisible
to check if the modal is visible or not, so it will be updated immediately, rather than using the current modal?.isVisible
from onyx.
let willModalBecomeVisible = false;
function setWillModalBecomeVisible (isVisible:boolean){ willModalBecomeVisible = isVisible }
2. Then, update [this](https://github.com/Expensify/App/blob/89b9cef1f4d6ee61473181431fc76d78d7d1bc11/src/pages/home/report/ReportActionItemMessageEdit.tsx#L331) to:
setWillModalBecomeVisible(true)
return;
3. Then, update [this](https://github.com/Expensify/App/blob/89b9cef1f4d6ee61473181431fc76d78d7d1bc11/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx#L278-L279) condition to:
const shouldAutoFocus =
!willModalBecomeVisible && !modal?.isVisible && isFocused && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentReportAction))) && shouldShowComposeInput;
4. Finally, we need to reset ```setWillModalBecomeVisible(false)``` once modal hides
### What alternative solutions did you explore? (Optional)
- We can update [this](https://github.com/Expensify/App/blob/db9e01cc302df7f2b3dd53a99c1082404676aedb/src/pages/home/report/ReportActionItemMessageEdit.tsx#L456) to:
isModalHidden().then(()=> setShouldShowComposeInputKeyboardAware(true))
- In the above, ```isModalHidden``` is the promise that will be resolved if the modal is closed. Other reasons lead to the edit composer is blurred, ```isModalHidden``` is always resolved
Hey @shubham1206agra, can you please review the proposals above? Thanks!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Bump @shubham1206agra to review the proposals above. I will DM you too for visibility. Thanks!
I remember arrow keys were supported earlier in this modal. Can someone check if that was the case. Similar issue https://github.com/Expensify/App/issues/33201
@shubham1206agra can you post in open source to confirm? Thanks!
@isabelastisser, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@isabelastisser @shubham1206agra 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!
@isabelastisser, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Bump @shubham1206agra for an update! I will DM you for visibility.
@Krishna2323 and @BhuvaneshPatil Can you both provide me with test branches? I want to test both solutions first.
@nkdengineer Your proposal lacks why the focus sticks on the Cancel button.
@shubham1206agra I mentioned why the focus sticks on the Cancel button in the below section:
Yeah, but it doesn't explain why the focus sticks.
@shubham1206agra focusLastDescendant
is function to focus on the last descendant component of the modal, in this case the last descendant is the "Cancel" button. Calling focusLastDescendant
leads to the focus sticks on the Cancel button.
Feel free to provide feedback if my response is unclear. Thanks
You still didn't understood what I am asking. I understand how cancel button gets focused. But why am I unable to change the focus by up/down arrow keys?
change the focus by up/down arrow keys?
Sorry because misunderstood your thought. I think we do not have this feature. We just can change the focus by using the Tab key.
I do not know why the main composer is focused if shouldAutoFocus is true but if you try removing this line, the bug is gone.
Then why did you say this? If the feature was not implemented.
@shubham1206agra, test branch here
@shubham1206agra Branch
@Krishna2323 Your solution does not solve the issue.
@shubham1206agra, can you pls explain whats not working?
Video from my branch:
https://github.com/Expensify/App/assets/85894871/b5b58834-1b1f-4d9e-ab18-44ae137abf30
@Krishna2323 Try to cancel delete using keyboard.
@shubham1206agra, that also works as expected.
https://github.com/Expensify/App/assets/85894871/98d1c26c-8ecb-40a9-9341-ab4b4e85b3dd
@isabelastisser Can you confirm if we should be able to toggle between delete and cancel in modal using arrow keys?
Also, there are other places where this modal is being used. Example - when deleting a workspace.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Can you confirm if we should be able to toggle between delete and cancel in modal using arrow keys?
@shubham1206agra, I don't think the up/down arrow keys are supposed to work on buttons.
The focus should not remain on the "Cancel" button when attempting to clear the message and save changes using the Enter key. The user should be able to delete the comment using the Enter key
The expected result is to not focus on the cancel button initially, and changes should be saved on Enter key press. My proposal works accordingly.
Chat - Focus Sticks on Cancel Button After Clearing Message & Saving with Enter Key
in node_modules\react-native-web\dist\exports\Modal\ModalFocusTrap.js
document.addEventListener('focus', trapFocus, true);
Modal binding the focus event. trapFocus
is a recursive function, so the cancel button will be focused.
in node_modules/@expensify/react-native-live-markdown/lib/module/MarkdownTextInput.web.js
useEffect(() => {
if (!divRef.current) {
return;
}
// focus the input on mount if autoFocus is set
if (autoFocus) {
divRef.current.focus();
}
}, [autoFocus]);
when autoFocus
is true
, triggered the focus event.
Composer will trigger Modal focus event,the "Cancel" button is focused.
!modal?willAlertModalBecomeVisible
is true
,shouldAutoFocus
should be false
const shouldAutoFocus =
!modal?.willAlertModalBecomeVisible && !modal?.isVisible && isFocused && (shouldFocusInputOnScreenFocus || (isEmptyChat && !ReportActionsUtils.isTransactionThread(parentReportAction))) && shouldShowComposeInput;
N/A
@shubham1206agra, please review the proposal above. Thanks!
@isabelastisser, can you pls provide confirmation on this.
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.61-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: N/A Email or phone of affected tester (no customers): abebemiherat@gmail.com Issue reported by: Applause - Internal Team
Action Performed:
1, Write a comment and hover over it, then click edit 2, Clear the message and press the Enter key 3, Observe that the focus remains on the "Cancel" button, preventing the deletion of the comment using the Enter key
Expected Result:
The focus should not remain on the "Cancel" button when attempting to clear the message and save changes using the Enter key. The user should be able to delete the comment using the Enter key
Actual Result:
The focus remains on the "Cancel" button when attempting to clear the message and save changes using the Enter key. The user is unable to delete the comment using the Enter key
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/01839aaf-6a53-4682-a3c6-03a91b06d71a
View all open jobs on GitHub
Upwork Automation - Do Not Edit