Closed Bug 1293197 Opened 4 years ago Closed 3 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox50 --- affected
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: aryx, Assigned: mikedeboer)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: qe-verify+
Flags: firefox-backlog+
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?
Attachment #8789791 - Flags: review?(jaws) → review-
(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.
My question is: if I rename the methods in my patch, perhaps even `toggleHighlight` to `toggleHighlightAll`, would that make it reviewable again?
Flags: needinfo?(jaws)
Attachment #8789791 - Flags: review- → review?(jaws)
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.
Flags: needinfo?(jaws)
Attachment #8789791 - Flags: review?(jaws)
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
Attachment #8789791 - Flags: review?(jaws) → review+
Pushed by mdeboer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/bdb72b431cc5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
This feature is disabled in 51.
Verified fixed using the latest Nightly 52.0a1 (Build ID: 20161109030210) on Windows 8.1 , Ubuntu 16.04 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
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)
(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 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.
Attachment #8789791 - Flags: approval-mozilla-beta?
Hi Roxana,
This might also need your help to verify. Thanks.
Flags: needinfo?(roxana.leitan)
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.
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
Flags: needinfo?(roxana.leitan)
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.
Attachment #8789791 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
You need to log in before you can comment on or make changes to this bug.