Closed Bug 1335454 Opened 7 years ago Closed 7 years ago

Add the ability to open the "Data Choices" options section to the UITour

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Version:
52 Branch
Type:
defect

Tracking

()

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

People

(Reporter: verdi, Assigned: Fischer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files)

firefox-preferences-updates.png
116.33 KB, image/png
Details
Bug 1335454 - Add the ability to open the "Data Choices" options section through UITour,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
We want links on the Firefox Privacy Notice - https://www.mozilla.org/en-US/privacy/firefox/ to be able to directly open about:preferences#advanced to the data choices tab when it's viewed in Firefox.
Nicole, we'll need this likely prior to v57.  Can you track it as part of the Taipei team's work?
Flags: needinfo?(nyee)
Flags: needinfo?(nyee)
Blocks: 1354686
No longer blocks: 1332318
Might be a good bug for the TPE team to start working on Monday? Michael will be working on the Tour Overlay UX Specs till Thursday (TPE time). So looks like there might be 3 days of downtime before work on the tour overlay begins.
Evan and Fisher what do you think? If you can pick it up, that would be great. If not, no worries.

Sync up with Michael Verdi to get details.
Flags: needinfo?(evan)
Flags: needinfo?(evan)
Hi Nicole and Verdi,

This issue seems not be actionable yet before we have the UITour (OnBoarding overlay) design spec.

And I have two questions here
1. Since we've re-designed and updated the Preferences in Bug 1335907 (the spec[1]), the "Data Choices" options section does not exist in current mozilla-central build (see in the attachment). What do we want to do for the design change? For instance, scroll to the the "Reports" section in the design spec[1] or any other idea?

2. Why do we need to make the links on the below web page directly open `about:preferences#advanced`? Currently, we cannot open the `about:preferences#advanced` page from a general web page. It's a security issue. Gecko doesn't allow us to do that (source code [2]).
> We want links on the Firefox Privacy Notice - https://www.mozilla.org/en-US/privacy/firefox/ to be able to directly open about:preferences#advanced to the data choices tab when it's viewed in Firefox. But for sure, we can directly open the Preferences page in the UITour (OnBoarding overlay).

[1]: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167550
[2]: http://searchfox.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#95-96
Flags: needinfo?(nyee)
Flags: needinfo?(mverdi)
CC team members.
And Bryant is also helping us understand the request of this bug. We might have a clear idea after tomorrow's UX meeting. Thanks, Bryant.
For 1. IMHO we can do what you suggests, mverdi should make a decision here.

