Andy McKay Reporter
	Description • 7 years ago

A WebExtension can change about:newtab to something else.

This is not surfaced in about:preferences and the new tab controls are on the new tab page itself. A user will no longer be able to get to them.

	Mark Striemer [:mstriemer] Assignee
	Comment 1 • 7 years ago

This should be surfaced in about:preferences next to the "When Firefox starts" option.

Mocks: https://mozilla.invisionapp.com/share/6HCITJKP8#/screens/242994342

	Mark Striemer [:mstriemer] Assignee
	Comment 3 • 7 years ago

Here's a screenshot of about:preferences with an extension controlling the home page and new tab page.

	Mark Striemer [:mstriemer] Assignee
	Comment 5 • 7 years ago

Rebased with the containers changes to fix a conflict. Here's an updated screenshot with multiple controlled prefs.

	Mark Striemer [:mstriemer] Assignee
	Comment 7 • 7 years ago

Markus, can you confirm that this placement is okay for the message? We don't have the "Show your new tab" option in startup and I'm doubtful we'll be able to land it for 57.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8908943 [details]
Bug 1373853 - Show extension that is controlling the new tab in preferences

https://reviewboard.mozilla.org/r/180556/#review186806

The screenshot of your patch shows the "Use current pages" button is enabled, but the mock up shows that button disabled.

::: browser/components/preferences/in-content/main.js:258
(Diff revision 3)
> +setEventListener("disableNewTabExtension", "command",
> +                 gMainPane.makeDisableControllingExtension("url_overrides", "newTabURL"));

The indentation looks off here.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:287
(Diff revision 3)
>  # This string is shown to notify the user that their home page is being controlled by an extension.
>  extensionControlled.homepage_override = An extension, %S, controls your home page.
>  
> +# LOCALIZATION NOTE (extensionControlled.newTabURL):
> +# This string is shown to notify the user that their new tab page is being controlled by an extension.
> +extensionControlled.newTabURL = An extension, %S, controls your new tab page.

"new tab" should be capitalized to match the string at http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/locales/en-US/chrome/browser/newTab.dtd#11

::: browser/themes/shared/incontentprefs/preferences.inc.css:150
(Diff revision 3)
>  #startupPageBox {
>    padding-top: 32px;
>  }
>  
> +#browserNewTabExtensionContent {
> +  /* Indent the the extension info to match the radio buttons. */

s/the the/the/

::: browser/themes/shared/incontentprefs/preferences.inc.css:151
(Diff revision 3)
>    padding-top: 32px;
>  }
>  
> +#browserNewTabExtensionContent {
> +  /* Indent the the extension info to match the radio buttons. */
> +  margin-inline-start: 34px;

Can you move this inside of the <radiogroup> so that we don't need to specify a separate margin-inline-start here? This will get out of date very quickly the next time we change paddings and margins, especially because this scenario likely won't get tested often.

	Mark Striemer [:mstriemer] Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8908943 [details]
Bug 1373853 - Show extension that is controlling the new tab in preferences

https://reviewboard.mozilla.org/r/180556/#review186806

I noticed that as well after posting it and fixed it already but didn't update the screenshot.

> Can you move this inside of the <radiogroup> so that we don't need to specify a separate margin-inline-start here? This will get out of date very quickly the next time we change paddings and margins, especially because this scenario likely won't get tested often.

