1397723 - Some strings show up as a tooltip even though the string was not found

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Bogdan Maris, Desktop Test Engineering]

Attachment: Screencast showing the issue

[Affected versions]:
- Firefox beta 56.0b9
- latest Nightly 57.0a1

[Affected platforms]:
- Windows 10 64bit
- macOS 10.12.6
- Ubuntu 16.04 32bit

[Steps to reproduce]:
1. Start Firefox
2. Go to about:preferences
3. Search for "type of"
4. Click "Exceptions" button where the tooltip is

[Expected result]:
- Search results show the exact string the user is searching for.

[Actual result]:
- "Type of" string does not exist inside the Exceptions dialog.

[Regression range]:
- This is not a regression since this is reproducible since the feature was first enabled by default (old nightly from 2017-06-16).

[Additional notes]:
- Screencast attached showing the issue.
- This is not an isolated case, I also can reproduce this using "the following c" string and quite possibly other.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1397723 - Correct Preferences search matching approach for search feature

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

The issue is cause by the string matching approach at stringMatchesFilters() [1]. I've no idea why we have to split query string into multiple string when string matching.

Current highlight results are incorrect due to the improper matching method. We split "type of" into "type" and "of" by whitespace. Because "type" matches in entire content "Type the exact address of the site ...", the search result will return true no matter "of" doesn't match in the second word.


[1] http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/findInPage.js#51-58
[2] http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/findInPage.js#57

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8906619 [details]
Bug 1397723 - Correct Preferences search matching approach for search feature

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8906619 [details]
Bug 1397723 - Correct Preferences search matching approach for search feature

https://reviewboard.mozilla.org/r/178344/#review183832

Thanks!

::: browser/components/preferences/in-content/findInPage.js:42
(Diff revision 2)
>      this.initializeCategories();
>      this.searchFunction(event);
>    },
>  
>    /**
> -   * Check that the passed string matches the filter arguments.
> +   * Check that the passed string matches the query string.

I think we can be more explicit here, and say that this returns true if content _contains_ the query.

::: browser/components/preferences/in-content/findInPage.js:49
(Diff revision 2)
> -   * @param String str
> -   *    to search for filter words in.
> -   * @param String filter
> -   *    is a string containing all of the words to filter on.
> +   * @param String content
> +   *    the text content to be searched
> +   * @param String query
> +   *    the query string
>     * @returns boolean
> -   *    true when match in string else false
> +   *    true when the query string matches in text content else false

`when the text content contains the query string`

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8906619 [details]
Bug 1397723 - Correct Preferences search matching approach for search feature

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b923cc5b501a
Correct Preferences search matching approach for search feature r=mconley

Comment 7

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

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

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8906619 [details]
Bug 1397723 - Correct Preferences search matching approach for search feature

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: minor search usability affected
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, see description
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: minor
[Why is the change risky/not risky?]: minor search usability affected
[String changes made/needed]: none

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8906619 [details]
Bug 1397723 - Correct Preferences search matching approach for search feature

Minor fix for pref search, let's uplift for beta 12.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Camelia would you mind just checking that this is fixed (in beta 12 once this lands). A quick check that the problem is gone, on one platform, should be fine. Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8850107a6a8b

Comment 12

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 x64, Ubuntu 16.04 x86 and macOS 10.12 using latest Nightly (2017-09-13) and Firefox 56 Beta 12 (buildID: 20170914024831): no results in Options after searching for "type of" - which is expected behaviour.