Show that a WebExtension has change about:newtab

VERIFIED FIXED in Firefox 58

Status

defect
P3
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: andy+bugzilla, Assigned: mstriemer)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified)

Details

(Whiteboard: [triaged])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

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

Updated

2 years ago
See Also: → 1330494
Reporter

Updated

2 years ago
Priority: -- → P3
Whiteboard: [triaged]
Assignee

Comment 1

2 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
Assignee

Updated

2 years ago
Assignee: nobody → mstriemer
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Attachment #8908943 - Flags: review?(dao+bmo)
Assignee

Updated

2 years ago
Attachment #8908943 - Flags: review?(dao+bmo) → review?(jaws)
Assignee

Comment 3

2 years ago
Posted image newtab-controlled.png (obsolete) —
Here's a screenshot of about:preferences with an extension controlling the home page and new tab page.
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
Posted image more-controlled-prefs.png (obsolete) —
Rebased with the containers changes to fix a conflict. Here's an updated screenshot with multiple controlled prefs.
Attachment #8909904 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Assignee

Comment 7

2 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.
Flags: needinfo?(mjaritz)
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.
Attachment #8908943 - Flags: review?(jaws) → review+
Assignee

Comment 9

2 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.
Assignee

Comment 10

2 years ago
Here's a screenshot with the new indentation and wording.
Attachment #8909914 - Attachment is obsolete: true
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
Assignee

Comment 12

2 years ago
Markus is out so I got confirmation from Emanuela over Slack that these screenshots are good.
Flags: needinfo?(mjaritz)
Keywords: checkin-needed
Assignee

Comment 13

2 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

Comment 14

2 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
Keywords: checkin-needed
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
Flags: needinfo?(mstriemer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(mstriemer)
Keywords: checkin-needed
Assignee

Comment 18

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

Comment 19

2 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
Keywords: checkin-needed
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.

Comment 22

2 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

Comment 23

2 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
Comment hidden (mozreview-request)

Comment 25

2 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

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42eb979f9038
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(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?
Assignee

Comment 28

2 years ago
Latest debug build shows no leaked windows with the update jaws suggested.
Keywords: checkin-needed
Assignee

Comment 29

2 years ago
I filed bug 1403236 to add the New Tab option and move the notice.
Flags: needinfo?(mstriemer)
See Also: → 1403236
Is that a feature worth mentionning in 58 release notes?
Flags: needinfo?(mstriemer)
Assignee

Comment 31

2 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?
Flags: needinfo?(mstriemer)
(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.
relnote-firefox: --- → ?

Comment 33

2 years ago
Posted image newtab.png
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.

Updated

2 years ago
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.