Closed Bug 1404890 Opened 2 years ago Closed 2 years ago

Move new tab settings for sections to about:preferences

Categories

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

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Iteration:
61.1 - Mar 26
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: mqudsi, Assigned: Mardak)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [strings landed])

Attachments

(3 files)

Attached image mockup.png
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170928180207

Steps to reproduce:

As a background, I'm an ex-Firefox user giving the browser another shot after many, many years. After over 10 minutes of trying to figure out how to disable pocket on the new tab page (and trying various options until I landed on browser.newtabpage.activity-stream.feeds.section.topstories), I made my way to Bugzilla to report that a user option should be made visible for this feature, but by chance happened across a comment that referenced "new tab settings" and found the gear on the new tab page.

As a developer myself, I realize that sometimes a fresh pair of eyes that have never seen the software and the UI before can catch things that a developer or a tester intimately familiar with the product can overlook, so I am suggesting that a) the new tab page settings are not easily discoverable (the problem, I think: I'm on a hi-res 4k display. The gear icon is far away from where my eyes were looking (in the center of the NTP), and it does not have enough contrast with the background to be easily noticeable).


Actual results:

I searched the browser options for new tab page settings (under menu | options) and found none.


Expected results:

Perhaps include "New Tab Page" as a section of the browser options dialog (i.e. an icon on the left) which exposes the same settings that are available via the NTP options link.

The attached screenshot is a (very quick) mockup of what this would look like.
Component: Untriaged → Preferences
Tina and Bryan, what do you think about this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(thsieh)
Flags: needinfo?(bbell)
Priority: -- → P5
We're entirely in favor of doing this. Coincidentally we're working on new mockups this week that would do this and provide more options for users who want to customize their New Tab.
Flags: needinfo?(bbell)
Flags: needinfo?(thsieh)
Component: Preferences → Activity Streams: Newtab
Blocks: 1432589
Summary: Include new tab settings in the general browser options dialog → Move new tab settings for sections to about:preferences
Depends on: 1417155
Blocks: 1432672
jaws suggested on irc to look at how form autofill has been adding preferences. Some digging shows a jsm observing "sync-pane-loaded":

https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillParent.jsm#131-137

Then dynamically add xul:

https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillPreferences.jsm

This should be relatively compatible with ActivityStream's existing structure where in particular:
- prefs are defined in a jsm
-- https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/ActivityStream.jsm#43
-- https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/ActivityStreamPrefs.jsm
- strings for prefs UI are dynamically loaded
-- e.g., https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/prerendered/locales/zh-TW/activity-stream-strings.js

Assuming we set the appropriate xul elements with attributes, e.g., searchkeywords, the existing about:preferences functionality should "just work"… ?

jaws, anything else to note?

There was some additional discussion on irc about React in iframe in about:preferences. Just summarizing that there could be some engineering benefits of having about:preferences implemented with something React-like, but having a mix of xul and iframe for an undetermined transition time will likely cause maintainability and performance regressions.
Flags: needinfo?(jaws)
That should cover it, though MattN worked closely with the form autofill implementation here so I'll forward a needinfo to him so he can chime in if necessary.
Flags: needinfo?(jaws) → needinfo?(MattN+bmo)
One thing I forgot to note is that design wants a new side section in about:preferences for bug 1417155.

E.g.,

* General  | Home
* [ Home ] | 
* Search   | Home Page
* Privacy  | [ default / custom / add-on ▼ ]
* FxA      |
           | New Tab
           | [ default / custom / add-on ▼ ]
           |
           | Customize
           | [x] Search
           |     Search the Web from your new tab.
           | [x] Top Sites
           |     Access the websites you visit most.
           | …

Dynamically adding the Home side section could probably cause jumping around, so maybe it should always be there? Potentially the "Home Page" and "New Tab" items will be statically declared, but the Customize stuff (i.e., the stuff for this bug that exists in the new tab page's pref sidebar) would be dynamic?
Blocks: 1396564
Blocks: 1417072
No longer blocks: 1396564, 1417072
(In reply to Ed Lee :Mardak from comment #4)
> jaws suggested on irc to look at how form autofill has been adding
> preferences. Some digging shows a jsm observing "sync-pane-loaded":
> 
> https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/
> FormAutofillParent.jsm#131-137

Yeah, "sync-pane-loaded" is a hack that should be officially supported with a better name to indicate when preferences are ready for injected content.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> That should cover it, though MattN worked closely with the form autofill
> implementation here so I'll forward a needinfo to him so he can chime in if
> necessary.

An important part is `waitForSystemAddonInjectionsFinished`[1]. You'll have to add your own so that the prefs code knows then you're done injecting your content in case you need to deep-link to an injected section (using the spotlight feature of bug 1407568).

[1] https://dxr.mozilla.org/mozilla-central/search?q=waitForSystemAddonInjectionsFinished
Flags: needinfo?(MattN+bmo)
Depends on: 1432664
Assignee: nobody → edilee
Severity: normal → enhancement
Iteration: --- → 60.2 - Feb 12
Priority: P3 → P1
Whiteboard: [AS60MVP]
uifeedback: https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected

Firefox Home Contents sub section with intro text and link to about:newtab (or about:home? probably doesn't matter…)

sections have checkbox and icon. description text is black as primary text

# row selection is bug 1400536
Keywords: uiwanted
Blocks: 1400536
uiwanted: How should existing sub options work -- in particular I believe they're all checkboxes for now: highlights customization and pocket stuff.
Flags: needinfo?(abenson)
Keywords: uiwanted
uiwanted: Are the strings from the design final and ready for localization?
Iteration: 60.2 - Feb 12 → 60.3 - Feb 26
Blocks: 1437659
Depends on: 1438651
> https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected
flod, it sounds like strings aren't finalized and might take a little bit to get ready. Should we land these in activity-stream strings.properties and not land in activity-stream-l10n until more finalized? Or just export and deal with potential churn?

New strings for this particular bug:

prefs_contents_header=Firefox Home Contents
prefs_contents_description=Choose what content shows on Firefox Home. Additional customization tools are found {page_link}.
prefs_contents_description_link=on the page

Although in this case, I wonder if there will be structural changes, e.g., for development, I used two separate strings:

prefs_contents_description=Choose what content shows on Firefox Home.
prefs_contents_customize_more_link=Customize more in page
Flags: needinfo?(francesco.lodolo)
Whiteboard: [AS60MVP] → [strings needed][AS60MVP]
(In reply to Ed Lee :Mardak from comment #11)
> > https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected
> flod, it sounds like strings aren't finalized and might take a little bit to
> get ready. Should we land these in activity-stream strings.properties and
> not land in activity-stream-l10n until more finalized? Or just export and
> deal with potential churn?

Do we have any clue about the timing of this discussion? I'm mostly concerned about the risk of entering the beta cycle with English in preferences for all locales, with only a couple of weeks left for 60 in Nightly, and that's far from great.
Flags: needinfo?(francesco.lodolo)
If we wrap them up this week would that give us enough time?
Flags: needinfo?(abenson) → needinfo?(francesco.lodolo)
Yes. Wrap up this week, expose strings early next week, land available translations in m-c at the end of the cycle. 

Then we can still update other languages in beta at the beginning of that cycle to expand coverage.
Flags: needinfo?(francesco.lodolo)
Okay. From the product side, this sounds like a must-have feature for 60, so we should land all the preferences strings sooner (more than the ones needed in this bug).
Blocks: 1434751
Getting strings landed earlier in this bug for:

bug 1434751: "Restore Defaults"
bug 1400536: "{num} row;{num} rows" (some fluent would be nice? ;) but probably out of scope for 60…)
Iteration: 60.3 - Feb 26 → 60.4 - Mar 12
One thing that's important to keep in mind here is how locking preferences works. We need to make sure that when prefs associated with this things are locked, the preferences UI becomes disabled.

This is important for enterprise.
Assignee: edilee → khudson
Strings landed for localization in https://github.com/mozilla/activity-stream-l10n/pull/11
Whiteboard: [strings needed][AS60MVP] → [strings landed][AS60MVP]
After discussing with design, we are going to try to add this to a separate Home panel in about:preferences. Otherwise the PR here looks pretty good to go.
Depends on: 1443337
Iteration: 60.4 - Mar 12 → 61.1 - Mar 26
Priority: P1 → P3
Whiteboard: [strings landed][AS60MVP] → [strings landed]
Priority: P3 → P1
No longer depends on: 1417155, 1443337
See Also: 14081331417155, 1443337
Assignee: khudson → edilee
No longer blocks: 1434751
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/d301c0e31298c803731efd870359c69c58033a65
feat(preferences): Add preferences to about:preferences when it loads (#4015)

Fix Bug 1404890 - Move new tab settings for sections to about:preferences

Bug 1445090 - These strings are probably unused: pocket_description, settings_pane_topstories_options_sponsored, settings_pane_highlights_body2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Blocks: 1446053
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/dc6b52e368240341c55697d95e3bb7328bbab1da
fix(preferences): Use plain DOM instead of XBL appendItem (#4042)

Followup to bug 1404890
Landing this as part of export bug 1446053:

```diff
diff --git a/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js b/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js
index 42056a439ab6..411584308c45 100644
--- a/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js
+++ b/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js
@@ -108,16 +108,17 @@ add_task(async function() {
   await searchCompletedPromise;
 
   // Checks if back to generalPane
   for (let i = 0; i < mainPrefTag.childElementCount; i++) {
     let child = mainPrefTag.children[i];
     if (child.id == "paneGeneral"
       || child.id == "startupGroup"
       || child.id == "homepageGroup"
+      || child.id == "homeContentsGroup"
       || child.id == "languagesGroup"
       || child.id == "fontsGroup"
       || child.id == "downloadsGroup"
       || child.id == "applicationsGroup"
       || child.id == "drmGroup"
       || child.id == "updateApp"
       || child.id == "browsingGroup"
       || child.id == "performanceGroup"
@@ -126,17 +127,17 @@ add_task(async function() {
       || child.id == "languageAndAppearanceCategory"
       || child.id == "filesAndApplicationsCategory"
       || child.id == "updatesCategory"
       || child.id == "performanceCategory"
       || child.id == "browsingCategory"
       || child.id == "networkProxyCategory") {
       is_element_visible(child, "Should be in general tab");
     } else if (child.id) {
-      is_element_hidden(child, "Should not be in general tab");
+      is_element_hidden(child, `Should not be in general tab: ${child.id}`);
     }
   }
 
   await BrowserTestUtils.removeTab(gBrowser.selectedTab);
 });
 
 /**
  * Test for if nothing is found
```
Blocks: 1446195
Blocks: 1446656
https://hg.mozilla.org/mozilla-central/rev/70fe801fb934
Target Milestone: --- → Firefox 61
Blocks: 1455050
See Also: → 1467771
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.