Closed Bug 1198279 Opened 4 years ago Closed 3 years ago

Closing Find Toolbar should clear all highlighted matches

Categories

(Toolkit :: Find Toolbar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ato, Assigned: ralin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

When you search for a term in-page using the Find Toolbar and select the “Highlight All” button, the found terms are not de-highlighted when the toolbar closes (either through pressing Escape or X’ing out the toolbar).

I’d expect the highlighting to disappear when I press Escape or the Find Toolbar closes.

Otherwise I will have to press Control + F to open the toolbar again and click the “Highlight All” button again to remove the highlighting.

(Related, I think it’s interesting behaviour that the toolbar doesn’t reopen when you press F3 to search for the next occurrence, but maybe this is a separate bug/feature request?)
Duplicate of this bug: 395357
Priority: -- → P1
Summary: Closing Find Toolbar should not highlight matches → Closing Find Toolbar should clear all highlighted matches
Yup , When something is being searched by doing cntl+f "recycle" for normal google search on paper recycle. Shows 13 results. In order to exit from the find tool bar below we do ESCAPE which should clear the previously typed characters. 

Upon closing of find toolbar , previously searched characters should be erased.
Blocks: 1271782
Attached patch 0001-Bug-1198279-WIP.patch (obsolete) — Splinter Review
Hi, Gijs

I got a patch and it is a very simple and straightforward thought to clear all matches.  

also I'd like to take this bug if possible, but not sure it is proper to NI you here.

Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8766694 [details] [diff] [review]
0001-Bug-1198279-WIP.patch

Review of attachment 8766694 [details] [diff] [review]:
-----------------------------------------------------------------

Mike is a better reviewer here.
Attachment #8766694 - Flags: review?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment on attachment 8766694 [details] [diff] [review]
0001-Bug-1198279-WIP.patch

Review of attachment 8766694 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/findbar.xml
@@ +762,4 @@
>              this.setAttribute("noanim", true);
>            this.hidden = true;
>  
> +          this.toggleHighlight(false);

Hi Jay, thanks for picking up this bug!

I'm afraid that `toggleHighlight` is doing too much for what we need here...

When you look down two lines you see a call to `onFindbarClose()` that ends up at Finder.jsm in the content process (see http://searchfox.org/mozilla-central/source/toolkit/modules/Finder.jsm#298)
There it calls `FinderHighlighter#hide()` (see http://searchfox.org/mozilla-central/source/toolkit/modules/FinderHighlighter.jsm#283) which, at the moment, doesn't do anything when the highlighting mode is not modal. That's where you come in!
Attachment #8766694 - Flags: review?(mdeboer) → feedback+
Thanks for your reply Mike.

I got a question about expected behavior: Should we re-highlight the last matches when reopen find bar? If so, "re-highlight" will take place at findbar#open() according to pref `findbar.highlightAll`.
I think it matters because we need to make sure that selections in content and find bar UI are consistent.

Please feel free to correct me if any mistake. Thanks.
Flags: needinfo?(mdeboer)
(re-)Highlight-all is only triggered from the findbar, thus from chrome. This means that no state is kept in the content/ frame scripts (Finder.jsm, FinderHighlighter.jsm) with respect to selections. We try to keep it simple.
Highlight-all feature is only second in priority to the actual find-in-page feature, thus if we were to start a find when the findbar is re-opened, we should also highlight all the matches. But at the moment we don't immediately do a find when the findbar is re-opened, only on F3 or Command+g
Short answer: no.
Flags: needinfo?(mdeboer)
Thanks. My understanding about "Highlight All" is like (just took a note here...)

     update pref                      trigger                
pref <---------->  toggleHighlight  <----------> findbar "Highlight All" button
    observe/trigger      |            update UI
                         |
                         | highlight/de-highlight
                         |
                         V
                 Selection in content

anyway, I think I got the point and your clarification did help me a lot to understand how it works, thanks.
keep the flow simple, that is, clearing the selection doesn't mean we need to turn the "highlight all" feature/pref off. Compare to an action/trigger, "highlight all" plays the role more like a setting. Find should be the trigger for all update.

I'll attach a mozreivew soon, and thanks again.
Comment on attachment 8767871 [details]
Bug 1198279 - Clear all highlighted matches when find bar closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62246/diff/1-2/
With regard to nested frames and editable content, I think reuse #highlight() might be a good way to remove highlighting perfectly.
Attachment #8767871 - Flags: review?(mdeboer) → review-
Comment on attachment 8767871 [details]
Bug 1198279 - Clear all highlighted matches when find bar closed.

https://reviewboard.mozilla.org/r/62246/#review59076

::: toolkit/modules/FinderHighlighter.jsm:284
(Diff revision 2)
>    /**
> -   * If modal highlighting is enabled and the outline + dimmed background is
> -   * currently visible, both will be hidden.
> +   * Clear all highlighted matches. If modal highlighting is enabled and
> +   * the outline + dimmed background is currently visible, both will be hidden.
>     */
>    hide(window = null) {
> +    this.highlight(false);

I think we need to move the `removeAllRanges`-related code from the `highlight()` method to this `hide()` method instead.
When bug 1280525 lands, this will cause a never-ending loop, which would be unfortunate.
Attachment #8766694 - Attachment is obsolete: true
Comment on attachment 8767871 [details]
Bug 1198279 - Clear all highlighted matches when find bar closed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62246/diff/2-3/
Attachment #8767871 - Flags: review- → review?(mdeboer)
Attachment #8767871 - Flags: review?(mdeboer) → review+
Comment on attachment 8767871 [details]
Bug 1198279 - Clear all highlighted matches when find bar closed.

https://reviewboard.mozilla.org/r/62246/#review59396

Splendid! Thanks! (Please push this changeset to try before landing)
No problem. my fault, I should ask you the try syntax for find bar right after I pushed mozreview.

I noticed that some failures on the treeherder. They don't look related to this bug, but in case my inexperience in find bar... could you help me to look into it? Thanks!
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a06bc9ffb12c
Clear all highlighted matches when find bar closed. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/a06bc9ffb12c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
See Also: → 1286660
You need to log in before you can comment on or make changes to this bug.