Closed Bug 1417155 Opened 6 years ago Closed 6 years ago

Add new Home / New Tab page preference section for default / blank / custom / add-on

Categories

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

58 Branch
enhancement

Tracking

()

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

People

(Reporter: mstanke, Assigned: k88hudson)

References

(Depends on 1 open bug, Blocks 3 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [strings m-c landed])

User Story

https://mozilla.invisionapp.com/share/65FPNL9RH4V#/screens/276669529

Attachments

(3 files)

At this moment, Firefox for desktop does not have a preference for new tab page. I suggest to add an option to General preferences to use the homepage as a new tab page too. Basically the same option as is in Firefox for Android.
What would that pref do? With Activity Stream enabled about:home and about:newtab provide the same experience.

There are plans to migrate Activity Stream preferences from the gear icon on the page to the main Firefox settings page.
In Firefox for Android there is an option to change the homepage to custom URL plus an extra option to use this URL for new tabs too. In Firefox for desktop there is only the option for homepage, but none to use the custom URL for new tabs too. I think it makes sense to introduce this option especially after the about:home and about:newtab have been unified. Now if the user changes the homepage, it will actually apply to the homepage opened for new Firefox instance (about:home), but not for new tabs (about:newtab).
Severity: normal → enhancement
One way to get this functionality is to use a Web Extension that resets the about:newtab page to the URL.
Priority: -- → P3
uiwanted: More details on how the new grouped section looks / behaves
Blocks: 1432589, 1404890
Iteration: --- → 60.3 - Feb 26
Keywords: uiwanted
Priority: P3 → P2
Summary: Allow to use homepage for new tab → Add new Home / New Tab page preference section for default / blank / custom / add-on
Blocks: 1432672
Iteration: 60.3 - Feb 26 → 60.2 - Feb 12
Blocks: 1383599
Blocks: 1432306
Whiteboard: [AS60MVP]
See Also: → 1433133
More uiwanted: Should the page controls be always visible or behind radio buttons:

E.g.,

Home Page
[ default / custom / add-on ▼ ]

New Tab
[ default / custom / add-on ▼ ]


vs

Home and New Tab Pages
(o) Use default for both home and new tab
( ) Use a custom url for both home and new tab
( ) Show a blank page for both home and new tab
( ) Use a different page for each home and new tab

Where selecting the last radio option would expand to the above?
Sounds like the latest update is to keep the home page functionality but move things into drop downs:

Home page
[ Firefox Home / Blank Page / Use Current Pages / Use Bookmark… / Custom ▼ ]
[ text box shows up when Custom is selected ]

Where selecting "use current" or "use bookmark" will end up making the selection switch to Custom. Use Current Pages will fill in the text box with open tabs. Use Bookmark… will have the dialog that will cause the text box to fill in. Custom by itself will show an empty text box. I guess if the text box is left blank, it'll revert to Firefox Home. ?


Then for new tabs:

New Tab page
[ Firefox Home / Blank Page / Use Bookmark… / Custom  ▼ ]
[ text box shows up when Custom is selected ]

