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.

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

Mobile: Fixes #11130: Fix regression: Search screen not hidden when cached for search result navigation #11131

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Sep 26, 2024

Summary

This pull request fixes a regression related to refactoring done in a recent pull request — the search results screen was not hidden when its visible prop was set to false. This happened after clicking on a search result and resulted in a split layout.

This pull request copies the approach used by screens/Notes.tsx — styling the search screen with flex: 0.001 when marked with visible: false and setting inert={true} to prevent accessibility navigation. This is similar to the logic used prior to the refactoring.

Fixes #11130.

Screen recordings

Android + TalkBack

search-bugfix.webm

Shows:

  1. Opening search with an existing query.
  2. Activating a search result.
    • The search screen is correctly hidden.
  3. Clicking back navigates back to the search result screen.
  4. Activating another search result.
    • The search screen is correctly hidden.
  5. TalkBack focus is moved from the title to the edit button.

Web + Chromium

Screencast.from.2024-09-26.15-47-55.webm

Shows:

  1. Opening search with an existing query.
  2. Activating a search result.
  3. Navigating to a different note from the result.
  4. Pressing the back arrow.
    • Navigates back to the first note.
  5. Pressing the back arrow.
    • Navigates back to the search screen.
  6. Opening a different search result.
  7. Pressing the back arrow.
    • Navigates back to the search screen.
  8. Pressing the back arrow.
    • Navigates back to the note list.

…en when

cached for search result navigation

For now, this commit uses the same approach as the Notes screen -- the
search screen is given `flex: 0.001`. This
was also the component's behavior prior to a recent
[prior to a recent refactoring](https://github.com/laurent22/joplin/blob/598677b2072ab0648c7702f1d27c1e5ca3d628d3/packages/app-mobile/components/screens/search.tsx#L163).
Comment on lines 53 to 54
// TODO: Improve. This hides the component without unmounting it.
flex: !visible ? 0.001 : 1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the similar logic in Notes.tsx:

rootStyle.flex = 0.001; // This is a bit of a hack but it seems to work fine - it makes the component invisible but without unmounting it

...and the equivalent logic in search.tsx prior to the refactoring:
rootStyle.flex = 0.001; // This is a bit of a hack but it seems to work fine - it makes the component invisible but without unmounting it

@mrjo118
Copy link
Contributor

mrjo118 commented Sep 26, 2024

I'm not able to view or download the first video. Are you saying that pressing a search result showing the note in split screen is a bug rather than a new feature? My presumption was that it was a feature to making looking through multiple search results easier

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Sep 26, 2024

Are you saying that pressing a search result showing the note in split screen is a bug rather than a new feature? My presumption was that it was a feature to making looking through multiple search results easier

It's a bug. However, I do think that having the ability to see search results at the same time as the note preview pane is useful, particularly on large-screen devices.

I'm not able to view or download the first video.

Edit: I've just converted it from a .mp4 to a .webm. Previously, the first video was a .mp4 encoded with ffmpeg and the second was a .webm captured by Ubuntu's (GNOME's) screen recorder.

@laurent22 laurent22 merged commit 42ab9ec into laurent22:dev Sep 27, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants