1434751 - Add Restore Defaults button to Home

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P3)

Description

[Ed Lee :Mardak]

https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected

Restore button that sets new window and tabs and section configs back to defaults.

uiwanted: What's the "with the default sections turned on" link? Should the button be disabled if everything is already default?

Should "Firefox Home Contents" section be hidden or just disabled when both Show dropdowns are set to something not "Firefox Home (Default)"?

Comment 1

[Ed Lee :Mardak]

uifeedback: The design was updated to conditionally show the button at the end of the "Home" first line if the dropdowns or contents are not default.

https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/277752159

Clicking the button will reset everything on the "Home" pane.

Comment 2

[Ed Lee :Mardak]

Bug 1404890 will land the strings for this bug: Restore Defaults

Comment 3

[Ed Lee :Mardak]

Bug 1417155 migrated the existing "Restore to Default" button for the Home page section to the Home panel:
https://hg.mozilla.org/mozilla-central/rev/810204c29a4d

k88hudson, I believe UX intended the button to restore everything on the page including new window, new tabs, and Home contents. Are there any issues of reusing this same button to also reset the contents section?

I suppose one approach might be to iterate through Preferences.getAll() and find pref names that start with browser.newtabpage.activity-stream… ?

Comment 4

[Ed Lee :Mardak]

Oh and from comment 1, sounds like UX wanted the button to only appear if any values are not the default.

Comment 5

[Kate Hudson :k88hudson]

It seems a bit weird to me that the Restore Defaults button next to the header for "New Windows and Tabs" refers to the "Firefox Home Content" section as well, but I guess that's what's intended.

Sounds like we'll need to implement both the scope of the restore as well as the hiding.

Comment 6

[Ed Lee :Mardak]

uiwanted: Confirming that the "Restore Defaults" button next to the "New Windows and Tabs" section causes preferences outside of that section to be restored too.

Comment 7

[Ed Lee :Mardak]

jaws is fixing some alignment issues with the current button position in bug 1451368. But we just got uifeedback:

Move the button up one line to the "Home" heading instead of the current location to the right of "New Windows and Tabs" and make it conditional to only appear if any pref on the whole page has been changed. Clicking the button would restore all the prefs on the page to the default.