(In reply to Evan Tseng [:evanxd] from comment #3)
> 2. Why do we need to make the links on the below web page directly open
> `about:preferences#advanced`? Currently, we cannot open the
> `about:preferences#advanced` page from a general web page. It's a security
> issue. Gecko doesn't allow us to do that (source code [2]).
> > We want links on the Firefox Privacy Notice - https://www.mozilla.org/en-US/privacy/firefox/ to be able to directly open about:preferences#advanced to the data choices tab when it's viewed in Firefox. But for sure, we can directly open the Preferences page in the UITour (OnBoarding overlay).

This can be done with code like bug 1177169. Please talk a look at that.
Hi Michael and Bryant,

One more question for design spec, we would like to directly open `about:preferences#advanced` in the UITour, right? Or we want to do it on a general web page (like https://www.mozilla.org/en-US/privacy/firefox/)?

A little confused about the design,
because we seemed say that we want to open `about:preferences#advanced` on the OnBoarding overlay UI. (saw in the bug title)
> Add the ability to open the "Data Choices" options section to the UITour

But we also said we want to open it in a general web page. (saw in Comment 0)
> We want links on the Firefox Privacy Notice - https://www.mozilla.org/en-US/privacy/firefox/ to be able to directly open about:preferences#advanced to the data choices tab when it's viewed in Firefox.

Which one (behavior) is what we want?

Thanks.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> For 1. IMHO we can do what you suggests, mverdi should make a decision here.
> 
> (In reply to Evan Tseng [:evanxd] from comment #3)
> > 2. Why do we need to make the links on the below web page directly open
> > `about:preferences#advanced`? Currently, we cannot open the
> > `about:preferences#advanced` page from a general web page. It's a security
> > issue. Gecko doesn't allow us to do that (source code [2]).
> > > We want links on the Firefox Privacy Notice - https://www.mozilla.org/en-US/privacy/firefox/ to be able to directly open about:preferences#advanced to the data choices tab when it's viewed in Firefox. But for sure, we can directly open the Preferences page in the UITour (OnBoarding overlay).
> 
> This can be done with code like bug 1177169. Please talk a look at that.
Thanks for the info.
And after checking with Gijs and Jaws on this for the further details.
The UITour-lib.js could achieve the goal of opening about:preferences from an external secure mozilla page.
Here are the doc[1] and the bug[2] for it.

However, still need to check what we want to do in this bug as Evan's Comment 7.
The bug title and the description are kind of confusing.

[1] http://bedrock.readthedocs.io/en/latest/uitour.html
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1310150
Thanks for looking into things but please don't nitpick the bug summary etc. We can help :verdi to identify what's actually needed here.

Yes, here is something we already have from [1] in comment 8:

1. Go to https://www.mozilla.org/en-US/firefox/firstrun/
2. Open console
3. run Mozilla.UITour.openPreferences('advanced')

On the latest Nightly the Data Choices checkboxes are located at the "Reports" section of the advanced panel. If that's good enough there is nothing to work on here (as there is no longer a "Data Choices" tab to switch to). We are also fortunately enough as the "advanced" keyword is kept on the new pref.

However, it's possible for users with smaller screen, they will not see the "Reports" section unless they scroll down. Do we want to scroll down to that section for the user?
(In reply to Evan Tseng [:evanxd] from comment #3)
> This issue seems not be actionable yet before we have the UITour (OnBoarding
> overlay) design spec.

Evan, I think you are confused with UITour (the library) and onboarding (the feature, unrelated to what's needed here). Unless I misread your comment there. If not, your question in comment 3 and comment 7 are invalid.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)

> However, it's possible for users with smaller screen, they will not see the
> "Reports" section unless they scroll down. Do we want to scroll down to that
> section for the user?

Yes we'd want to scroll down to that section.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> We are also fortunately enough as the "advanced" keyword is kept
> on the new pref.
> 

So does this mean that if we move this section to Privacy & Security it will still work?
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #11)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #9)
> > We are also fortunately enough as the "advanced" keyword is kept
> > on the new pref.
> > 
> 
> So does this mean that if we move this section to Privacy & Security it will
> still work?

I was thinking about the fact that the "advanced" keyword will take the user to the same panel regardless of Firefox versions, but given that we need the exact options to be visible it's not really enough, so never mind.

The requirement here, I guess, is that wherever we move the options we want the UITour to make it visible to the user, so we will need to make it work if we move.

(Are we moving the options?)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)

> However, it's possible for users with smaller screen, they will not see the
> "Reports" section unless they scroll down. Do we want to scroll down to that
> section for the user?

Yeah, this is a known issue with the new prefs reorg - some of the pages are longer, and some existing links to prefs are now confusing when the relevant bit is too far down.

See bug 1353832. (The specific approach there is WONTFIX to decouple it from the new search functionality, instead AIUI we're adding special-case links as in bug 1353805 and bug 1353808. I suspect you'll want to do the same here.
Yes, the patch for this bug should follow the approach in bug 1353805 and bug 1353808. Please wait for those patches to get r+ so that the approach has been validated.
Depends on: 1353805, 1353808
Flags: needinfo?(nyee)
Depends on: 1358394
Is this under your radar? This is asked a week ago and it's dependency has since been fixed. Please see what's needed to exposed this feature to UITour library.
Assignee: nobody → evan
Status: NEW → ASSIGNED
Flags: needinfo?(evan)
Hi Tim,

I'm working on performance settings related (priorities) bugs. And this is in the scope of onboarding. Let's unassign me.
Assignee: evan → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(evan)
Depends on: 1358475
For the new Preferences, the external page should be able open the reports subcategory (the subcategory holds the options of sending data to Mozilla") by calling `Mozilla.UITour.openPreferences('privacy-reports')`. However there are still 2 bugs (1358394 and 1358475) about loading Preferences which require fixing or user might face abnormal experience.

For the old Preferences(Under, including FF 54), we still need to update Moz UITour to make it able to open to Preferences > Network > Data Choices.
Assignee: nobody → fliu
Blocks: 1360468
(In reply to Fischer [:Fischer] from comment #18)
> Created attachment 8862895 [details]
> Bug 1335454 - Add the ablity to open advanced tab in the old Preferences
> through UITour,
> 
> Enhance `Mozilla.UITour.openPreferences` to open advanced tab in the old
> Preferences. A note is added to remind user using `navigator.userAgent` to
> test Firefox version before calling the api.
> 
> Review commit: https://reviewboard.mozilla.org/r/134772/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/134772/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6269af7f4cf6f76fd4154b062622ef0481135927
Comment on attachment 8862895 [details]
Bug 1335454 - Add the ability to open the "Data Choices" options section through UITour,

https://reviewboard.mozilla.org/r/134772/#review137752

::: browser/base/content/utilityOverlay.js:718
(Diff revision 1)
>  function openPreferences(paneID, extraArgs) {
>    function switchToAdvancedSubPane(doc) {
>      if (extraArgs && extraArgs["advancedTab"]) {
> +      // After the Preferences reorg works in Bug 1335907, no more advancedPrefs element.
> +      // The old Preference is pref-off behind `browser.preferences.useOldOrganization` on Nightly.
> +      // During the tranistion between the old and new Preferences, should do checking before proceeding.

spelling error, "transition" instead of "tranistion"

::: browser/components/uitour/UITour-lib.js:749
(Diff revision 1)
>     * @param {String} pane - Pane to open/switch the preferences to.
>     * Valid values match fragments on about:preferences and are subject to change e.g.:<ul>
>     * <li>general

Please update this list to state which values are supported for the various versions of preferences.

::: browser/components/uitour/UITour-lib.js:766
(Diff revision 1)
> +   * please utilize `navigator.userAgent` to check Firefox version.
> +   * Before Firefox 54(included), please call `openPreferences("dataChoicesTab")`.
> +   * After Firefox 55(included), please call `openPreferences("privacy-reports")`.
> +   * the valid id values are below and subject to change:
> +   * <ul>
> +   * <li> generalTab - the General tab
> +   * <li> dataChoicesTab - the Data Choices tab
> +   * <li> networkTab - the Network tab
> +   * <li> updateTab - the Update tab
> +   * <li> encryptionTab - the Certificates tab
> +   * </ul>
> +   *
>     * @since 42
>     */
> -  Mozilla.UITour.openPreferences = function(pane) {
> +  Mozilla.UITour.openPreferences = function(pane, advancedTab) {

We shouldn't add a second argument here. It would be wrong to determine what to open based on the userAgent version since the pref can be flipped independent of the version and we may want to slowly enable the new preferences for a small amount of people first and thus basing this off of the version number would be wrong for some subset of people.

Instead, callers should just request that "privacy-reports" be opened, and inside of this function please check the `browser.preferences.useOldOrganization` pref and if that pref is set to true, then you should set extraArgs to {advancedTab: "dataChoicesTab"}.

::: browser/components/uitour/UITour-lib.js:781
(Diff revision 1)
> +   * </ul>
> +   *
>     * @since 42
>     */
> -  Mozilla.UITour.openPreferences = function(pane) {
> +  Mozilla.UITour.openPreferences = function(pane, advancedTab) {
> +    let extraArgs = undefined;

let extraArgs;
Attachment #8862895 - Flags: review?(jaws) → review-
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #20)
> ::: browser/components/uitour/UITour-lib.js:766
> (Diff revision 1)
> > +   * please utilize `navigator.userAgent` to check Firefox version.
> > +   * Before Firefox 54(included), please call `openPreferences("dataChoicesTab")`.
> > +   * After Firefox 55(included), please call `openPreferences("privacy-reports")`.
> > +   * the valid id values are below and subject to change:
> > +   * <ul>
> > +   * <li> generalTab - the General tab
> > +   * <li> dataChoicesTab - the Data Choices tab
> > +   * <li> networkTab - the Network tab
> > +   * <li> updateTab - the Update tab
> > +   * <li> encryptionTab - the Certificates tab
> > +   * </ul>
> > +   *
> >     * @since 42
> >     */
> > -  Mozilla.UITour.openPreferences = function(pane) {
> > +  Mozilla.UITour.openPreferences = function(pane, advancedTab) {
> 
> We shouldn't add a second argument here. It would be wrong to determine what
> to open based on the userAgent version since the pref can be flipped
> independent of the version and we may want to slowly enable the new
> preferences for a small amount of people first and thus basing this off of
> the version number would be wrong for some subset of people.
> 
> Instead, callers should just request that "privacy-reports" be opened, and
> inside of this function please check the
> `browser.preferences.useOldOrganization` pref and if that pref is set to
> true, then you should set extraArgs to {advancedTab: "dataChoicesTab"}.
> 
Hi Jaws,
OK, looks like the combinations are a bit complex here. We now having 3 cases:
 - Case A: FF 54 with always the old Preferences
 - Case B: FF 55 with the old Preferences still enabled
 - Case C: FF 55 with the new Preferences enabled
Unlike FF which user may not upgrade, the UITour-lib.js can be deployed by websites and take effect right away.
An inconsistent match between FF and the UITour-lib.js could happen.

If I wasn't wrong, there is no direct way for the website script, UITour-lib.js to know the `browser.preferences.useOldOrganization` pref state, so an new event asking FF about the pref state is needed.
Introducing an new event might increase the complexity and face the inconsistent match against old FF versions. Probably not a ideal plan.

Alternative plan might be user still tests userAgent
 - for under FF 54, calls `Mozilla.UITour.openPreferences("advanced", "dataChoicesTab")`
 - for above FF 55, calls `Mozilla.UITour.openPreferences("privacy-reports")`
   - In UITour.jsm, we detect if is the old Preferences. If still old Preferences, then mapping "privacy-reports to "dataChoicesTab" of "advanced".

How do you think?

Thanks
Flags: needinfo?(jaws)
I agree that we should not be sending an event with the current value of browser.preferences.useOldOrganization.

I think we should only support opening this with "privacy-reports". We can call openPreferences("advanced", "dataChoicesTab") if the pref doesn't exist or is set to false. This should be a simple patch that you can uplift to FF54.
Flags: needinfo?(jaws)
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #22)
> I agree that we should not be sending an event with the current value of
> browser.preferences.useOldOrganization.
> 
> I think we should only support opening this with "privacy-reports". We can
> call openPreferences("advanced", "dataChoicesTab") if the pref doesn't exist
> or is set to false. This should be a simple patch that you can uplift to
> FF54.
The current pref is `browser.preferences.useOldOrganization` and doesn't exist by default, which means we will want to call `openPreferences("advanced", "dataChoicesTab")` when it is there and set to true.

OK so if we are going to do uplifting to FF54, that means one extra `browser.preferences.useOldOrganization=true` has to be added
 in the patch to uplift.
No, we shouldn't need to add the pref to allow for uplift. We can use the second argument of Services.prefs.getBoolPref to provide the default value if the pref doesn't exist. See http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/modules/libpref/nsIPrefBranch.idl#67 for more details.
(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #24)
> No, we shouldn't need to add the pref to allow for uplift. We can use the
> second argument of Services.prefs.getBoolPref to provide the default value
> if the pref doesn't exist. See
> http://searchfox.org/mozilla-central/rev/
> ae8c2e2354db652950fe0ec16983360c21857f2a/modules/libpref/nsIPrefBranch.
> idl#67 for more details.
Currently in FF55 `browser.preferences.useOldOrganization` doesn't exist by default and all the codes are treating `browser.preferences.useOldOrganization = false` if the pref not found[1][2].

As a result while let website call `openPreferences("privacy-reports")` uniformly and in the UITour.jsm if we do remapping such as
```
  if (Services.prefs.getBoolPref("browser.preferences.useOldOrganization", true)) {
    // In FF 55, no `browser.preferences.useOldOrganization` means the new Preferences,
    // so this condition in fact falls into the new Preferences.
    // But we are opening in the way of the old Preferences
    openPreferences("advanced", "dataChoicesTab"); 
  } else {
    openPreferences("privacy-reports");
  }
```
In FF54, this is fine since always the old Preferences there. But actually it is the new Preferences opened in FF 55.


[1] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/browser/components/about/AboutRedirector.cpp#147
[2] https://dxr.mozilla.org/mozilla-central/search?q=browser.preferences.useOldOrganization&redirect=false&case=true
Comment on attachment 8862895 [details]
Bug 1335454 - Add the ability to open the "Data Choices" options section through UITour,

https://reviewboard.mozilla.org/r/134772/#review141218

::: browser/app/profile/firefox.js:691
(Diff revision 2)
> +// Right now the new Prferrences is turned on FF55 Nighly and
> +// will be released after FF55. See Bug 1335907 and 1343682 for more details.
> +#if defined(NIGHTLY_BUILD)
> +pref("browser.preferences.useOldOrganization", false);
> +#else
> +pref("browser.preferences.useOldOrganization", true);
> +#endif
> +

This should not be necessary. Please see my note in UITour.jsm for more details.

::: browser/components/uitour/UITour.jsm:543
(Diff revision 2)
>            return false;
>          }
>  
> -        window.openPreferences(data.pane);
> +        let paneID = data.pane;
> +        let extraArgs;
> +        if (Services.prefs.getBoolPref("browser.preferences.useOldOrganization")) {

Please change this to:
> if (Services.prefs.getBoolPref("browser.preferences.useOldOrganization", true)) {

This will return true if the preference doesn't exist, which would be the case on FF54. The ability to provide a default value was introduced in FF54 by bug 1338306.
Attachment #8862895 - Flags: review?(jaws) → review-
Comment on attachment 8862895 [details]
Bug 1335454 - Add the ability to open the "Data Choices" options section through UITour,

https://reviewboard.mozilla.org/r/134772/#review141218

> This should not be necessary. Please see my note in UITour.jsm for more details.

OK, thanks

> Please change this to:
> > if (Services.prefs.getBoolPref("browser.preferences.useOldOrganization", true)) {
> 
> This will return true if the preference doesn't exist, which would be the case on FF54. The ability to provide a default value was introduced in FF54 by bug 1338306.

Updated, thanks
Whiteboard: [photon-onboarding]
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Comment on attachment 8862895 [details]
Bug 1335454 - Add the ability to open the "Data Choices" options section through UITour,

https://reviewboard.mozilla.org/r/134772/#review145168
Attachment #8862895 - Flags: review?(jaws) → review+
(In reply to Fischer [:Fischer] from comment #31)
> Comment on attachment 8862895 [details]
> Bug 1335454 - Add the ability to open the "Data Choices" options section
> through UITour,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/134772/diff/3-4/

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64548de6b373d9f9562a7b9c7725e31bff58323
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/764076f746f1
Add the ability to open the "Data Choices" options section through UITour, r=jaws
Keywords: checkin-needed
(In reply to Phil Ringnalda (:philor) from comment #34)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/0dd38da1f706
> for hitting
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=101972325&repo=autoland on ASan (where, and probably because,
> crashreporter is disabled).
Hi Phil,
What does it mean by "probably because crashreporter is disabled"? Thanks
Flags: needinfo?(philringnalda)
It means "ASan is our only platform on which crashreporter is disabled, and the preference pane you are opening contains a crashreporter preference, so rather than starting your debugging by looking for address sanitization reasons why the test would fail, I recommend you start by looking for reasons why the test would fail in a build with the crashreporter disabled."
Flags: needinfo?(philringnalda)
(In reply to Phil Ringnalda (:philor) from comment #36)
> It means "ASan is our only platform on which crashreporter is disabled, and
> the preference pane you are opening contains a crashreporter preference, so
> rather than starting your debugging by looking for address sanitization
> reasons why the test would fail, I recommend you start by looking for
> reasons why the test would fail in a build with the crashreporter disabled."
Thanks, yeah, the failure reason is because the expected crashreporter preference wouldn't exist when crashreporter is disabled.
Keywords: checkin-needed
(In reply to Fischer [:Fischer] from comment #37)
> Comment on attachment 8862895 [details]
> Bug 1335454 - Add the ability to open the "Data Choices" options section
> through UITour,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/134772/diff/4-5/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e65f7cbd0480f919d247ea790fca89ae0185cb
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6380ec73b696
Add the ability to open the "Data Choices" options section through UITour, r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6380ec73b696
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Are there any specs or prefs to set for me to verify this issue?
Flags: needinfo?(mverdi)
(In reply to Justin [:JW_SoftvisionQA] from comment #42)
> Are there any specs or prefs to set for me to verify this issue?

There shouldn't be any prefs to set. We'll have to actually add this function to the privacy notice page in order to test it though.
Flags: needinfo?(mverdi)
I've managed to reproduce this bug on Nightly 54.0a1 (2017-01-31) (64-bit) from Linux x64!

This feature is now implemented on Latest Nightly and verified as fixed in 56.0a1 (2017-06-25) (64-bit)

Build ID: 20170625100343
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
OS: Linux 4.8.0-56-generic
QA Whiteboard: [bugday-20170621]
This issue has been verified on Win10 x64 with today's nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: