1293197 - 'Whole Words' after enabling not applied until change to search term

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 51.0a1 + 50.0a2 20160807 on Windows 8.1

After enabling 'Whole Words' in the find bar, this filter doesn't get applied until the search term changes (except you have 'Highlight All' enabled).

Steps to reproduce:
1. Open https://support.mozilla.org/en-US/kb/get-started-firefox-overview-main-features
2. Open findbar (e.g. Cmd/Ctrl+F).
3. Check that none of the findbar filters are active.
4. Type 'fire'.
5. Enable the 'Whole Words' filter.
Actual result:
"Fire" in page still highlighted and background of search box in find bar not red.
Expected result:
No highlight in page and background of search box red.

Don't get confused by 'Highlight All' not working properly when enabled, seems to bug 1200067.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

https://reviewboard.mozilla.org/r/77868/#review76322

We should probably have a test for this.

::: toolkit/content/widgets/findbar.xml:346
(Diff revision 1)
>              case "accessibility.typeaheadfind.casesensitive":
>                this._self._setCaseSensitivity(prefsvc.getIntPref(aPrefName));
>                break;
>              case "findbar.entireword":
>                this._self._entireWord = prefsvc.getBoolPref(aPrefName);
> -              this._self._updateEntireWord();
> +              this._self._setEntireWord(this._self._entireWord, true);

It seems confusing that we have two functions with very similar names, and this new change makes _setEntireWord get called twice.

Once when the toolbarbutton is called, which calls _setEntireWord through the oncommand attribute, then again from the prefObserver which is triggerd when _setEntireWord sets the pref.

When does the "entire-word"-related findUI get updated now? It used to get updated when the pref changed and when the findbar opened, now it will only get updated when the findbar is opened?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8789791 [details]
> Bug 1293197 - make sure that toggling Entire Words mode also retriggers the
> active search. Also fixes a JS error that crept in recently.
> 
> https://reviewboard.mozilla.org/r/77868/#review76322
> 
> We should probably have a test for this.
> 
> ::: toolkit/content/widgets/findbar.xml:346
> (Diff revision 1)
> >              case "accessibility.typeaheadfind.casesensitive":
> >                this._self._setCaseSensitivity(prefsvc.getIntPref(aPrefName));
> >                break;
> >              case "findbar.entireword":
> >                this._self._entireWord = prefsvc.getBoolPref(aPrefName);
> > -              this._self._updateEntireWord();
> > +              this._self._setEntireWord(this._self._entireWord, true);
> 
> It seems confusing that we have two functions with very similar names, and
> this new change makes _setEntireWord get called twice.

I agree. The gist of it is that the flow is analogous to `toggleHighlight` and `_setHighlightAll`; which means that I'd be better off renaming the functions to `toggleEntireWord` and keep `_setEntireWord`.


> Once when the toolbarbutton is called, which calls _setEntireWord through
> the oncommand attribute, then again from the prefObserver which is triggerd
> when _setEntireWord sets the pref.

Yup, that was my goal, because I only want one flow to set off the machinery. And the pref observer is _always_ invoked, thus I chose that is the pivot point in the flow.

> When does the "entire-word"-related findUI get updated now? It used to get
> updated when the pref changed and when the findbar opened, now it will only
> get updated when the findbar is opened?

No, still both. In fact, it's updated (almost) each time `_find` is called, which happens when the findbar is used.

Comment 4

[Mike de Boer [:mikedeboer]]

My question is: if I rename the methods in my patch, perhaps even `toggleHighlight` to `toggleHighlightAll`, would that make it reviewable again?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

Yeah, at least making the function names match the highlightAll code would help. 

This still needs a test though.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77868/diff/1-2/

Comment 7

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aaf22ac9572

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

https://reviewboard.mozilla.org/r/77868/#review78136

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdb72b431cc5
make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently. r=jaws

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Gerry Chang [:gchang]]

This feature is disabled in 51.

Comment 12

[roxleitan]

Verified fixed using the latest Nightly 52.0a1 (Build ID: 20161109030210) on Windows 8.1 , Ubuntu 16.04 and Mac OS X 10.12.

Comment 13

[eyal gruss]

does this also address the behavior when disabling whole words?
when i disable match case the search is triggered again, however when i disable whole words it is not (on version 50)

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to eyal gruss (eyaler) from comment #13)
> does this also address the behavior when disabling whole words?
> when i disable match case the search is triggered again, however when i
> disable whole words it is not (on version 50)

Yes, it would.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

Approval Request Comment
[Feature/regressing bug #]: bug 269442
[User impact if declined]: When clicking the 'Whole Words' button, the active search term and (when turned on) highlights are not refreshed right away, only after changing the search term.
[Describe test coverage new/current, TreeHerder]: landed on m-c, now on aurora/ 52.
[Risks and why]: minor, this was an oversight since the original implementation.
[String/UUID change made/needed]: n/a.

Comment 16

[Gerry Chang [:gchang]]

Hi Roxana,
This might also need your help to verify. Thanks.

Comment 17

[Brindusa Tot, Desktop QA]

Verified bug as fixed on 
  Nightly 53.0a1. Build ID 20161117030212


Verified fixed using the latest Nightly 52.0a1 (Build ID: 20161109030210) on Windows 8.1 , Ubuntu 16.04 and Mac OS X 10.12.

Comment 18

[Brindusa Tot, Desktop QA]

Sorry for previous commment.
(In reply to Brindusa Tot[:brindusat] from comment #17)
> Verified bug as fixed on 
>   Nightly 53.0a1. Build ID 20161117030212
> 
> 
> Verified fixed using the latest Nightly 52.0a1 (Build ID: 20161109030210) on
> Windows 8.1 , Ubuntu 16.04 and Mac OS X 10.12.


The correct comment is: 
Verified as fixed, with config paramter "findbar.modalHighlight" true and false, on
  - Nightly 53.0a1. Build ID 20161118030222
  - Aurora 52.0a2, Build ID 20161118004020

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8789791 [details]
Bug 1293197 - make sure that toggling Entire Words mode also retriggers the active search. Also fixes a JS error that crept in recently.

Fix an issue related to "Whole words" in search. Beta51+. Should be in 51 beta 2.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/606df4f05812

Comment 21

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161121093909

Verified as fixed using the latest Firefox 51 beta 2 (having preference findbar.modalHighlight true and false) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11. 

Setting status-firefox51 and status-firefox53 (as per comment 18) to verified.