Similar to above but not "Use Current Pages". This means we'll need to reintroduce a pref to set the new tab page. Perhaps to satisfy bug 1383599, selecting Blank Page will set browser.newtabpage.enabled to false. ?
Blocks: 1417441
See Also: → 1400710
(In reply to Ed Lee :Mardak from comment #6)

> This means we'll need to
> reintroduce a pref to set the new tab page.

The newtab page pref was removed because it was actively abused by search hijackers (see bug 1118285). Do you have plans to avoid this re-occurring?
I remember seeing bug 1426438 go by for removing NewTabURL, but I guess I thought the original bug removing the pref was from add-on abuse. If it's a problem to have any custom new tab page as a pref, then I guess the new tab options would just be [ Firefox Home / Blank Page ▼ ]? Do webextensions allow for setting a url statically or even dynamically via some event (although this latter one would be harder to display in about:preferences)?
uifeedback: https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected

Use Home icon for a "Home" section in about:preferences (might be slightly different from the toolbar icon)

Show sub section with dropdowns for "home & new windows" and "new tabs":
home maintains existing functionality but via dropdown items that will reveal custom input box as in comment 6
new tab only has "Firefox Home (Default)" and blank.. and optionally extension if installed (like existing behavior)

not for this bug: General section should have a checkbox to "restore last session" instead of the 3 radio of home vs blank vs restore. as home can be set to blank now.

not for this bug: there's a restore defaults section
Keywords: uiwanted
Blocks: 1434751
(In reply to Ed Lee :Mardak from comment #9)
> https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected
designakt, how should the incoming dropdown changes handle webextensions? They should show up as drop down items? Can there be multiple webextensions to choose from? What happens when switching to "Firefox Home" or Blank?

Or if we don't change the behavior now for webextension, I would guess the dropdown gets disabled and shows that a webextension is controlling home and/or new tab pages?
Flags: needinfo?(mjaritz)
Attached image New_NewTabOverride.png
(In reply to Ed Lee :Mardak from comment #10)
> (In reply to Ed Lee :Mardak from comment #9)
> > https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected

> designakt, how should the incoming dropdown changes handle webextensions?
Please add the extension to the dropdown and use the current extension notice below the relevant section (if the extension is selected) - so that people can disable the extension.

> They should show up as drop down items?
Yes, please.

> Can there be multiple webextensions to choose from?
Yes... we currently let the last installed control the page... but this dropdown could expose previously installed extensions... if they are enabled.

> What happens when switching to "Firefox Home" or Blank?
The extension notice disapears. (The extension stays enabled, but is not controling the page anymore.)

> Or if we don't change the behavior now for webextension, I would guess the
> dropdown gets disabled and shows that a webextension is controlling home
> and/or new tab pages?
That might be a fallback, but seams less elegant.

Bob, is the proposed behaviour possible? Or do we have to disable the extension for the user to be able to change what page to show.
Flags: needinfo?(mjaritz) → needinfo?(bob.silverberg)
(In reply to Markus Jaritz [:designakt] (UX) from comment #11)
> Created attachment 8948938 [details]
> New_NewTabOverride.png
> 
> (In reply to Ed Lee :Mardak from comment #10)
> > (In reply to Ed Lee :Mardak from comment #9)
> > > https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected
> 
> > designakt, how should the incoming dropdown changes handle webextensions?
> Please add the extension to the dropdown and use the current extension
> notice below the relevant section (if the extension is selected) - so that
> people can disable the extension.
> 
> > They should show up as drop down items?
> Yes, please.
> 
> > Can there be multiple webextensions to choose from?
> Yes... we currently let the last installed control the page... but this
> dropdown could expose previously installed extensions... if they are enabled.
> 
> > What happens when switching to "Firefox Home" or Blank?
> The extension notice disapears. (The extension stays enabled, but is not
> controling the page anymore.)
> 
> > Or if we don't change the behavior now for webextension, I would guess the
> > dropdown gets disabled and shows that a webextension is controlling home
> > and/or new tab pages?
> That might be a fallback, but seams less elegant.
> 
> Bob, is the proposed behaviour possible? Or do we have to disable the
> extension for the user to be able to change what page to show.

It's certainly possible. We'd have to make some changes to the way precedence is handled. Right now it's all based on install date, but this would require us to give a different extension precedence regardless of its install date. That's doable, and I agree that this interface would be better, but we just have to realize that it's not an insignificant amount of work.

If we go that route, we probably don't even need the "Disable" button beside the extension controlled notice, and maybe we don't need the notice at all, if we think the presence of an extension in the drop down will be enough to draw the user's attention to the fact that an extension is controlling the setting.
Flags: needinfo?(bob.silverberg)
(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> If we go that route, we probably don't even need the "Disable" button beside
> the extension controlled notice, and maybe we don't need the notice at all,
> if we think the presence of an extension in the drop down will be enough to
> draw the user's attention to the fact that an extension is controlling the
> setting.

I've been thinking about that too. I opted for the notice to make it clear that this is an extension, and to give users a chance to disable the extension. (And with that align with the pattern we established)
Iteration: 60.2 - Feb 12 → 60.3 - Feb 26
Assignee: nobody → edilee
Iteration: 60.3 - Feb 26 → 60.4 - Mar 12
Priority: P2 → P1
Blocks: 1437659
flod, similar to your concern from bug 1404890 comment 12 of strings late in nightly, the strings for this particular bug's changes are most likely landing directly in m-c as that's where they exist. Are there timing concerns there too or there's regular mozilla-beta l10n updates given the lack of aurora localization period. (Sorry, it's been a while since we landed strings in m-c.)
Flags: needinfo?(francesco.lodolo)
Whiteboard: [AS60MVP] → [strings m-c needed][AS60MVP]
(In reply to Ed Lee :Mardak from comment #14)
> flod, similar to your concern from bug 1404890 comment 12 of strings late in
> nightly, the strings for this particular bug's changes are most likely
> landing directly in m-c as that's where they exist. Are there timing
> concerns there too or there's regular mozilla-beta l10n updates given the
> lack of aurora localization period. (Sorry, it's been a while since we
> landed strings in m-c.)

OK, I assumed they were AS strings, not just normal Firefox strings in /preferences.

The deadline to land strings for 60 is merge day (March 12), but you really want them to land before that week-end to be safe.
Flags: needinfo?(francesco.lodolo)
:Mardak - if you're aiming to land those strings in Firefox (m-c) and not inject them dynamically from AS addon, could you use Fluent? We're migrating Preferences to Fluent right now (tracking bug 1415730) - landed preferences.xul switch already, and will switch main.xul next week.

If you'll land it as .properties, we'll want to migrate them to .ftl anyway soon.
Flags: needinfo?(edilee)
Sure, I'll keep an eye on those other about:preferences fluent migration bugs / patches to see how to do it for this bug.
Flags: needinfo?(edilee)
Blocks: 1440219
See Also: → 1435912
There's updated designs for the extension stuff:
https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens

In particular each window/tab dropdown has a separator before listing all extensions. And there's 3 possible messages showing "An extension is controlling the page you see on (New Windows and Home|New Tabs|New Windows, New Tabs and Home). [Manage Extension]"

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> I agree that this interface would be better, but we just have to realize
> that it's not an insignificant amount of work.
It's desired to have these designs implemented for 60, which is just a few weeks left. What needs to be updated on the webextension management side for both controlling new tab and new windows/home?

Alternatively, if it can't be done for 60, we'll need to figure out designs for combining the existing extension behavior, which I would guess looks like:

Home
Open Home and New Windows with: [an extension]
An extension, <name>, is controlling your home page. [Disable Extension]
Open New Tabs with: [an extension]
An extension, <name>, is controlling your New Tab page. [Disable Extension]

Where the "An extension, <name>…" is existing code/ui but the dropdown needs to have a new entry. Where I guess switching away from "an extension" is equivalent to clicking [Disable extension] (for all extensions that would have controlled??)

Alternatively, maybe the dropdown gets disabled if an extension is controlling to avoid additional oddness of multiple attempting to control?
Flags: needinfo?(bob.silverberg)
Thanks for flagging me on this, Ed. I don't believe that I will be the one working on it, however, so I am going to needinfo Mike Conca who can help with figuring out who can work on this and if it's possible to get it done for 60.
Flags: needinfo?(bob.silverberg) → needinfo?(mconca)
Request received.  Talking to ddurst to figure this out.
Flags: needinfo?(mconca)
:ddurst and I talked. He needs to speak with a couple of people on his team before finalizing the path forward.  Dropping a NI here for that.
Flags: needinfo?(ddurst)
I talked with Aaron about his new design for the new Tab / Home override message for extensions. 
We noticed some nesecary adjustments to his design to ensure the behavior for extensions stays consistent:
See extension management section in: https://mozilla.invisionapp.com/share/EGFI16P8Y36

When updating the apperance of the new tab / home message, we also need to update the style of the other overrides. 
Those are: Tracking Protection, Containers, Cookies and Proxy.

As users now can change the homepage / new Tab setting through the drop-down even when an extension is in control, we need to ensure that this is translated into disabeling the extension - as is the behavior with the current message. And re-enable it when selecting it from the drop-down again.

And we had to add a 4th option for the extension message, for the case new Tab and Home are controlled by 2 different extensions.

I'll needinfo bob to ensure those changes are feasible.
Flags: needinfo?(bob.silverberg)
I'm probably not the best person to be investigating/discussing this. I think Mark would be more appropriate so I'm going to transfer my needinfo to him.
Flags: needinfo?(bob.silverberg) → needinfo?(mstriemer)
As discussed in our meeting today I'm not sure we want to disable an extension if the user changes their choice. The extension might be providing some other functionality as well such as adding a search engine, awesome bar keyword, page action, context menu action, etc.

Looking at the mocks [1] for the new Home section in Preferences, I don't see how this accomplishes what was requested in comment 0. If I have my homepage set to https://blog.mozilla.org and the New Tab is set to "Firefox Home" wouldn't I still see about:newtab on new tabs?

With regards to webextensions for New Tab and homepage in 60: it should be possible to add the dropdown and disable it when an extension is controlling the setting, similar to how the homepage section works. I can take a look adding this or providing support.

To avoid disabling this option, as Bob mentioned, this would be a decent amount of work on the webextensions side since it moves away from the last installed wins model into a user choice model. This seems like a much better solution, so I'm happy to work on fixing this in a separate bug for a later release.

[1] https://mozilla.invisionapp.com/share/EGFI16P8Y36
Flags: needinfo?(mstriemer)
tspurway, it looks like it would be a bit tricky to add experimentation to the mozilla-central code controlling about:preferences content, and if we scope this bug to just creating a "Home" sidebar/panel to contain the existing "Home page" settings, then during a experiment where we turn off activity stream about:preferences, the "Home" panel would only have the "Home page" settings, and that would probably be a bit strange. Alternatively, if we don't fix this bug for 60, i.e., leave the existing "Home page" settings and adding activity stream section prefs under that, the General panel would be a little bit longer than what it is now.

So it seems like if we do want to run an experiment in 60 beta with activity stream prefs in-page sidebar vs about:preferences, we probably don't want to fix this bug for 60?
Flags: needinfo?(tspurway)
Assignee: edilee → khudson
Flags: needinfo?(tspurway)
Depends on: 1443337
We won't get these additional fields in for 60, but we're going to try to land the new prefs section (with just the Firefox Home Content part) since Bryan and Aaron have indicated it's essential for the design.
Priority: P1 → P2
(In reply to Kate Hudson :k88hudson from comment #26)
> We won't get these additional fields in for 60, but we're going to try to
> land the new prefs section (with just the Firefox Home Content part) since
> Bryan and Aaron have indicated it's essential for the design.

How about the underlying about:config prefs? Any chance of seeing those in 60? Why browser.newtab.url was completely removed instead of being restricted to about:blank, about:newtab and about:home is beyond my understanding of these things.
Flags: needinfo?(ddurst)
Iteration: 60.4 - Mar 12 → 61.1 - Mar 26
Priority: P2 → P3
Whiteboard: [strings m-c needed][AS60MVP] → [strings m-c needed]
No longer blocks: 1383599
Depends on: 1383599
Priority: P3 → P1
No longer blocks: 1404890
See Also: → 1404890
Ok, I've updated patch with the following changes:

- Adds the #home section to about:preferences
- Adds the New Windows and Tabs section with options for changing the homepage and new tab
- Ports functionality for disabling the appropriate UI when homepage/new tab are locked via prefs or overridden by extensions

Still to do (maybe in a follow-up patch, since this is already pretty big)
- Show the web extension in the drop down as per specs rather than
User Story: (updated)
... show a message below
- Change the startup preference to be a checkbox rather than radio buttons
(In reply to Kate Hudson :k88hudson from comment #32)
> - Change the startup preference to be a checkbox rather than radio buttons
Already split to a followup ;) bug 1440219
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review233618

::: browser/locales/en-US/browser/preferences/preferences.ftl:341
(Diff revision 3)
>      .label = Settings…
>      .accesskey = e
> +
> +## Home Section
> +
> +new-windows-tabs-header = New Windows and Tabs

can you use a prefix here? We try to keep each section to use their own prefix (the top part of the file is for chrome of prefs, and has no section).

For example, starting each string with `home-` would work!
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review233616

::: browser/locales/en-US/browser/preferences/preferences.ftl:343
(Diff revision 2)
> +
> +## Home Section
> +
> +new-windows-tabs-header = New Windows and Tabs
> +
> +new-windows-tabs-description = Chose what you see when you open your homepage, new windows, and new tabs

typo: Choose

::: browser/locales/en-US/browser/preferences/preferences.ftl:347
(Diff revision 2)
> +
> +new-windows-tabs-description = Chose what you see when you open your homepage, new windows, and new tabs
> +
> +## Home Section - Home Page Customization
> +
> +homepage-mode-label = Homepage and new windows:

We removed almost all colons from prefs. Are these in line with the existing prefs?

::: browser/locales/en-US/browser/preferences/preferences.ftl:353
(Diff revision 2)
> +
> +restore-defaults =
> +    .label = Restore Defaults
> +    .accesskey = R
> +
> +homepage-mode-choice-default

Please add a note that this can be translated, but Firefox needs to be treated as a brand and kept in English.

::: browser/locales/en-US/browser/preferences/preferences.ftl:357
(Diff revision 2)
> +
> +homepage-mode-choice-default
> +    .label = Firefox Home (Default)
> +
> +homepage-mode-choice-custom
> +    .label = Custom URL(s)...

Please use … (single Unicode character), not 3 dots.

::: browser/locales/en-US/browser/preferences/preferences.ftl:362
(Diff revision 2)
> +    .label = Custom URL(s)...
> +
> +homepage-mode-choice-blank
> +    .label = Blank Page
> +
> +newtabs-mode-label = New tabs:

Same question here about having colon
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #35)
> can you use a prefix here? We try to keep each section to use their own
> prefix (the top part of the file is for chrome of prefs, and has no section).
> 
> For example, starting each string with `home-` would work!

Oh, I realize now that my request is not that trivial.

For strings you want to preserve, leave them as is to avoid invalidating them.

For new strings in the Home section, please, use `home-`. The subsection is small enough that can be part of `home-` naming scheme.

So:

`home-new-windows-tabs-header`
`home-new-windows-tabs-description`
`home-mode-label`
`home-restore-defaults`
`home-mode-choice-default`
`home-mode-choice-custom`
`home-mode-choice-blank`
`home-newtabs-mode-label`

(and preserve:)
`use-current-pages`
`choose-bookmark`

You can also use the longer `homepage-` if you prefer! :)

thnx!
Comment on attachment 8958572 [details]
Bug 1417155 - Add Home section to about:preferences for Activity Stream preferences

https://reviewboard.mozilla.org/r/227470/#review233680


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/preferences.js:55
(Diff revision 1)
>  function init_all() {
>    Preferences.forceEnableInstantApply();
>  
>    gSubDialog.init();
>    register_module("paneGeneral", gMainPane);
> +  register_module("paneHome", gHomePane);

Error: 'ghomepane' is not defined. [eslint: no-undef]
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review233970

::: browser/locales/en-US/browser/preferences/preferences.ftl:355
(Diff revisions 3 - 4)
>      .label = Restore Defaults
>      .accesskey = R
>  
> -homepage-mode-choice-default
> +# "Firefox" should be treated as a brand and kept in English, while "Home"
> +# should be localized matching the title of the #home panel above.
> +home-mode-choice-default

You need an equal sign here after the ID, even when there's no value (see home-restore-defaults for example).

Same applies to the next 2 strings.
Attachment #8958572 - Flags: review?(francesco.lodolo)
Attachment #8958572 - Flags: review?(gandalf)
Comment on attachment 8958572 [details]
Bug 1417155 - Add Home section to about:preferences for Activity Stream preferences

https://reviewboard.mozilla.org/r/227470/#review233990

lgtm!
Attachment #8958572 - Flags: review?(gandalf) → review+
Attachment #8958573 - Flags: review?(gandalf)
Attachment #8958573 - Flags: review?(francesco.lodolo)
Michelle, would you be OK with dropping the colons on the labels for the New Windows and Tabs section (i.e. the strings "Homepage and new windows" as well as "New tabs")?

flod pointed out that some work was done to remove colons from the other areas of about:preferences (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1382135 and https://bugzilla.mozilla.org/show_bug.cgi?id=1399699)
Flags: needinfo?(mheubusch)
Comment on attachment 8958572 [details]
Bug 1417155 - Add Home section to about:preferences for Activity Stream preferences

https://reviewboard.mozilla.org/r/227470/#review233992
Attachment #8958572 - Flags: review?(francesco.lodolo) → review+
Blocks: 1443337
No longer depends on: 1443337
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review233616

> We removed almost all colons from prefs. Are these in line with the existing prefs?

Ok, I talked to Michelle and she agreed they should be removed
Flags: needinfo?(mheubusch)
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review234388

A couple of minor nits, but the FTL itself looks good. Leaving the code to more suitable eyes.

::: browser/locales/en-US/browser/preferences/preferences.ftl:347
(Diff revision 8)
> +
> +home-new-windows-tabs-description = Choose what you see when you open your homepage, new windows, and new tabs
> +
> +## Home Section - Home Page Customization
> +
> +home-mode-label = Homepage and new windows

Maybe "home-homepage-mode-label" for consistency? I would also move "home-newtabs-mode-label" close to this string

::: browser/locales/en-US/browser/preferences/preferences.ftl:354
(Diff revision 8)
> +home-restore-defaults =
> +    .label = Restore Defaults
> +    .accesskey = R
> +
> +# "Firefox" should be treated as a brand and kept in English, while "Home"
> +# should be localized matching the title of the #home panel above.

Let's just say

# "Firefox" should be treated as a brand and kept in English, while "Home" and "(Default)" can be localized.

Reasons:
* Localizers are exposed to a string at a time, they have no concept of above looking at the file.
* I don't think they necessarily need to use the same words as the sidebar (different context, different space constraints).
Attachment #8958573 - Flags: review?(francesco.lodolo) → review+
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review234390

::: browser/components/preferences/in-content/home.xul:55
(Diff revision 8)
> +          <textbox id="homePageUrl"
> +                  class="uri-element check-home-page-controlled"
> +                  data-preference-related="browser.startup.homepage"
> +                  type="autocomplete"
> +                  autocompletesearch="unifiedcomplete"
> +                  placeholder="Paste a URL..." />

This one is hard-coded, and should use a single unicode character for the ellipsis
Comment on attachment 8958572 [details]
Bug 1417155 - Add Home section to about:preferences for Activity Stream preferences

https://reviewboard.mozilla.org/r/227470/#review234776

::: browser/components/preferences/in-content/preferences.xul:151
(Diff revision 2)
> +                      helpTopic="prefs-home"
> +                      data-l10n-id="category-home"
> +                      data-l10n-attrs="tooltiptext"
> +                      align="center"
> +                      hidden="true">
> +

nit, please remove this blank line
Attachment #8958572 - Flags: review?(jaws) → review+
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review234778

Some of the requests below are pertaining to code that was there before your changes but since you're moving it now would be a good time to fix them.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:168
(Diff revision 9)
> +    is(content.document.getElementById("homeMode").disabled, true,
> +      "Homepage menulist should be disabled");
> +    is(content.document.getElementById("homePageUrl").disabled, true,
> +      "Homepage custom input should be disabled");
> +    is(content.document.getElementById("useCurrentBtn").disabled, true,
> -       "\"Use Current Page\" button should be disabled");
> +        "\"Use Current Page\" button should be disabled");

These lines got an extra space added at the beginning but they shouldn't have. They should start at the same column as the 'c' in `content`.

::: browser/components/preferences/in-content/home.js:60
(Diff revision 9)
> +   * _renderCustomSettings: Hides or shows the UI for setting a custom
> +   * homepage URL
> +   * @param {bool} shouldShow Should the custom UI be shown?
> +   */
> +  _renderCustomSettings(shouldShow) {
> +    const el = document.getElementById("customSettings");

Please rename `el` here to be more specific since later in the function it can be confusing which element `el` is referring to.

::: browser/components/preferences/in-content/home.js:96
(Diff revision 9)
> +  /**
> +   * _isNotAboutPreferences: Is a given tab set to about:preferences?
> +   * @param {Element} aElement A tab
> +   * @returns {bool} Is the linkedBrowser of aElement set to about:preferences?
> +   */
> +  _isNotAboutPreferences(aElement) {

Functions (and variables) should be written in the affirmative, otherwise adding a negation to them can become confusing.

Please change this function to _isTabAboutPreferences()

Note too that the function expects a Tab, not a generic `element` so the argument name should be changed too.

::: browser/components/preferences/in-content/home.js:109
(Diff revision 9)
> +  _getTabsForHomePage() {
> +    let tabs = [];
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");
> +
> +    if (win && win.document.documentElement
> +      .getAttribute("windowtype") == "navigator:browser") {

indentation here is broken

::: browser/components/preferences/in-content/home.js:113
(Diff revision 9)
> +    if (win && win.document.documentElement
> +      .getAttribute("windowtype") == "navigator:browser") {
> +      // We should only include visible & non-pinned tabs
> +
> +      tabs = win.gBrowser.visibleTabs.slice(win.gBrowser._numPinnedTabs);
> +      tabs = tabs.filter(this._isNotAboutPreferences);

tabs = tabs.filter(tab => !this._isTabAboutPreferences(tab));

::: browser/components/preferences/in-content/home.js:136
(Diff revision 9)
> +      el.value = this.HOME_MODE_CUSTOM;
> +    }
> +  },
> +
> +  _setInputDisabledStates(isControlled) {
> +

please remove this blank line

::: browser/components/preferences/in-content/home.js:186
(Diff revision 9)
> +    switch (value) {
> +      case this.HOME_MODE_BLANK:
> +        return false;
> +      default:
> +        return true;

This can be simplified to just:
`return value != this.HOME_MODE_BLANK;`

::: browser/components/preferences/in-content/home.js:225
(Diff revision 9)
> +   * forms.
> +   */
> +  async _updateUseCurrentButton() {
> +    let useCurrent = document.getElementById("useCurrentBtn");
> +    let tabs = this._getTabsForHomePage();
> +

please remove this blank line

::: browser/components/preferences/in-content/home.js:227
(Diff revision 9)
> +  async _updateUseCurrentButton() {
> +    let useCurrent = document.getElementById("useCurrentBtn");
> +    let tabs = this._getTabsForHomePage();
> +
> +    const tabCount = tabs.length;
> +

please remove this blank line

::: browser/components/preferences/in-content/home.js:244
(Diff revision 9)
> +   * Sets the home page to the current displayed page (or frontmost tab, if the
> +   * most recent browser window contains multiple tabs), updating preference
> +   * window UI to reflect this.

This comment seems a bit wrong, since as I read it it sounds like it will set a singular home page, but it will actually set the home page to the collection of non-about:preferences pages that are opened.

::: browser/components/preferences/in-content/main.js
(Diff revision 9)
> -   * browser.startup.page
> -   * - what page(s) to show when the user starts the application, as an integer:
> -   *
> -   *     0: a blank page
> -   *     1: the home page (as set by the browser.startup.homepage pref)
> -   *     2: the last page the user visited (DEPRECATED)
> -   *     3: windows and tabs from the last session (a.k.a. session restore)
> -   *
> -   *   The deprecated option is not exposed in UI; however, if the user has it
> -   *   selected and doesn't change the UI for this preference, the deprecated
> -   *   option is preserved.

We should keep this documentation in tree. It's getting removed here but I don't see it being added anywhere.
Attachment #8958573 - Flags: review?(jaws) → review+
Comment on attachment 8958573 [details]
Bug 1417155 - Add new Home/New Tab page section to about:prefs#home

https://reviewboard.mozilla.org/r/227472/#review234806
Attachment #8958573 - Flags: review?(gandalf) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a8ce37674daafc23dbe9d9e4203039c5feee48b8 -d 87a99640fb81: rebasing 452944:a8ce37674daa "Bug 1417155 - Add Home section to about:preferences for Activity Stream preferences r=flod,gandalf,jaws"
merging browser/components/preferences/in-content/preferences.js
merging browser/components/preferences/in-content/preferences.xul
merging browser/locales/en-US/browser/preferences/preferences.ftl
rebasing 452945:0ab7c8c40b23 "Bug 1417155 - Add new Home/New Tab page section to about:prefs#home r=flod,gandalf,jaws" (tip)
merging browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js
merging browser/components/preferences/in-content/main.js
merging browser/components/preferences/in-content/main.xul
merging browser/components/preferences/in-content/preferences.xul
merging browser/components/preferences/in-content/tests/browser_extension_controlled.js
merging browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js
merging browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_1.js
merging browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js
merging browser/locales/en-US/browser/preferences/preferences.ftl
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_extension_controlled.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_homepages_filter_aboutpreferences.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/locales/en-US/browser/preferences/preferences.ftl! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by khudson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7852878194fc
Add Home section to about:preferences for Activity Stream preferences r=flod,gandalf,jaws
https://hg.mozilla.org/integration/autoland/rev/810204c29a4d
Add new Home/New Tab page section to about:prefs#home r=flod,gandalf,jaws
https://hg.mozilla.org/mozilla-central/rev/7852878194fc
https://hg.mozilla.org/mozilla-central/rev/810204c29a4d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Whiteboard: [strings m-c needed] → [strings m-c landed]
Blocks: 1447527
Kate, this is a UI change for the end-user that should be added to release notes, could you nominate it for addition please? (in the Firefox tracking flags, set relnote-firefox to ?, click on the "comment" link and fill in the information).

Thanks
Depends on: 1447657
Depends on: 1447676
Depends on: 1447879
Depends on: 1447893
Depends on: 1447972
I forgot to add the NI in comment #65
Flags: needinfo?(khudson)
Blocks: 1448336
Release Note Request (optional, but appreciated)

[Why is this notable]:
Adds a new about:preferences section

[Affects Firefox for Android]:
Firefox

[Suggested wording]:
The settings for customizing your homepage and new tab page in Firefox have been moved to a new Preferences section, Home. It can be accessed from the sidebar in Preferences or by visiting about:preferences#home.

[Links (documentation, blog post, etc)]:
None
relnote-firefox: --- → ?
Flags: needinfo?(khudson)
Depends on: 1449207
See Also: → 1452144
Depends on: 1454344
(In reply to Kate Hudson :k88hudson from comment #67)
> Release Note Request (optional, but appreciated)
> 
> [Why is this notable]:
> Adds a new about:preferences section
> 
> [Affects Firefox for Android]:
> Firefox
> 
> [Suggested wording]:
> The settings for customizing your homepage and new tab page in Firefox have
> been moved to a new Preferences section, Home. It can be accessed from the
> sidebar in Preferences or by visiting about:preferences#home.
> 
> [Links (documentation, blog post, etc)]:
> None

Thank you, this was added to Nightly release notes for 61. For beta and final release notes, our release managers will decide on the potential inclusion and final wording, hence keeping the flag as 'relnote ?'.
Depends on: 1460423
See Also: → 1467771
Depends on: 1188732
Depends on: 1497443
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.