Closed Bug 1357352 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: Fischer, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1357352 - Hide data-hidden-from-search=true in Preferences search result
59 bytes, text/x-review-board-request
mconley
: review+
Details 
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
Blocks: 1335905
Whiteboard: [photon-preference]
(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
Target Milestone: --- → Firefox 56
Target Milestone: Firefox 56 → Firefox 55
Assignee: nobody → rchien
Status: NEW → ASSIGNED
QA Contact: hani.yacoub
Flags: qe-verify+
Attachment #8863985 - Flags: review?(jaws) → review?(mconley)
Priority: -- → P1
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".
Attachment #8863985 - Flags: review?(mconley) → review+
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!
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c219b4312a7
Hide data-hidden-from-search=true in Preferences search result r=mconley
https://hg.mozilla.org/mozilla-central/rev/5c219b4312a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Blocks: 1363062
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: