App icon indicating copy to clipboard operation
App copied to clipboard

Android/iOS- Highlighted search result go to search page rather that the highlighted detail page

Open IuliiaHerets opened this issue 1 year ago • 10 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.69-4 Reproducible in staging?: Y Reproducible in production?: N/A - new feature, doesn't exist in prod If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/

  2. Go to search router

  3. Write anything eg. "admin" and clear the search again ( to get the highlight on the first result)

  4. Click search icon on keyboard (enter)

Expected Result:

The highlighted item should open the detail page when clicking enter (Mweb behavior)

Actual Result:

The highlighted search result go to search page rather that the highlighted detail page

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/29478d14-d77b-4c4a-9829-98b9fb474c3b

View all open jobs on GitHub

IuliiaHerets avatar Dec 02 '24 20:12 IuliiaHerets

Triggered auto assignment to @luacmartins (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] avatar Dec 02 '24 20:12 melvin-bot[bot]

💬 A slack conversation has been started in #expensify-open-source

melvin-bot[bot] avatar Dec 02 '24 20:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 02 '24 20:12 melvin-bot[bot]

: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:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

github-actions[bot] avatar Dec 02 '24 20:12 github-actions[bot]

Demoting to NAB since it's just highlighting the wrong item. This is likely the offending PR @nyomanjyotisa @Pujan92

luacmartins avatar Dec 02 '24 20:12 luacmartins

@luacmartins, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 06 '24 09:12 melvin-bot[bot]

@luacmartins This doesn't seem to be from https://github.com/Expensify/App/pull/52298, as we have specific logic on search input submit. I think for smaller screens the first item should not be highlighted in the selection list when the sections data are altered to avoid this issue.

Pujan92 avatar Dec 06 '24 14:12 Pujan92

The expected behavior here is to open the "Admin" room or whatever search row is highlighted, not return to search right? Its not super clear in the OP so wanted to double check what we're aiming for here @luacmartins

MitchExpensify avatar Dec 07 '24 16:12 MitchExpensify

Correct, it should open the Admin room since that's what's highlighted

luacmartins avatar Dec 09 '24 22:12 luacmartins

I think for smaller screens the first item should not be highlighted in the selection list when the sections data are altered to avoid this issue

Should we implement this then? Or should we update the logic on search input submit to open Admin room in this case? Because in native platforms, it will always execute submitSearch on enter click, instead of opening the focused item

nyomanjyotisa avatar Dec 10 '24 00:12 nyomanjyotisa

I think the behavior should be consistent on all platforms, so we should highlight the first item again once the search is altered

luacmartins avatar Dec 10 '24 19:12 luacmartins

Job added to Upwork: https://www.upwork.com/jobs/~021866990715543185912

melvin-bot[bot] avatar Dec 11 '24 23:12 melvin-bot[bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

melvin-bot[bot] avatar Dec 11 '24 23:12 melvin-bot[bot]

@luacmartins Could you please confirm the nexts steps here ?

  1. Is this issue considered a regression from #52298 ?
  2. Should the issue be addressed by author / reviewer as a follow-up or we're open for proposals ?

ikevin127 avatar Dec 14 '24 00:12 ikevin127

@luacmartins @MitchExpensify @ikevin127 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!

melvin-bot[bot] avatar Dec 16 '24 09:12 melvin-bot[bot]

Reminder on this @luacmartins

MitchExpensify avatar Dec 16 '24 13:12 MitchExpensify

As pointed out here, I don't think this issue is a regression, so we're open to proposals

luacmartins avatar Dec 16 '24 21:12 luacmartins

@luacmartins, @MitchExpensify, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 17 '24 09:12 melvin-bot[bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Dec 18 '24 16:12 melvin-bot[bot]

Still looking for proposals

luacmartins avatar Dec 18 '24 16:12 luacmartins

Still looking for proposals

ikevin127 avatar Dec 18 '24 19:12 ikevin127

Edited by proposal-police: This proposal was edited at 2025-01-02 07:53:16 UTC.

Proposal

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

Android/iOS- Highlighted search result go to search page rather that the highlighted detail page

What is the root cause of that problem?

On smaller screens, the top-most result in Recent Chats is not highlighted when the search router is opened for the first time. However, it will highlight the first item (from Recent Chats or Recent Searches, if available) when the user types in the search bar and then clears it. On native platforms specifically, every time the user presses the search icon on the keyboard (Enter), it will always execute submitSearch https://github.com/Expensify/App/blob/207622d1d5c1e3c229cd4294c3b449d60fc76767/src/components/Search/SearchRouter/SearchRouter.tsx#L188-L203 And then will be called on this onSubmit https://github.com/Expensify/App/blob/207622d1d5c1e3c229cd4294c3b449d60fc76767/src/components/Search/SearchRouter/SearchRouter.tsx#L281-L287

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