I think jaws' fix could just land, the fix here should make sure that the align/center doesn't regress when moving the button. (Hopefully there's no UI jumping around issues when the button appears and disappears…)

Comment 8

[Andrei Oprea [:andreio]]

Attachment: Bug 1434751 - Add Restore Defaults button to Home

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

Comment 9

[Andrei Oprea [:andreio]]

And I will make a github follow up after this gets r+.

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

https://reviewboard.mozilla.org/r/249652/#review261178

Main concern is the AboutPreferences.jsm being deeply tangled with home.js/xul details.

::: browser/components/preferences/in-content/home.js:83
(Diff revision 1)
> +   * hide the Restore Defaults button.
> +   */
> +  watchHomeTabPrefChange() {
> +    let observer = {
> +      observe: this._onHomeTabPrefChange.bind(this)
> +    };

You don't need an object to wrap a single observe function. And are we going to get anything other than nsPref:changed? Pretty sure this should work…

const observer = () => this.toggleRestoreDefaultsBtn();

::: browser/components/preferences/in-content/home.js:414
(Diff revision 1)
> +  /**
> +   * Set all prefs on the Home tab back to their default values.
> +   */
> +  restoreDefaultPrefsForHome() {
> +    this.restoreDefaultHomePage();
> +    Preferences.getAll().filter(pref => pref.id.includes("activity-stream"))

This duplicated getting of activity stream prefs array could probably be shared as a getter on this object.

::: browser/components/preferences/in-content/home.js:417
(Diff revision 1)
> +  restoreDefaultPrefsForHome() {
> +    this.restoreDefaultHomePage();
> +    Preferences.getAll().filter(pref => pref.id.includes("activity-stream"))
> +      .forEach(pref => pref.value = pref.defaultValue);
> +
> +    document.getElementById("restoreDefaultHomePageBtn").style.visibility = "hidden";

The pref observers are going to trigger anyway, so this extra style setting shouldn't be needed?

::: browser/components/preferences/in-content/home.xul:22
(Diff revision 1)
> -    <caption flex="1" align="center"><label data-l10n-id="home-new-windows-tabs-header"/></caption>
>      <button id="restoreDefaultHomePageBtn"
> -            class="homepage-button check-home-page-controlled"
> +      class="homepage-button check-home-page-controlled"
> -            data-preference-related="browser.startup.homepage"
> +      data-preference-related="browser.startup.homepage"
> -            data-l10n-id="home-restore-defaults"
> +      data-l10n-id="home-restore-defaults"
> -            preference="pref.browser.homepage.disable_button.restore_default"/>
> +      preference="pref.browser.homepage.disable_button.restore_default"/>

Do these preference/data-preference-related attributes need to be updated?

::: browser/extensions/activity-stream/lib/AboutPreferences.jsm:167
(Diff revision 1)
>  
> +    const homePref = Preferences.get(STARTUP_HOMEPAGE_PREF);
> +    const newtabPref = Preferences.get(NEWTAB_ENABLED_PREF);
> +    // If homepage and newtab have default values hide the Restore Defaults btn
> +    if ((homePref.value === homePref.defaultValue) && !newtabPref.hasUserValue) {
> +      restoreDefaultPrefsBtn.style.visibility = "hidden";

This reaching into other prefs on the Home pane and adjusting the button doesn't seem quite right. That functionality should probably stay in home.js.

Why not just call `gHomePane.toggleRestoreDefaultsBtn()` after it's done rendering preferences? `gHomePane` will need to be a global `var` on `window` (not a locally `let` scoped variable).

Comment 11

[Andrei Oprea [:andreio]]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

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

Comment 12

[Ed Lee :Mardak]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

https://reviewboard.mozilla.org/r/249652/#review261510

Probably would be good to dig in to the expected usage / behavior of data-preference-related. Also, the issues in mozreview should be marked fixed or commented as appropriate.

::: browser/components/preferences/in-content/home.js:43
(Diff revision 2)
>    HOME_MODE_CUSTOM: "2",
>    NEWTAB_ENABLED_PREF: "browser.newtabpage.enabled",
> +  ACTIVITY_STREAM_PREF_BRANCH: "browser.newtabpage.activity-stream.",
> +
> +  get homePanePrefs() {
> +    return Preferences.getAll().filter(pref => pref.id.includes("activity-stream"));

Do all the prefs we care about actually start with ACTIVITY_STREAM_PREF_BRANCH? Probably safer to check that instead of anywhere "activity-stream" just in case sync prefs or some other pref matches, e.g., browser.library.activity-stream.enabled

::: browser/components/preferences/in-content/home.xul
(Diff revision 2)
>            hidden="true">
>    <hbox>
> -    <caption flex="1" align="center"><label data-l10n-id="home-new-windows-tabs-header"/></caption>
> +    <caption><label data-l10n-id="home-new-windows-tabs-header"/></caption>
> -    <button id="restoreDefaultHomePageBtn"
> -            class="homepage-button check-home-page-controlled"
> -            data-preference-related="browser.startup.homepage"

I see this is removed now, but I'm still unclear what's the expected value for this attribute. A quick glance seems to show it's used in `_setInputDisabledStates`, so are we breaking any functionality by removing it?

::: browser/components/preferences/in-content/tests/browser_hometab_restore_defaults.js:7
(Diff revision 2)
> +  const before = SpecialPowers.Services.prefs.getStringPref("browser.newtabpage.activity-stream.feeds.section.topstories.options", "");
> +
> +  await SpecialPowers.pushPrefEnv({set: [
> +    // Hide Pocket pref so we don't trigger network requests when we reset all preferences
> +    ["browser.newtabpage.activity-stream.feeds.section.topstories.options", JSON.stringify(Object.assign({}, JSON.parse(before), {hidden: true}))],
> +    // Set a user pref to false to force the Restore Defaults button to be visible

Could you also add a test where Restore Defaults button is hidden initially?

::: browser/extensions/activity-stream/lib/AboutPreferences.jsm:257
(Diff revision 2)
>          subcheck.setAttribute("label", formatString(nested.titleString));
>          linkPref(subcheck, nested.name, "bool");
>        });
>      });
> +
> +    gHomePane.toggleRestoreDefaultsBtn();

A comment for why this is needed would be good. Also, I'm not sure if the default state of the button should be hidden or not. I suppose in the common case where users don't change anything, the button would usually be hidden?

Comment 13

[Andrei Oprea [:andreio]]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

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

Comment 14

[Andrei Oprea [:andreio]]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

https://reviewboard.mozilla.org/r/249652/#review261510

> I see this is removed now, but I'm still unclear what's the expected value for this attribute. A quick glance seems to show it's used in `_setInputDisabledStates`, so are we breaking any functionality by removing it?

Initially I was confused about this but it looks like it's used to disable elements based on Policy rules [0].
`data-preference-related` is used to reference the pref that the button controls and if the pref is setAndLocked then the button is locked. The `preference` attribute is a direct way to disable the button.
So we shouldn't keep them. I also don't think this patch needs to update/change this code, we are resetting more options but that does not influence the expected behaviour of disabling. And the Firefox Home Content preferences are hidden anyway when these policies are enabled.

[0] https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/browser/components/enterprisepolicies/Policies.jsm#514-542
[1] https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/browser/components/preferences/in-content/home.js#84

> A comment for why this is needed would be good. Also, I'm not sure if the default state of the button should be hidden or not. I suppose in the common case where users don't change anything, the button would usually be hidden?

Hidden otherwise you would have a button that aparrently doesn't do anything when you click it.

Comment 15

[Ed Lee :Mardak]

Comment on attachment 8983838 [details]
Bug 1434751 - Add Restore Defaults button to Home

https://reviewboard.mozilla.org/r/249652/#review261668

Thanks. The tests will help make sure someone doesn't accidentally break the API between home.js and AboutPreferences.jsm

Comment 16

[Andrei Oprea [:andreio]]

Try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14129c279f25a5eb5a8b4a951dd59bc21cb7fc1&selectedJob=186470238

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/705c9f7e55d4
Add Restore Defaults button to Home r=Mardak

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/705c9f7e55d4

Comment 19

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4230

Comment 20

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/a7d1a2da528b33392b12587f2aae908a6f488ee6
chore(mc): Port Bug 1434751 - Set Restore Defaults btn visibility (#4230)

Comment 21

[Cristian Fogel [:cfogel]]

I've noticed a small difference between the mock-up/disucssions and current implementation. 
The scenario and my question would still be valid from the user point of view.

For the homepage and the windows dropdown; choosing the [Custom URLs...] option only, does not trigger the display of the [Restore Defaults] button. The button is displayed after something is written and saved in the custom url text-field.
Even tho there are no changes affecting the Activity Stream page, another non-default option is chosen in this page.

To summarize, should the button appear when a change is made in the page or the AS_page+behaviour is affected?
Is this expected?

Comment 22

[Andrei Oprea [:andreio]]

The issue is being tracked in bug 1475995.

Comment 23

[Cristian Fogel [:cfogel]]

Awesome, thanks again.
Verified the enchancement on Ubuntu 16.04 LTS, win10x64, macOS 10.9 with Firefox 63.0b12. 

The above mentioned issue will be verified with the fix  in bug 1475995.