1198279 - Closing Find Toolbar should clear all highlighted matches

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

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?)

Comment 2

[arjunakumar92]

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.

Comment 3

[Ray Lin[:ralin]]

Attachment: 0001-Bug-1198279-WIP.patch

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.

Comment 4

[:Gijs (he/him)]

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.

Comment 5

[Mike de Boer [:mikedeboer]]

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!

Comment 6

[Ray Lin[:ralin]]

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.

Comment 7

[Mike de Boer [:mikedeboer]]

(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.

Comment 8

[Ray Lin[:ralin]]

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 9

[Ray Lin[:ralin]]

Attachment: Bug 1198279 - Clear all highlighted matches when find bar closed.

Review commit: https://reviewboard.mozilla.org/r/62246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62246/

Comment 10

[Ray Lin[:ralin]]

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/

Comment 11

[Ray Lin[:ralin]]

With regard to nested frames and editable content, I think reuse #highlight() might be a good way to remove highlighting perfectly.

Comment 12

[Mike de Boer [:mikedeboer]]

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.

Comment 13

[Ray Lin[:ralin]]

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/

Comment 14

[Mike de Boer [:mikedeboer]]

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)

Comment 15

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=023eb4569b35

Comment 16

[Ray Lin[:ralin]]

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!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a06bc9ffb12c
Clear all highlighted matches when find bar closed. r=mikedeboer

Comment 18

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/a06bc9ffb12c