Search - Unable to perform search by hitting Enter on search bar when expense is selected

lanitochka17 commented 1 week ago

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.53-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A 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): applausetester+kh1610015@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to Search
Click Filters
Apply any filter that will return results
Select an expense via checkbox
Click on the search input field
Type anything and hit Enter

Expected Result:

App will perform a new search

Actual Result:

The dropdown button is expanded

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Standalone
[ ] Android: HybridApp
[ ] Android: mWeb Chrome
[ ] iOS: Standalone
[ ] iOS: HybridApp
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/cab1eefa-0d9f-46a0-bb50-052c2c62e1f4

View all open jobs on GitHub

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @anmurali (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.

lanitochka17 commented 1 week ago

@anmurali 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

Krishna2323 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-10-24 19:31:51 UTC.

Proposal 

Please re-state the problem that we are trying to solve in this issue.

Search - Unable to perform search by hitting Enter on search bar when expense is selected

What is the root cause of that problem?

pressOnEnter prop is passed, so the enter press is caught by the button. https://github.com/Expensify/App/blob/476ad6bcca6de4faac6a41c37874fd9be62ddfe5/src/components/Search/SearchPageHeader.tsx#L367

What changes do you think we should make in order to solve the problem? 

We have several options to solve this.

We can remove the pressOnEnter prop.

We can use useActiveElementRole and pass false to pressOnEnter when activeElementRole is presentation.

const activeElementRole = useActiveElementRole();
const disableEnterShortcut = activeElementRole && [CONST.ROLE.PRESENTATION].includes(activeElementRole as 'presentation');

Through ref forwarding, we can get the input ref of the input in SearchRouterInput and then we can use textInputRef.current?.isFocused() to disabled the pressOnEnter prop. We might also need to use useEffect to keep the updated value of textInputRef.current?.isFocused().
If needed, we can also remove pressOnEnter prop from other pages like workspace categories, tags, etc.

What alternative solutions did you explore? (Optional)

In the useKeyboardShortcut config we can add captureOnInputs: false. https://github.com/Expensify/App/blob/32d4ddf6ee7569dc50d1358104fc5b02d7f6f5f5/src/components/Button/index.tsx#L175-L184
Or we can introduce a new prop captureOnInputs and then pass it from SearchPageHeader > ButtonWithDropdownMenu > Button.
Else we can make the default value false. https://github.com/Expensify/App/blob/32d4ddf6ee7569dc50d1358104fc5b02d7f6f5f5/src/hooks/useKeyboardShortcut.ts#L31

Result

FitseTLT commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search - Unable to perform search by hitting Enter on search bar when expense is selected

What is the root cause of that problem?

We have set pressOnEnter on the drop down button and in side the button we already have a code that disable the shortcut based on the active element role but it only disables it if the active element is presentation, which is correct https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/Button/index.tsx#L163 but we set our text inputs role as presentation https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/Search/SearchRouter/SearchRouterInput.tsx#L99

What changes do you think we should make in order to solve the problem?

I propose two options

We can give the text input a proper role like textbox We will need to add the role here
```
    TEXTBOX: 'textbox',
```

2. Along with active role element management we can keep track of whether a text input is focused. Here
https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/ActiveElementRoleProvider/index.tsx#L9-L11

const [isTextInputFocused, setIsTextInputFocused] = useState<boolean | null>(null); const handleFocusIn = () => { setRole(document?.activeElement?.role ?? null); setIsTextInputFocused(['INPUT', 'TEXTAREA'].includes(document.activeElement?.nodeName ?? '')); };

....

const value = React.useMemo(
    () => ({
        role: activeRoleRef,
        isTextInputFocused,
    }),
    [activeRoleRef, isTextInputFocused],

then

const useIsTextInputFocused = () => { const {isTextInputFocused} = useContext(ActiveElementRoleContext); return isTextInputFocused; };

https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/components/Button/index.tsx#L163

const isTextInputFocused = useIsTextInputFocused(); const shouldDisableEnterShortcut = useMemo( () => isTextInputFocused || (accessibilityRoles.includes(activeElementRole ?? '') && activeElementRole !== CONST.ROLE.PRESENTATION), [activeElementRole, isTextInputFocused], );


### What alternative solutions did you explore? (Optional)

Krishna2323 commented 1 week ago

PROPOSAL UPDATED

Added alternative solution

melvin-bot[bot] commented 6 days ago

@anmurali Eep! 4 days overdue now. Issues have feelings too...

anmurali commented 6 days ago

I don't think it's very clear how this should behave. You are changing the filter criteria, but there is a selected expense. Should the filter results update? What if the selected expense no longer matches the criteria? cc @luacmartins @trjExpensify @JmillsExpensify

luacmartins commented 19 hours ago

I imagine we'd just clear the selection and perform the new Search. cc @JmillsExpensify for your input as well.

Expensify / App