We can create a new function that will handle the onSubmit. The idea is to navigate to the highlightedItem using onListItemPress if the textInputValue is not empty and it is not the initial state (i.e., the user has already typed and cleared the search query).

    // Track if the user has typed initially
    const [isInitialType, setIsInitialType] = useState(true);

We set it to false on onSearchQueryChange

            if (userQuery.length > 0) {
                setIsInitialType(false);
            }

We also need recentSearchesData to retrieve the recent searches (if available). Because if there is recent searches, the highlighted item is the last recentSearchesData

    const sortedRecentSearches = useMemo(() => {
        return Object.values(recentSearches ?? {}).sort((a, b) => b.timestamp.localeCompare(a.timestamp));
    }, [recentSearches]);

    const recentSearchesData = sortedRecentSearches?.slice(0, 5).map(({query, timestamp}) => {
        const searchQueryJSON = SearchQueryUtils.buildSearchQueryJSON(query);
        return {
            text: searchQueryJSON ? SearchQueryUtils.buildUserReadableQueryString(searchQueryJSON, personalDetails, reports, taxRates) : query,
            singleIcon: Expensicons.History,
            searchQuery: query,
            keyForList: timestamp,
            searchItemType: CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.SEARCH,
        };
    });
    const onSubmit = useCallback(() => {
        if (!textInputValue && !isInitialType) {
            const highlightedItem = recentSearchesData[0] || searchOptions.recentReports[0];
            if (highlightedItem) {
                onListItemPress(highlightedItem);
            }
        } else {
            submitSearch(textInputValue);
        }
    }, [textInputValue, recentSearchesData, searchOptions.recentReports, onListItemPress, submitSearch]);
                    <SearchRouterInput
                        ...
                        onSubmit={onSubmit}
                        ...

To achieve the same behavior on mWeb version when the textInputValue is empty and there is no highlighted item, we can add onKeyPress event to this TextInput

                        onKeyPress={(event) => {
                            if (Browser.isMobile() && event.nativeEvent.key === 'Enter') {
                                onSubmit();
                            }
                        }}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

POC

https://github.com/user-attachments/assets/523b1945-53e1-4641-916f-71a2f6ffa659

nyomanjyotisa avatar Dec 20 '24 08:12 nyomanjyotisa

Edited by proposal-police: This proposal was edited at 2024-12-21 14:07:20 UTC.

Proposal

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

Highlighted search result go to search page rather that the highlighted detail page

What is the root cause of that problem?

First, I will explain why the highlighted item on the mobile web can open the detail page when pressing Enter.

We use useKeyboardShortcut to listen for the Enter key press. When Enter is pressed, it executes the selectFocusedOption function to select the highlighted row in the list. and this useKeyboardShortcut only works on the mobile web (mWeb).

https://github.com/Expensify/App/blob/4cd759c6a12f37a7fe818537204f1e39ed882647/src/components/SelectionList/BaseSelectionList.tsx#L746-L752

https://github.com/Expensify/App/blob/4cd759c6a12f37a7fe818537204f1e39ed882647/src/components/SelectionList/BaseSelectionList.tsx#L399-L407

And the selected item, when Enter is pressed, will be handled for search or navigation.

https://github.com/Expensify/App/blob/eb896c68d2d36fccb0fb7a5ecef5360f62bd000b/src/components/Search/SearchRouter/SearchRouter.tsx#L301

https://github.com/Expensify/App/blob/eb896c68d2d36fccb0fb7a5ecef5360f62bd000b/src/components/Search/SearchRouter/SearchRouter.tsx#L239-L243

In this case, we encountered an issue (mweb) where, if no item in the list is highlighted, pressing Enter does not trigger any action.

https://github.com/user-attachments/assets/b74831e8-b08d-4fc8-8324-83d26cc3b447

For native apps (Android/iOS), we detect the Enter key press using onSubmitEditing.

https://github.com/Expensify/App/blob/9675b9cb27b8fd4170a6461adbcf62c9afdb5844/src/components/Search/SearchRouter/SearchRouterInput.tsx#L109

https://github.com/Expensify/App/blob/eb896c68d2d36fccb0fb7a5ecef5360f62bd000b/src/components/Search/SearchRouter/SearchRouter.tsx#L285-L287

When we press Enter, it calls submitSearch without handling the case for navigation to the highlighted item

https://github.com/Expensify/App/blob/eb896c68d2d36fccb0fb7a5ecef5360f62bd000b/src/components/Search/SearchRouter/SearchRouter.tsx#L188-L203

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

To resolve this issue in this ticket, on the native app, when Enter is pressed, if the text input value is empty, we need to check if a row is highlighted and then navigate to it.

  1. We need to get the focused item in the selection list.

Create getFocusedItem function

