Closed Bug 1561583 Opened 1 year ago Closed 1 year ago

[UX] Do not clear search input when closing search-in-file

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: fvsch, Assigned: janelledement)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Steps to reproduce:

  1. Open a file in debugger
  2. Hit Cmd+F (or Ctrl+F) and type e.g. "hello"
  3. Hit the ESC key or click the close button
  4. Hit Cmd+F (or Ctrl+F) again

Expected result:

  • The search field value would be "hello", and selected so that typing replaces this value

Actual result:

  • The search field is empty

The expected result is similar to what VS Code (and, incidentally, the Style Editor) do. It's a good way to not loose your previous search, while allowing users to replace it effortlessly.

Something to watch out for: when selecting some text in the file and hitting Cmd+F, this text is used as the search value; which is great and consistent with e.g. VS Code. Any fix should preserve this behavior.

(I'm also wondering if this behavior is causing this UX issue in the first place, if we're setting the search input's value to the current selection which is empty, so it empties the search field?)

Blocks: dbg-frontend
Keywords: good-first-bug
Priority: -- → P3

I've looked into this a little bit and so far there seems to be two things that are causing this problem.

  1. When a user opens the search bar, toggleSearch() is run. This function does not allow the search bar to be populated with a query, unless searchOn is set to true (which is only the case when the search bar is already open).

  2. When the search bar is escaped or closed, the query is being set to an empty string (inside closeSearch()). So even if the first problem is fixed, the query will still show up as an empty string.

My solution would be to make toggleSearch() wait for this.props.searchOn to be set to true if the search bar needs to be opened first, and not allowing closeSearch() to reset query to an empty string. @jlast, let me know what you think of this approach.

Let’s give that a shot and see how it feels!

This patch allows the search bar to retain the previous query on re-opening.

Attachment #9075483 - Attachment description: Search bar retains previous query on re-opening. * First draft 1561583 fix * Minor Change → Search bar retains previous query on re-opening.
Assignee: nobody → janelledement
Status: NEW → ASSIGNED
Blocks: 1565711
Blocks: 1565713

@davidwalsh @jlast I did some digging and figured out a few things.

If I go back to the original commit, all mochitests pass again.

However, if I revert to that change-set, the highlight problem is unsurprisingly back when the searchBar is escaped with a query inside. (This is problem David pointed out on the patch).

The reason this is happening is because when the escape key is pressed (as opposed to x-ing out of searchBar) something different happens (summarized below)

X out search bar:
• Search is cleared
• Search bar closed

Escaping search bar:
• Search bar closed (not cleared)

The problem I’m having is when I add code to clear the search (whether in the App.js handler or in the SearchBar.js handler), I fail other existing searchBar related mochitest(s).

Is changing editing an existing mochitest a ‘no no’?

Also as a side note: this problem might deserve its own bug since it existed on versions before this patch was made. I think it’s a regression actually. If you like, I could look into when this happened if it’s important to know.

Awesome research Janelle!

Changing a mochitest is great if the change that you're making is behavioral, which this is. Which test fails and for what reason? If we're checking that search opens with no value, for example, which it will no longer do, it will fail. Keeping tests up to date is great!

With my latest fix for clearing as well as closing the search on escape, browser_dbg-search-file.js fails. There seems to be a timing issue that was making most of the assertions fail. I added a line that solves the timing problem. I'll commit this shortly.

No longer blocks: 1565711
No longer blocks: 1565713
Pushed by dwalsh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/640aff6b43b5
Search bar retains previous query on re-opening. r=davidwalsh
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
QA Whiteboard: [qa-70b-p2]
You need to log in before you can comment on or make changes to this bug.