Fischer [:Fischer] Assignee
	Description • 8 years ago

The currently search function in Preferences might accidentally search on the pref-off element. One example is as below:
While go to the panePrivacy category, it would run a search[1] and show all the groups with `data-category="panePrivacy"` As a result, the pref-off Site Data group is revealed. See the attached Site_Data_Section_Leaked_from_Search.png. 


[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#169

	Fischer [:Fischer] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8853365 [details]
Bug 1352374 - Part 1: Exclude element with data-hidden-from-search=true from search results,

Hi Jaws,

The patch is divided into 2 parts.
Part 1: Exclude the unexpected search result
Part 2: Update codes around Site Data btw

Thanks

	Fischer [:Fischer] Assignee
	Comment 4 • 8 years ago

(In reply to Fischer [:Fischer] from comment #3)
> Comment on attachment 8853365 [details]
> Bug 1352374 - Part 1: Exclude element with data-out-of-search=true from
> search results
> 
> Hi Jaws,
> 
> The patch is divided into 2 parts.
> Part 1: Exclude the unexpected search result
> Part 2: Update codes around Site Data btw
> 
> Thanks
Hi Jaws,
To be clear.
The purpose of Part 2 is to make in-content align in-content-old as well as move some codes to the right place.
Thanks

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8853365 [details]
Bug 1352374 - Part 1: Exclude element with data-hidden-from-search=true from search results,

https://reviewboard.mozilla.org/r/125484/#review128126

r- because I think your change where you're setting the attribute to true within privacy.js seems the opposite of what you intended.

::: browser/components/preferences/in-content/preferences.js:182
(Diff revision 1)
> +    // XXX:
> +    // If the "data-out-of-search" is "true",
> +    // then it means that element do not want to be searched on.
> +    // This is because some elements might be hidden for pref-off or whatever reason.
> +    // In this case, those element should be excluded from search result.

Please remove the XXX and rewrite the comment to:
// If the "data-hidden-from-search" is "true", the
// element will not get considered during search. This
// should only be used when an element is still under
// development and should not be shown for any reason.

::: browser/components/preferences/in-content/preferences.js:187
(Diff revision 1)
> +    // XXX:
> +    // If the "data-out-of-search" is "true",
> +    // then it means that element do not want to be searched on.
> +    // This is because some elements might be hidden for pref-off or whatever reason.
> +    // In this case, those element should be excluded from search result.
> +    if (element.getAttribute("data-out-of-search") != "true") {

s/data-out-of-search/data-hidden-from-search/

::: browser/components/preferences/in-content/privacy.js:265
(Diff revision 1)
>        let bundlePrefs = document.getElementById("bundlePreferences");
>        document.getElementById("offlineAppsList")
>                .style.height = bundlePrefs.getString("offlineAppsList.height");
>        let offlineGroup = document.getElementById("offlineGroup");
>        offlineGroup.hidden = false;
> +      offlineGroup.setAttribute("data-out-of-search", true);

Isn't this doing the opposite of what you want? When the pref is set to true, shouldn't this attribute get removed or set to false?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8853366 [details]
Bug 1352374 - Part 2: Move a few sections of code that were missed when rebasing bug 1335907.

https://reviewboard.mozilla.org/r/125486/#review128128

::: commit-message-e65fd:1
(Diff revision 1)
> +Bug 1352374 - Part 2: Update codes about Site Data

Thank you for fixing this, but please try to write a more descriptive commit message. For this one you can use:

Bug 1352374 - Move a few sections of code that were missed when rebasing bug 1335907. r=jaws

	Jared Wein [:jaws] (please needinfo? me)
	Comment 7 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Thank you for fixing this

I am deeply sorry for your work getting lost here. I appreciate you getting a patch written so quickly to put it back. I am going through the list of changesets that touched browser/components/preferences since the project began to make sure there are not any other changes lost.

	Fischer [:Fischer] Assignee
	Comment 8 • 8 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8853365 [details]
> ::: browser/components/preferences/in-content/preferences.js:182 (Diff revision 1)
> > +    // XXX:
> > +    // If the "data-out-of-search" is "true",
> > +    // then it means that element do not want to be searched on.
> > +    // This is because some elements might be hidden for pref-off or whatever reason.
> > +    // In this case, those element should be excluded from search result.
> 
> Please remove the XXX and rewrite the comment to:
> // If the "data-hidden-from-search" is "true", the
> // element will not get considered during search. This
> // should only be used when an element is still under
> // development and should not be shown for any reason.
> 
Thanks, will update.

> ::: browser/components/preferences/in-content/preferences.js:187 (Diff revision 1)
> > +    if (element.getAttribute("data-out-of-search") != "true") {
> 
> s/data-out-of-search/data-hidden-from-search/
> 
Thanks, will take "data-hidden-from-search"

> ::: browser/components/preferences/in-content/privacy.js:265 (Diff revision 1)
> > +      offlineGroup.setAttribute("data-out-of-search", true);
> 
> Isn't this doing the opposite of what you want? When the pref is set to
> true, shouldn't this attribute get removed or set to false?
Thanks, yes this is doing the opposite intention. Update the patch too quickly. Sorry for that, will fix it.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8853365 [details]
Bug 1352374 - Part 1: Exclude element with data-hidden-from-search=true from search results,

https://reviewboard.mozilla.org/r/125484/#review128570

	Fischer [:Fischer] Assignee
	Comment 12 • 8 years ago

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd97a9cfaef1f6a69ce09895925ddb170f0bda3

	Pulsebot
	Comment 14 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/180a1001ddaa
Part 1: Exclude element with data-hidden-from-search=true from search results, r=jaws
https://hg.mozilla.org/integration/autoland/rev/a5c309cc22d8
Part 2: Move a few sections of code that were missed when rebasing bug 1335907. r=jaws

	Fischer [:Fischer] Assignee
	Comment 15 • 8 years ago

Hi Wes,
Would you please help to push this into m-c?
It is ready to land, thanks

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/180a1001ddaa
https://hg.mozilla.org/mozilla-central/rev/a5c309cc22d8

	Camelia Badau [:cbadau], Desktop QA
	Comment 17 • 7 years ago

Verified fixed on Windows 7 x64, Windows 10 x86, Mac OSX 10.12.4 and Ubuntu 16.04 x64 using latest Nightly 55.0a1 (2017-05-10).