https://github.com/Expensify/App/blob/4cd759c6a12f37a7fe818537204f1e39ed882647/src/components/SelectionList/BaseSelectionList.tsx#L398

  const getFocusedItem = useCallback(() => {
      const focusedOption = focusedIndex !== -1 ? flattenedSections.allOptions.at(focusedIndex) : undefined;

      if (!focusedOption || (focusedOption.isDisabled && !focusedOption.isSelected)) {
          return;
      }

      return focusedOption;
  }, [focusedIndex, flattenedSections.allOptions]);

https://github.com/Expensify/App/blob/4cd759c6a12f37a7fe818537204f1e39ed882647/src/components/SelectionList/BaseSelectionList.tsx#L737-L743

Update to

    useImperativeHandle(ref, () => ({scrollAndHighlightItem, clearInputAfterSelect, updateAndScrollToFocusedIndex, updateExternalTextInputFocus, scrollToIndex, getFocusedItem}), [
        scrollAndHighlightItem,
        clearInputAfterSelect,
        updateAndScrollToFocusedIndex,
        updateExternalTextInputFocus,
        scrollToIndex,
        getFocusedItem,
    ]);
  1. We will update onSubmit

https://github.com/Expensify/App/blob/eb896c68d2d36fccb0fb7a5ecef5360f62bd000b/src/components/Search/SearchRouter/SearchRouter.tsx#L285-L287

  onSubmit={() => {
      const item = listRef.current?.getFocusedItem();
      if (!item) {
          submitSearch(textInputValue);
          return;
      }
      onListItemPress(item);
  }}

or

  onSubmit={() => {
      const item = listRef.current?.getFocusedItem();

      if (!textInputValue && !isSearchQueryItem(item)) {
      // or if (!textInputValue && item && !isSearchQueryItem(item)) {
          onRouterClose();

          if (item?.reportID) {
              Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(item?.reportID));
          } else if ('login' in item) {
              ReportUserActions.navigateToAndOpenReport(item.login ? [item.login] : [], false);
          }
          return;
      }
      submitSearch(textInputValue);
  }}

Regarding the mWeb issue, if we want to fix it in this ticket, we will update it as follows:

https://github.com/Expensify/App/blob/9675b9cb27b8fd4170a6461adbcf62c9afdb5844/src/components/Search/SearchRouter/SearchRouterInput.tsx#L93

    <TextInput
        ...
        onKeyPress={(event) => {
          const isWeb = getPlatform() === CONST.PLATFORM.WEB;
          if (!isWeb) {
              return;
          }
          if (event.nativeEvent.key !== 'Enter') {
              return;
          }
          onSubmit();
        }}
    />
POC

https://github.com/user-attachments/assets/53c1d61a-9f88-4f63-9cb6-2a81d60941ee

Test branch

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Test getFocusedItem :

  1. should return undefined when focusedIndex is -1
  2. should return undefined when focused item is undefined
  3. should return undefined when focused item is disabled and not selected
  4. should return the focused item when it is valid

Test onSubmit logic:

  1. calls submitSearch when no item is focused
  2. calls onListItemPress with the focused item

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

huult avatar Dec 21 '24 14:12 huult

@ikevin127 Proposals are ready for your review

MitchExpensify avatar Dec 23 '24 13:12 MitchExpensify

Will review today.

ikevin127 avatar Dec 23 '24 17:12 ikevin127

Both proposals are suggesting similar solutions when it comes to our main issue with slightly different implementations, both are fixing the issue, but the second proposal is considering another case that is related / in scope. Before I make a decision I want to check with @Expensify/design and / or @luacmartins on whether we should address the related case here.

The issue is that, on mWeb, if no item is selected and input is empty and we press Enter aka software keyboard search button, nothing happens. Compared to Native where the search input page is dismissed (w/ no specific search query):

iOS: Native iOS: mWeb Safari

IMO pressing Enter aka software keyboard search button should do something like it happens on Native, should dismiss the search input page with no specific search query, as to me it feels like something's broken on mWeb when it doesn't do anything.

I think we should fix the mWeb case such that we would have the same behaviour on both mWeb and Native. Please let me know what do you think!

ikevin127 avatar Dec 23 '24 19:12 ikevin127

Interesting, I think I tend to agree with you. Basically hitting enter on a blank Search query essentially is like resetting the search query back to nothing, aka searching for nothing which would just bring you back to the default home state of Search.

shawnborton avatar Dec 24 '24 15:12 shawnborton

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Dec 25 '24 16:12 melvin-bot[bot]

@luacmartins, @MitchExpensify, @ikevin127 Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Dec 27 '24 09:12 melvin-bot[bot]

We're still awaiting some more input on https://github.com/Expensify/App/issues/53401#issuecomment-2560208680 regarding final decision.

ikevin127 avatar Dec 27 '24 23:12 ikevin127