That unfortunately doesn't indent the content here. There is an "indent" class that seems like it would work but that's for checkboxes and radio buttons are a few pixels wider :(

I just removed the indent for now, it probably isn't critical to have it indented since it's at the bottom of the list.

	Mark Striemer [:mstriemer] Assignee
	Comment 10 • 7 years ago

Here's a screenshot with the new indentation and wording.

	Sören Hentzschel
	Comment 11 • 7 years ago

I have a small design input: The current order of things in the screenshot is a bit confusing for me.

It's:

When Nightly starts
[x] Show your home page
[x] Show a blank page
[x] Show your windows and tabs from last time

<< sentence about new tab override >>

Home page

<< sentence about home page override >>

<< home page settings >>

Or to make it more clear:

- Firefox start settings (including home page)
- new tab override sentence
- home page override sentence
- home page settings

So there are start / home page settings splitted in two parts and the sentence about the new tab override is between these two parts, there is no real connection.

Does it not make sense to change the order like the following?

When Nightly starts
[x] Show your home page
[x] Show a blank page
[x] Show your windows and tabs from last time

Home page

<< sentence about home page override >>

<< home page settings >>

<< sentence about new tab override >>

or in other words:

- Firefox start settings (including home page)
- home page override sentence
- home page settings
- new tab override sentence

	Mark Striemer [:mstriemer] Assignee
	Comment 12 • 7 years ago

Markus is out so I got confirmation from Emanuela over Slack that these screenshots are good.

	Mark Striemer [:mstriemer] Assignee
	Comment 13 • 7 years ago

The placement of this is not the same as what is outlined in the mocks [1] since we do no have the "Show your New Tab page" option under Startup. Once we've implemented that this will likely move up to be beside it.

[1] https://mozilla.invisionapp.com/share/6HCITJKP8#/screens/242994344

	Pulsebot
	Comment 14 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6aa63fcf5c4e
Show extension that is controlling the new tab in preferences r=jaws

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 7 years ago

Backed out for leaking preferences windows, e.g. after browser-chrome's browser/base/content/test/general/browser_bug735471.js ran:

https://hg.mozilla.org/integration/autoland/rev/50ed4d668bf776fc022d8306fe90e2c6e6eca0b8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6aa63fcf5c4ec13f9bef8ff21d9fea7a04d5436a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132278258&repo=autoland

TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 2 window(s) until shutdown [url = about:preferences#privacy]
TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_bug1170531.js | leaked 2 window(s) until shutdown [url = about:preferences]

Please also fix the indentation here: https://hg.mozilla.org/integration/autoland/rev/6aa63fcf5c4ec13f9bef8ff21d9fea7a04d5436a#l1.62

	Mark Striemer [:mstriemer] Assignee
	Comment 18 • 7 years ago

Looks like I forgot to push my updates after review. Last try build doesn't show any related test failures.

	Pulsebot
	Comment 19 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1e37c409e4f
Show extension that is controlling the new tab in preferences r=jaws

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 7 years ago

I had to back this out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=132587502&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/47f15c732421cedfdf82b8799c89942caf57c655

	Jared Wein [:jaws] (please needinfo? me)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8908943 [details]
Bug 1373853 - Show extension that is controlling the new tab in preferences

https://reviewboard.mozilla.org/r/180556/#review187828

::: browser/components/preferences/in-content/main.js:216
(Diff revision 5)
> +    Services.obs.addObserver({
> +      observe(subject, topic, data) {
> +          handleControllingExtension("url_overrides", "newTabURL");
> +      },
> +    }, "newtab-url-changed");

You will need to remove this observer in onunload, like http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/components/preferences/in-content/search.js#43-45

This is at least part of the leak if not all. I don't see anything else that would leak, but I didn't look at the test.

	Pulsebot
	Comment 22 • 7 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/6c69f8021a5e
Show extension that is controlling the new tab in preferences r=jaws

	Pulsebot
	Comment 23 • 7 years ago

Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/dc8d998dc56b
Backed out changeset 6c69f8021a5e for accidentially landing. r=backout

	Pulsebot
	Comment 25 • 7 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42eb979f9038
Show extension that is controlling the new tab in preferences r=jaws

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/42eb979f9038

	Markus Jaritz [:designakt] (UX)
	Comment 27 • 7 years ago

(In reply to Mark Striemer [:mstriemer] from comment #13)
> The placement of this is not the same as what is outlined in the mocks [1]
> since we do no have the "Show your New Tab page" option under Startup. Once
> we've implemented that this will likely move up to be beside it.
> 
> [1] https://mozilla.invisionapp.com/share/6HCITJKP8#/screens/242994344

Is there a follow up bug for that to get fixed?

	Mark Striemer [:mstriemer] Assignee
	Comment 28 • 7 years ago

Latest debug build shows no leaked windows with the update jaws suggested.

	Mark Striemer [:mstriemer] Assignee
	Comment 29 • 7 years ago

I filed bug 1403236 to add the New Tab option and move the notice.

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 30 • 7 years ago

Is that a feature worth mentionning in 58 release notes?

	Mark Striemer [:mstriemer] Assignee
	Comment 31 • 7 years ago

There should be more related changes in 58 that may be worth mentioning together. How do they get added to the release notes?

	Pascal Chevrel:pascalc (PTO until August 21)
	Comment 32 • 7 years ago

(In reply to Mark Striemer [:mstriemer] from comment #31)
> There should be more related changes in 58 that may be worth mentioning
> together. How do they get added to the release notes?

Here are instructions:
https://wiki.mozilla.org/Release_Management/Release_Notes

Now that Aurora is gone, we start writing release notes when they land and stick on Nightly.

	CosminB
	Comment 33 • 7 years ago

This issue is verified as fixed on Firefox 58.0a1 (20171015220106) under Wind 7 64-bit and Mac OS X 10.13.

The extension that can change about:newtab is displayed in about:preferences tab in “When Nightly starts” option.

Please see the attached screenshot.