Closed Bug 1352374 Opened 3 years ago Closed 3 years ago

Unexpectedly search on the pref-off Site Data section in Preferences

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(3 files)

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
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
Attachment #8853365 - Flags: review?(jaws)
Attachment #8853366 - Flags: review?(jaws)
(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
Assignee: nobody → fliu
Status: NEW → ASSIGNED
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?
Attachment #8853365 - Flags: review?(jaws) → 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
Attachment #8853366 - Flags: review?(jaws) → review+
(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.
(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.
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
Attachment #8853365 - Flags: review?(jaws) → review+
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
Keywords: checkin-needed
Hi Wes,
Would you please help to push this into m-c?
It is ready to land, thanks
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/180a1001ddaa
https://hg.mozilla.org/mozilla-central/rev/a5c309cc22d8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Whiteboard: [storage-v1]
Flags: needinfo?(wkocher)
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).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.