Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix - Message action menu - Last option is not selected when pressing up key #38007

Merged
merged 3 commits into from
Mar 14, 2024
Merged

Fix - Message action menu - Last option is not selected when pressing up key #38007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5dbbf91
fix disabled indexes check loop
FitseTLT Mar 8, 2024
fb68564
return actualindex
FitseTLT Mar 8, 2024
535cad7
use allowNegativeIndexes prop
FitseTLT Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/EmojiPicker/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ function EmojiPickerMenu({forwardedRef, onEmojiSelected, activeEmoji}) {
disableHorizontalKeys: isFocused,
// We pass true without checking visibility of the component because if the popover is not visible this picker won't be mounted
isActive: true,
allowNegativeIndexes: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being used anywhere or did you just add it for possible use in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using it here for EmojiPickerMenu

Copy link
Contributor

@Ollyws Ollyws Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see yeah we're passing false for useArrowKeyFocusManager outside of that.

});

const filterEmojis = _.throttle((searchTerm) => {
Expand Down
19 changes: 15 additions & 4 deletions src/hooks/useArrowKeyFocusManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type Config = {
itemsPerRow?: number;
disableCyclicTraversal?: boolean;
disableHorizontalKeys?: boolean;
allowNegativeIndexes?: boolean;
};

type UseArrowKeyFocusManager = [number, (index: number) => void];
Expand Down Expand Up @@ -44,6 +45,7 @@ export default function useArrowKeyFocusManager({
itemsPerRow,
disableCyclicTraversal = false,
disableHorizontalKeys = false,
allowNegativeIndexes = false,
}: Config): UseArrowKeyFocusManager {
const allowHorizontalArrowKeys = !!itemsPerRow;
const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex);
Expand Down Expand Up @@ -84,7 +86,13 @@ export default function useArrowKeyFocusManager({
while (disabledIndexes.includes(newFocusedIndex)) {
newFocusedIndex -= allowHorizontalArrowKeys ? itemsPerRow : 1;
if (newFocusedIndex < 0) {
break;
if (disableCyclicTraversal) {
if (!allowNegativeIndexes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing something? Everything seems to work fine for me without this if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which if ??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!allowNegativeIndexes) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is the exceptional case for emoji picker menu so if you remove it, it will break the emoji picker search input auto focusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm strange, it seems to work just fine for me with this block removed. Am I missing something here?

Screen.Recording.2024-03-13.at.08.29.15.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't talking about the removal of that logic, I thought you were asking why we need that check.
What makes EmojiPickerMenu auto-focusing to fail is the returning actualIndex part of the code but in the while loop we are iterating until we get non-disabled index but now when we reach negative index that principally means there is no non-disable index above the current actualIndex so we should return the current actualIndex index. Now comes an exceptional case as the EmojiPicker one on which we are using negative indexes to auto focus search input therefore we make an exception and when negative indexes are allowed instead of returning the actualIndex we just break.

Copy link
Contributor

@Ollyws Ollyws Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes EmojiPickerMenu auto-focusing to fail

Could you quickly demonstrate this just so I can understand what exactly we are fixing here? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the break mechanism everything will work fine but principaly that's wrong and it will create the same issue for the future. To demonstrate that remove this block then pass disableCyclicTraversal as true for context menu here

App/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx

Lines 157 to 158 in 6e42e55

disabledIndexes,
maxIndex: filteredContextMenuActions.length - 1, 

        disableCyclicTraversal: true,

Now, while you are on the first active index if you press up the focus will be lost and unless u press down it will not appear that is what it will cause if we use the break logic but now as it is the emoji picker the only exceptional case we allow negative indexes to auto focus search input we are treating it as an exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok so while it essentially has no effect currently, I suppose it's better for future-proofing. Thanks for the explanation!

return actualIndex;
}
break;
}
newFocusedIndex = maxIndex;
}
if (newFocusedIndex === currentFocusedIndex) {
// all indexes are disabled
Expand All @@ -93,7 +101,7 @@ export default function useArrowKeyFocusManager({
}
return newFocusedIndex;
});
}, [allowHorizontalArrowKeys, disableCyclicTraversal, disabledIndexes, itemsPerRow, maxIndex]);
}, [allowHorizontalArrowKeys, disableCyclicTraversal, disabledIndexes, itemsPerRow, maxIndex, allowNegativeIndexes]);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ARROW_UP, arrowUpCallback, arrowConfig);

Expand Down Expand Up @@ -127,8 +135,11 @@ export default function useArrowKeyFocusManager({
newFocusedIndex += allowHorizontalArrowKeys ? itemsPerRow : 1;
}

if (newFocusedIndex < 0) {
break;
if (newFocusedIndex > maxIndex) {
if (disableCyclicTraversal) {
return actualIndex;
}
newFocusedIndex = 0;
}
if (newFocusedIndex === currentFocusedIndex) {
// all indexes are disabled
Expand Down
Loading