1357352 - Should not display the element with data-hidden-from-search=true in the Preferences search result

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Fischer [:Fischer]]

STR:
1. Open about:config to set browser.preferences.search=true and browser.storageManager.enabled=false.
2. Enter about:preferences > Privacy & Security
3. Make sure the Site Data section is hidden
4. Search "site" in the search box

Expected: The Site Data section remains hidden

Actual: The Site Data section is displayed as one of the search results


The bug 1352374 has introduced one data-hidden-from-search attribute as a flag to let the search operation knows which element should not be displayed as the search result.

We could filter out those data-hidden-from-search elements at [1]. Then only search on ok-to-search elements at [2].

Should also add one test case in the browser/components/preferences/in-content/tests/browser_search_within_preferences.js


[1] https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/browser/components/preferences/in-content/findInPage.js#209
[2] https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/browser/components/preferences/in-content/findInPage.js#214

Comment 1

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #0)
> STR:
> 1. Open about:config to set browser.preferences.search=true and
> browser.storageManager.enabled=false.
> 2. Enter about:preferences > Privacy & Security
> 3. Make sure the Site Data section is hidden
> 4. Search "site" in the search box
> 
> Expected: The Site Data section remains hidden
> 
> Actual: The Site Data section is displayed as one of the search results
> 
> 
> The bug 1352374 has introduced one data-hidden-from-search attribute as a
> flag to let the search operation knows which element should not be displayed
> as the search result.
> 
> We could filter out those data-hidden-from-search elements at [1]. Then only
> search on ok-to-search elements at [2].
> 
> Should also add one test case in the
> browser/components/preferences/in-content/tests/
> browser_search_within_preferences.js
> 
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> a374c35469935a874fefe64d3e07003fc5bc8884/browser/components/preferences/in-
> content/findInPage.js#209
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> a374c35469935a874fefe64d3e07003fc5bc8884/browser/components/preferences/in-
> content/findInPage.js#214
Consider the case like,
```
<groupbox id="somePrivacySettingsGroup" data-category="panePrivacy" hidden="true">
  <caption><label>Some Privacy Settings</label></caption>
  <checkbox id="setting_1" label="Settings 1" />
  <checkbox id="setting_2" label="Settings 2" hidden="true" data-hidden-from-search="true"/>
</groupbox>
```
we might also need to stop searching on data-hidden-from-search child element inside the searchWithinNode call[3].
[3] https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/browser/components/preferences/in-content/findInPage.js#259

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8863985 [details]
Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8863985 [details]
Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

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

Comment 5

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

Comment on attachment 8863985 [details]
Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

https://reviewboard.mozilla.org/r/135706/#review139322

Thanks! I have some suggestions, see below.

::: browser/components/preferences/in-content/findInPage.js:206
(Diff revision 3)
> -      let rootPreferencesChildren = rootPreferences.children;
> +      let rootPreferencesChildren = Array.from(rootPreferences.children)
> +        .filter((node) => node.getAttribute("data-hidden-from-search") !== "true");

Alternatively,

```js
node.dataset.hiddenFromSearch !== "true";
```

or maybe even better:

```js

rootPreferences.querySelectorAll(":not([data-hidden-from-search])");

```
^-- since this will make the DOM do the work in the native layer instead of doing the filtering in JS.

::: browser/components/preferences/in-content/findInPage.js:299
(Diff revision 3)
>        matchesFound = matchesFound || complexTextNodesResult || labelResult || valueResult;
>      }
>  
>      for (let i = 0; i < nodeObject.childNodes.length; i++) {
>        // Search only if child node is not hidden
> -      if (!nodeObject.childNodes[i].hidden) {
> +      if (!nodeObject.childNodes[i].hidden && nodeObject.getAttribute("data-hidden-from-search") !== "true") {

As above, you _could_ use the dataset API here: https://developer.mozilla.org/en/docs/Web/API/HTMLElement/dataset

but I won't make you do it if you don't want to. :)

::: browser/components/preferences/in-content/tests/browser_search_within_preferences.js:194
(Diff revision 3)
> +  for (let i = 0; i < mainPrefTag.childElementCount; i++) {
> +    let child = mainPrefTag.children[i];
> +    if (child.id == "siteDataGroup") {
> +      is_element_hidden(child, "Should be in search results");
> +    }
> +  }

Instead of looping through the children like this, can't we query the mainPrefTag element like:

```js
let child = mainPrefTag.querySelector("#siteDataGroup");
is_element_hidden(child, "Should be in search results");
```

Also, that message should probably be "Should be hidden in search results".

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8863985 [details]
Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

https://reviewboard.mozilla.org/r/135706/#review139322

> As above, you _could_ use the dataset API here: https://developer.mozilla.org/en/docs/Web/API/HTMLElement/dataset
> 
> but I won't make you do it if you don't want to. :)

Yep, I tried dataset at the first time but unfortunately I think XUL element hasn't support dataset attribute. dataset always return undefined in this scenario. But I'm in favor of using dataset if we can!

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8863985 [details]
Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c219b4312a7
Hide data-hidden-from-search=true in Preferences search result r=mconley

Comment 9

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/5c219b4312a7

Comment 10

[Hani Yacoub, Desktop QA]

Build ID: 20170515030205
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.