Add Restore Defaults button to Home

VERIFIED FIXED in Firefox 63

Status

()

enhancement
P3
normal
VERIFIED FIXED
Last year
9 months ago

People

(Reporter: Mardak, Assigned: andreio)

Tracking

(Blocks 1 bug, {feature})

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: [AS61MVP])

Attachments

(2 attachments)

Reporter

Description

Last year
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)"?
Reporter

Updated

Last year
Whiteboard: [AS60MVP] → [strings needed][AS60MVP]
Reporter

Comment 1

Last year
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.
Keywords: uiwanted
Summary: Add Restore Defaults section to Home → Add Restore Defaults button to Home
Reporter

Comment 2

Last year
Bug 1404890 will land the strings for this bug: Restore Defaults
Depends on: 1404890
Whiteboard: [strings needed][AS60MVP] → [AS60MVP]
Iteration: 60.4 - Mar 12 → ---
Priority: P2 → P3
Reporter

Updated

Last year
Iteration: --- → 61.1 - Mar 26
Whiteboard: [AS60MVP] → [AS61MVP]
Iteration: 61.1 - Mar 26 → ---
Reporter

Updated

Last year
Iteration: --- → 61.2 - Apr 9
Priority: P3 → P2
Reporter

Updated

Last year
Depends on: 1443337
No longer depends on: 1404890
Reporter

Comment 3

Last year
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… ?
Flags: needinfo?(khudson)
Reporter

Comment 4

Last year
Oh and from comment 1, sounds like UX wanted the button to only appear if any values are not the default.
Iteration: 61.2 - Apr 9 → 61.3 - Apr 23
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.
Flags: needinfo?(khudson)
Reporter

Comment 6

Last year
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.
Keywords: uiwanted
Reporter

Comment 7

Last year
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…)
Depends on: 1451368
Keywords: uiwanted
Iteration: 61.3 - Apr 23 → ---
Reporter

Updated

Last year
Iteration: --- → 62.1 - May 21
Priority: P2 → P3
Iteration: 62.1 - May 21 → 62.2 - Jun 4
Iteration: 62.2 - Jun 4 → 62.3 - Jun 18
Assignee: nobody → andrei.br92
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8983838 - Flags: review?(edilee)
Assignee

Comment 9

Last year
And I will make a github follow up after this gets r+.
Iteration: 62.3 - Jun 18 → 63.1 - July 9
Iteration: 63.1 - July 9 → 63.2 - July 23
Reporter

Comment 10

Last year
mozreview-review
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).
Attachment #8983838 - Flags: review?(edilee)
Comment hidden (mozreview-request)
Reporter

Comment 12

Last year
mozreview-review
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?
Attachment #8983838 - Flags: review?(edilee)
Comment hidden (mozreview-request)
Assignee

Comment 14

Last year
mozreview-review-reply
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.
Reporter

Comment 15

Last year
mozreview-review
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
Attachment #8983838 - Flags: review?(edilee) → review+

Comment 17

Last year
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705c9f7e55d4
Add Restore Defaults button to Home r=Mardak
Keywords: checkin-needed

Comment 18

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/705c9f7e55d4
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Iteration: 63.2 - July 23 → 63.3 - Aug 6

Updated

11 months ago
Iteration: 63.3 - Aug 6 → 63.2 - July 23
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?
Flags: needinfo?(andrei.br92)
Assignee

Comment 22

9 months ago
The issue is being tracked in bug 1475995.
Flags: needinfo?(andrei.br92)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.