Mahmoud Al-Qudsi Reporter
	Description • 7 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 1 • 7 years ago

Tina and Bryan, what do you think about this?

	bbell
	Comment 2 • 7 years ago

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.

	Ed Lee :Mardak Assignee
	Comment 3 • 7 years ago

uiwanted: https://trello.com/c/kV4iQYE4/152-new-tab-should-have-more-robust-settings-inside-of-firefox-preferences

	Ed Lee :Mardak Assignee
	Comment 4 • 7 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 7 years ago

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.

	Ed Lee :Mardak Assignee
	Comment 6 • 7 years ago

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?

	Matthew N. [:MattN]
	Comment 7 • 7 years ago

(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

	Ed Lee :Mardak Assignee
	Comment 8 • 7 years ago

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

	Ed Lee :Mardak Assignee
	Comment 9 • 7 years ago

uiwanted: How should existing sub options work -- in particular I believe they're all checkboxes for now: highlights customization and pocket stuff.

	Ed Lee :Mardak Assignee
	Comment 10 • 7 years ago

uiwanted: Are the strings from the design final and ready for localization?

	Ed Lee :Mardak Assignee
	Comment 11 • 6 years ago

> 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

	Francesco Lodolo [:flod]
	Comment 12 • 6 years ago

(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.

	Aaron Benson
	Comment 13 • 6 years ago

If we wrap them up this week would that give us enough time?

	Francesco Lodolo [:flod]
	Comment 14 • 6 years ago

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.

	Ed Lee :Mardak Assignee
	Comment 15 • 6 years ago

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).

	Ed Lee :Mardak Assignee
	Comment 16 • 6 years ago

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…)

	Mike Kaply [:mkaply]
	Comment 17 • 6 years ago

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.

	Ed Lee :Mardak Assignee
	Comment 18 • 6 years ago

uifeedback: Updated strings https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/280785532

	Aaron Benson
	Comment 19 • 6 years ago

Strings here (for easier copying and pasting) :) 

https://docs.google.com/document/d/1uI7xtTuI9pw9rbOzN7q1kjUeIOEH3wfJtFr-4-degzQ/edit?usp=sharing

	Ed Lee :Mardak Assignee
	Comment 22 • 6 years ago

Strings landed for localization in https://github.com/mozilla/activity-stream-l10n/pull/11

	Kate Hudson :k88hudson
	Comment 23 • 6 years ago

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.

	[github robot]
	Comment 24 • 6 years ago

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

	[github robot]
	Comment 25 • 6 years ago

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

	Ed Lee :Mardak Assignee
	Comment 26 • 6 years ago

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
```

	Ed Lee :Mardak Assignee
	Comment 27 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/70fe801fb934