1390158 - Notify the user on the new tab page when an extension has updated it

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Mark Striemer [:mstriemer]]

When an a user views a new tab page for an extension for the first time the should be notified that this page is managed by an extension and be shown options to keep the changes or revert to the stock new tab page.

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

Comment 1

[YF (Yang)]

I prefer it to be a notification bar instead of hamburger popup, which allows the user to experience it first without covering.

Comment 2

[Mark Striemer [:mstriemer]]

I'm not sure notification bars are going to be used much in the future but that is mostly an assumption.

I believe this doorhanger can be ignored and the user will be shown it again the next time they start Firefox if it hasn't been acknowledged. I'm not sure what the idea was behind hanging it from the hamburger but it could be attempting to direct users to the Add-ons menu item.

That being said, having it hang from the "Extension (New Tab by Yahoo)" in the mocks part and allow the user to open it again later makes more sense to me. Markus is on PTO this week so we'll likely need to move forward with the hamburger menu version for now.

Comment 3

[Markus Jaritz [:markus] (UX)]

(In reply to Mark Striemer [:mstriemer] from comment #2)
> I'm not sure notification bars are going to be used much in the future but
> that is mostly an assumption.

Notification bars are much less noticeable, that is why I use a doorhanger.

> I believe this doorhanger can be ignored and the user will be shown it again
> the next time they start Firefox if it hasn't been acknowledged.

It would be great to have it hang from the extensions icon in the toolbar, 
if the extension has one there. If not, 

> I'm not sure what the idea was behind hanging it from the hamburger but it 
> could be attempting to direct users to the Add-ons menu item.

That and making it clear it is a message from Firefox, protecting your choice.

> That being said, having it hang from the "Extension (New Tab by Yahoo)" in
> the mocks part and allow the user to open it again later makes more sense to
> me.

I am hoping to get to a pattern of every message that we share about an extension, 
after install, to hang from their extension icon (or Firefox Menu as a fallback).
(instead of a place in the url bar. As not every situation has a related url)

(From Mark on Slack)
> So for the new tab doorhanger, when the user clicks *Restore Settings* the tab 
> will close since the extension is no longer installed. 
> Do we want to open a new tab to replace that tab?

I would like to have the current tab re-load with the new content.

> This may be Activity Stream but theoretically it could be a different extension’s 
> new tab page. Do we show the doorhanger again if that’s the case?

If a person has      confirmed that extension taking over new tab, no doorhanger.
If a person, has NOT confirmed that extension taking over new tab, show doorhanger.
(would happen if multiple new tab extensions get installed at the same time.)

> There’s also a *Learn more* link in the mocks. Where does that go?

The "learn more" should link to a sumo page explaining why extensions can take over 
your new tab, and how you can get back to the default newTab.

> If I have accepted that I want the Yahoo! extension to control my New Tab, 
> then I install the Bing extension that controls the New Tab I’ll get a doorhanger 
> for Bing. What happens if I then remove Bing and it reverts to Yahoo!? 
> Do I see the doorhanger again since the New Tab has changed? 
> (and if I do, what does “Revert Settings” mean?)

No, you don't, as you already accepted Yahoo! as your new Tab before.

Comment 4

[Markus Jaritz [:markus] (UX)]

(From Mark on Slack)
> If the user navigates away do we hide the doorhanger?

Yes. If they open another new tab, or just do any other action outside the doorhanger, it would disappear.

> So if they open a new tab immediate type and hit enter, they’ll end up on their search page. Should the doorhanger be hidden?

I expected they wouldn't be able to just type...
- as we would show the url of the newtab replacement, until they confirm
- and because the focus would be on the doorhanger, not the url bar (but hitting enter shouldn't confirm/dismiss the doorhanger)

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: Bug 1390158 - Notify user of extension controlling New Tab on first access

Review commit: https://reviewboard.mozilla.org/r/195958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195958/

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195956/#review201182

This should be ready for a full review but there are a couple things that I'm not certain on that could require changes. I added some comments below.

::: browser/components/customizableui/content/panelUI.inc.xul:463
(Diff revision 1)
>      <popupnotificationcontent id="update-restart-notification-content" orient="vertical">
>        <description id="update-restart-description">&updateRestart.message2;</description>
>      </popupnotificationcontent>
>    </popupnotification>
> +
> +  <popupnotification id="appMenu-extension-new-tab-notification"

@jaws I want this panel to get hidden when the user focuses the page or navigates away from the tab.

I think I'd want `<panel noautofocus="false">` for the first part, but that's set by the main appMenu notification panel.

I'm guessing for hiding on swithing tabs I'd observe some event.

Do you have any ideas of how to best accomplish this?

::: browser/components/extensions/ext-url-overrides.js:36
(Diff revision 1)
> +
> +async function handleNewTabNotificationCommand(action, extensionId) {
> +  let extension = await AddonManager.getAddonByID(extensionId);
> +  switch (action) {
> +    case "keep":
> +      await ExtensionSettingsStore.addSetting(

@aswan: I'm using `ExtensionSettingsStore` to store if a user has confirmed this change. Is that reasonable or should I store this somewhere else?

Comment 7

[Mark Striemer [:mstriemer]]

Attachment: new-tab-doorhanger.png

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Do you have an updated link to the mocks? The link in comment #0 gives me a 404.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Striemer [:mstriemer] from comment #6)
> @jaws I want this panel to get hidden when the user focuses the page or
> navigates away from the tab.
> 
> I think I'd want `<panel noautofocus="false">` for the first part, but
> that's set by the main appMenu notification panel.

noautofocus="false" is the default value. Is this not working for you?

> I'm guessing for hiding on swithing tabs I'd observe some event.

You can set the `tabspecific="true"` attribute on the panel to hide on tab switching. See http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/browser/base/content/browser.js#1542

Comment 10

[Mark Striemer [:mstriemer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> (In reply to Mark Striemer [:mstriemer] from comment #6)
> > @jaws I want this panel to get hidden when the user focuses the page or
> > navigates away from the tab.
> > 
> > I think I'd want `<panel noautofocus="false">` for the first part, but
> > that's set by the main appMenu notification panel.
> 
> noautofocus="false" is the default value. Is this not working for you?

I originally had a <panel> for this on its own but it seems like it makes sense to use AppMenuNotifications to handle it. That has its own <panel> that has noautofocus="true" on it though [0].

I guess the easiest solution might be to go back to creating my own <panel> and setting these attributes. Do you think it would be worth adding support for tabspecific="true" and noautofocus="false" to AppMenuNotifications?

Here's the new link to the mocks [1]. I don't have the page to link to yet in the "Learn more" link so I moved that to bug 1414029.

[0] http://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#406
[1] https://mozilla.invisionapp.com/share/6HCITJKP8#/screens/244736434

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

I think we need to go back to UX and discuss how some of our notifications require interaction (Firefox update needs restart) and this one wouldn't (dismiss on change of focus). They both will anchor at the same point and they both will look fairly similar to the untrained eye who doesn't read it.

We should be consistent here.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review201544

Please request review with the updated patch after we hear from UX.

Comment 13

[Markus Jaritz [:markus] (UX)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> I think we need to go back to UX and discuss how some of our notifications
> require interaction (Firefox update needs restart) and this one wouldn't
> (dismiss on change of focus). They both will anchor at the same point and
> they both will look fairly similar to the untrained eye who doesn't read it.
> 
> We should be consistent here.

We are consistent in the level of attention each dialog requires at the moment we show it.
When we show the update message, we know we really need the user to restart now, 
with this message, we need to make the user aware now, but it is OK to not make a decision now.
(If they do not make a decision now, the message should be re-surfaced after a restart.)

To more closely relate this message to the relevant extension, we should
anchor the message to the browser_action of the extension, if available.
If no browser_action is available, Firefox menu is a good fit, as it contain the access to add-on.

Comment 14

[Markus Jaritz [:markus] (UX)]

(In reply to Mark Striemer [:mstriemer] from comment #7)
> Created attachment 8924734 [details]
> new-tab-doorhanger.png

Mark, please do use 2 grey buttons, instead of making one blue. 
For this dialog we have no recommended default that we can suggest people to click.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review201970

I think that using the ExtensionSettingStore makes sense here but can you just make the value for the "newTabURL" setting into an object with "url" and "confirmed" properties instead of adding a new setting?  This will take some care since you want this to all work when the new code runs for the first time in a profile that was created with a version before this patch (ie where the value for newTabURL is a string) but I think it will simplify some other parts of the code.

::: browser/components/extensions/ext-url-overrides.js:54
(Diff revision 1)
> +    // TODO: Hide the panel on navigation, tab switch, clicks.
> +    // TODO: Steal focus from the address bar.

Are you intending to land this without these handled?  If so please file follow-up bugs and reference them here.

Comment 16

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/1-2/

Comment 17

[Mark Striemer [:mstriemer]]

Attachment: new-tab-doorhanger.png

Comment 18

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/2-3/

Comment 19

[Mark Striemer [:mstriemer]]

Attachment: new-tab-doorhanger-browser-action.png

Comment 20

[Mark Striemer [:mstriemer]]

This should match what Markus described now. It will prefer a browserAction if it is on the toolbar, then it will fall back to the menu icon. It gets dismissed when you click away or change tabs, and the address bar is not focused.

I'll address Andrew's comment about ExtensionSettingsStore tomorrow.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Should there be a visual separator between the two action buttons on the popup?

Also, it should be easy to support hiding the panel on tab switch. I don't think that needs to wait for a follow-up.

Comment 22

[Mark Striemer [:mstriemer]]

I was thinking the same thing. I'll look into it.

I moved this into its own panel and it does close on tab switch now. Since it could hang from the app menu or a browser action it didn't make sense to use AppMenuNotifications anymore.

Comment 23

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/3-4/

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203398

r- because I still have questions about the re-showing of the notification as well as improving the wording of the popup. The other issues should be easy to resolve and are pretty straightforward.

::: browser/components/customizableui/content/panelUI.inc.xul:787
(Diff revision 4)
> +                     secondarybuttonlabel="&newTabControlled.restoreButton.label;"
> +                     secondarybuttonaccesskey="&newTabControlled.restoreButton.accesskey;"
> +                     closebuttonhidden="true"
> +                     dropmarkerhidden="true"
> +                     checkboxhidden="true"
> +                     hidden="true">

We shouldn't need hidden=true here since we have it on the parent panel and we set hidden=false on this and the panel at the same time.

::: browser/components/extensions/ext-url-overrides.js:47
(Diff revision 4)
> +  observe(subject, topic, data) {
> +    let item = ExtensionSettingsStore.getSetting(STORE_TYPE, NEW_TAB_SETTING_NAME);

The "browser-open-newtab-start" notification is used for "modular peformance tracking starting as close as is reasonably possible". To make sure we don't interfere with the performance tracking, please place all of this code in a requestIdleCallback.

::: browser/components/extensions/ext-url-overrides.js:65
(Diff revision 4)
> +    // Setup the command handler.
> +    let handleCommand = async (event) => {
> +      await handleNewTabNotificationCommand(
> +        event.originalTarget.getAttribute("anonid") == "button" ? "keep" : "restore",
> +        item.id);
> +      content.hidden = true;

This shouldn't be necessary since the popup is hiding.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:21
(Diff revision 4)
> +  return new Promise(resolve => {
> +    let interval = setInterval(() => {
> +      if (initialValue != getNotificationSetting(extensionId)) {
> +        clearInterval(interval);
> +        resolve();
> +      }
> +    }, 10);
> +  });

Please remove the setInterval and use TestUtils.waitForCondition in its place (with default interval value of 100).

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:148
(Diff revision 4)
> +  await BrowserTestUtils.removeTab(gBrowser.selectedTab);
> +  let newTabOpened = waitForNewTab();
> +  BrowserOpenTab();
> +  await newTabOpened;
> +
> +  // Verify the doorhanger is not shown a second time.

So if a user quickly hits "Ctrl+T" (or Cmd+T on Mac) to open a new tab and then starts typing a search or address, the popup will hide and not return.

The user may only see a brief flash of the popup if they type fast upon opening a new tab and there isn't a way to get the notification back to see what it was.

Can we show this on all new tab pages until the user has responded to it?

::: browser/locales/en-US/chrome/browser/browser.dtd:968
(Diff revision 4)
>  <!ENTITY updateRestart.acceptButton.accesskey "R">
>  <!ENTITY updateRestart.cancelButton.label "Not Now">
>  <!ENTITY updateRestart.cancelButton.accesskey "N">
>  <!ENTITY updateRestart.panelUI.label2 "Restart to update &brandShorterName;">
>  
> +<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">

We should include the extension name here. If the user decides to not take action right away, it would be good for them to know what to disable later.

::: browser/locales/en-US/chrome/browser/browser.dtd:972
(Diff revision 4)
> +<!ENTITY newTabControlled.restoreButton.label "Restore Settings">
> +<!ENTITY newTabControlled.restoreButton.accesskey "R">

I understand what this is supposed to do (potentially revert to about:newtab or potentially revert to a different add-on) but this wording seems VERY confusing to users.

We are letting the ambiguity of what will happen surface to the user. But we should be able to figure out what will happen at the point of creating this notification.

Can we change this to three buttons (most users would only see two)?

[Keep changes][Disable $extensionName and revert to $extensionName2][Disable $extensionName]

The middle option would only be displayed if we know about a follow-up extension that would be controlling the new tab page if the current one gets disabled.

If we really can't do three buttons here, then I would prefer that we change the wording of the "Restore Settings" button to [Disable $extensionName and revert to $extensionName2]

::: browser/themes/shared/customizableui/panelUI.inc.css:460
(Diff revision 4)
> +/* START notification popups for extension controlled content */
> +#extension-notification-panel > .panel-arrowcontainer > .panel-arrowcontent {
> +  padding: 0;
> +}
> +
> +#extension-new-tab-notification .popup-notification-body {

Please use the child selector here.

::: browser/themes/shared/customizableui/panelUI.inc.css:464
(Diff revision 4)
> +
> +#extension-new-tab-notification .popup-notification-body {
> +  width: 30em;
> +}
> +
> +#extension-new-tab-notification .popup-notification-description {

Ditto.

::: browser/themes/shared/customizableui/panelUI.inc.css:469
(Diff revision 4)
> +#extension-new-tab-notification .popup-notification-description {
> +  font-size: 1.3em;
> +  font-weight: lighter;
> +}
> +
> +#extension-new-tab-notification .notification-panel-content {

Ditto.

::: browser/themes/shared/customizableui/panelUI.inc.css:473
(Diff revision 4)
> +#extension-new-tab-notification .popup-notification-warning,
> +#extension-new-tab-notification .popup-notification-icon {

Ditto.

Comment 25

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203490

Markus, it looks like we've got some UX concerns here. Do you mind commenting on them tomorrow? We're nearly at the 58 merge day.

Comment 26

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/4-5/

Comment 27

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203398

> So if a user quickly hits "Ctrl+T" (or Cmd+T on Mac) to open a new tab and then starts typing a search or address, the popup will hide and not return.
> 
> The user may only see a brief flash of the popup if they type fast upon opening a new tab and there isn't a way to get the notification back to see what it was.
> 
> Can we show this on all new tab pages until the user has responded to it?

My understanding was that we wanted the user to notice the doorhanger by clearing focus on the URL bar when we show the doorhanger. This should prevent them from doing a quick search and not seeing the doorhanger.

This would only happen once per session provided they aren't adding/removing multiple New Tab extensions in one session.

I think either approach would work fine, but I'd rather confirm with Markus. I can see annoyances with both.

1. Clear focus on the URL bar and only show it once.
2. Allow the user to type and miss the initial doorhanger but show it on each New Tab.

> We should include the extension name here. If the user decides to not take action right away, it would be good for them to know what to disable later.

We discussed this in our project meeting this morning. Suggested as a follow up but I should have time to sneak it in before we hear back from Markus.

> I understand what this is supposed to do (potentially revert to about:newtab or potentially revert to a different add-on) but this wording seems VERY confusing to users.
> 
> We are letting the ambiguity of what will happen surface to the user. But we should be able to figure out what will happen at the point of creating this notification.
> 
> Can we change this to three buttons (most users would only see two)?
> 
> [Keep changes][Disable $extensionName and revert to $extensionName2][Disable $extensionName]
> 
> The middle option would only be displayed if we know about a follow-up extension that would be controlling the new tab page if the current one gets disabled.
> 
> If we really can't do three buttons here, then I would prefer that we change the wording of the "Restore Settings" button to [Disable $extensionName and revert to $extensionName2]

The goal was to have the current tab get replaced with the new New Tab page once the user clicks "Restore Settings". We use "Disable Extension" elsewhere which might be more clear, but including the extension name is probably pretty long here.

Maybe we can include another message in the body?

```
An extension, Tabby Cat, has changed the page you see when you open a
New Tab. You can restore your settings if you do not want
this change.

Once restored, your New Tab will be set by Cliqz.
-- or --
Once restored, you will see the default New Tab page.
```

The second message could be optional. So if we know it will switch to another extension, include the message with the extension name, otherwise don't add the message.

Comment 28

[Mark Striemer [:mstriemer]]

Attachment: doorhanger-with-addon-name.png

Here's a doorhanger with the add-on name and icon in the body.

Comment 29

[Mark Striemer [:mstriemer]]

Attachment: Add the add-on name and icon in in a very not wonderful fashion

Review commit: https://reviewboard.mozilla.org/r/198282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198282/

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Striemer [:mstriemer] from comment #27)
> Maybe we can include another message in the body?
> 
> ```
> An extension, Tabby Cat, has changed the page you see when you open a
> New Tab. You can restore your settings if you do not want
> this change.
> 
> Once restored, your New Tab will be set by Cliqz.
> -- or --
> Once restored, you will see the default New Tab page.
> ```
> 
> The second message could be optional. So if we know it will switch to
> another extension, include the message with the extension name, otherwise
> don't add the message.

That's great! I like this better than my recommendation. I like including the second message too, but I can also see this working without it. Minor nit, on the first message I think we should say "Once restored, you New Tab page will be set by Cliqz" (I inserted "page" here to be consistent with your second message).

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203600

r=me assuming that we stick with stealing focus from the URL bar. I'd prefer not (as already mentioned before), but if UX disagrees then this can move forward. If we do change the behavior here I'll be very prompt with reviewing the changes so you can have a good chance at getting this in 58.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203602

Oh, actually I see now that the extension name code isn't in the patch, just the screenshot. I'll mark r+ once I get a chance to see that. Sorry for the bouncing around here.

Comment 33

[Mark Striemer [:mstriemer]]

It's in there, it's just a little... unorthodox. browser/components/extensions/ext-url-overrides.js line 70-74. I wasn't sure what the right way was to encode "%S" in the .dtd file so I just used "EXTENSION_NAME". I'll switch it to %S.

Including the second message will require some work in ExtensionSettingsStore to get the next extension that we think will control this. Should I file a follow up for that or would you like it done here?

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

You can file a follow up for that.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review203920

Oh! I see it is a different patch to review. This one looks fine.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8927071 [details]
Add the add-on name and icon in in a very not wonderful fashion

https://reviewboard.mozilla.org/r/198282/#review204170

::: browser/locales/en-US/chrome/browser/browser.dtd:968
(Diff revision 1)
>  <!ENTITY updateRestart.acceptButton.accesskey "R">
>  <!ENTITY updateRestart.cancelButton.label "Not Now">
>  <!ENTITY updateRestart.cancelButton.accesskey "N">
>  <!ENTITY updateRestart.panelUI.label2 "Restart to update &brandShorterName;">
>  
> -<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">
> +<!ENTITY newTabControlled.message "An extension, EXTENSION_NAME, has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">

Please add a localizer note that explains what EXTENSION_NAME is and how it will get replaced and that an image will be added alongside it.

Comment 37

[Mark Striemer [:mstriemer]]

Comment on attachment 8927071 [details]
Add the add-on name and icon in in a very not wonderful fashion

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198282/diff/1-2/

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8927071 [details]
Add the add-on name and icon in in a very not wonderful fashion

https://reviewboard.mozilla.org/r/198282/#review204202

Comment 39

[Andrew Swan [:aswan]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review204214

A few nits, but the big one is about how/when the observer is removed.

::: browser/components/extensions/ext-url-overrides.js:24
(Diff revision 5)
>  const STORE_TYPE = "url_overrides";
>  const NEW_TAB_SETTING_NAME = "newTabURL";
> +const NEW_TAB_SETTING_TYPE = "newTabNotification";
> +
> +let lastNewTabNotifiedId = null;
> +let observerRegistered = false;

Can you give this a more specifc name, e.g. `newTabObserverRegistered` or something?

::: browser/components/extensions/ext-url-overrides.js:39
(Diff revision 5)
> +  });
> +}
> +
> +function userWasNotified(extensionId) {
> +  let setting = ExtensionSettingsStore.getSetting(NEW_TAB_SETTING_TYPE, extensionId);
> +  return lastNewTabNotifiedId == extensionId

Is the point of `lastNewTabNotifiedId` to avoid showing a new notification if we've already showed one but the user hasn't taken any action on it yet?  If so, can you explain that in a comment.
Actually, upon further consideration, if that's the desired behavior, why don't we just remove the observer the first time it fires instead of keeping this extra state?

::: browser/components/extensions/ext-url-overrides.js:40
(Diff revision 5)
> +}
> +
> +function userWasNotified(extensionId) {
> +  let setting = ExtensionSettingsStore.getSetting(NEW_TAB_SETTING_TYPE, extensionId);
> +  return lastNewTabNotifiedId == extensionId
> +    || (setting && setting.value == "confirmed");

Can we just store a boolean here instead of a string?

::: browser/components/extensions/ext-url-overrides.js:71
(Diff revision 5)
> +  let doc = win.document;
> +  let panel = doc.getElementById("extension-notification-panel");
> +
> +  // Setup the command handler.
> +  let handleCommand = async (event) => {
> +    await handleNewTabNotificationCommand(

This is a matter of taste, but any reason not to just inline the logic from `handleNewTabNotificationCommands()` here?

::: browser/components/extensions/ext-url-overrides.js:90
(Diff revision 5)
> +  let action = CustomizableUI.getWidget(
> +    `${global.makeWidgetId(item.id)}-browser-action`);
> +  if (action) {
> +    action = action.areaType == "toolbar" && action.forWindow(win).node;
> +  }
> +  let menuButton = doc.getElementById("PanelUI-menu-button");

Can you move this to the line below?  There's no need to look it up if we have a toolbar button.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:19
(Diff revision 5)
> +
> +function getNewTabDoorhanger() {
> +  return document.getElementById("extension-new-tab-notification");
> +}
> +
> +function settingChanged(extensionId) {

Please name this `awaitSettingChanged` or `promiseSettingChanged`.  Actually do you need it?  If you're waiting for the popup to be dismissed, shouldn't the setting be applied by the time that happens?

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:29
(Diff revision 5)
> +}
> +
> +function clickKeepChanges(notification) {
> +  let button = document.getAnonymousElementByAttribute(
> +    notification, "anonid", "button");
> +  button.doCommand();

just curious, why `doCommand()` and not `click()`

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:277
(Diff revision 5)
> +  // FIXME: We need to enable the add-on so it gets cleared from the
> +  // ExtensionSettingsStore for now. See bug 1408226.

Hm, this won't be an issue once bug 1404584 lands.

Comment 40

[Markus Jaritz [:markus] (UX)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> r- because I still have questions about the re-showing of the notification
> as well as improving the wording of the popup.
> 
> So if a user quickly hits "Ctrl+T" (or Cmd+T on Mac) to open a new tab and
> then starts typing a search or address, the popup will hide and not return.
> 
> The user may only see a brief flash of the popup if they type fast upon
> opening a new tab and there isn't a way to get the notification back to see
> what it was.

The focus should be on the popup, so typing should not hide it.

> Can we show this on all new tab pages until the user has responded to it?

That might be too frequent, and can get annoying. If a user decided to not deal with this now, chances are, they also do not want to deal with it after a few minutes. I prefer to show it on every first newTab the users sees after restarting the browser. That might be a long enough interval for the user to re-consider. (and it's the same behavior Chrome uses.)



> > +<!ENTITY newTabControlled.message "An extension has changed the page you see when you open a New Tab. You can restore your settings if you do not want this change.">
> 
> We should include the extension name here. If the user decides to not take
> action right away, it would be good for them to know what to disable later.

I agree that this can help identify the reason for the problem. It does not add to solving it. Which is, what this message is about. If one wants to know the extension, they can have a look at the url bar, or in most cases also at the icon which this message is hanging from.
I do not think that having the name in the popup adds more value than having the message be shorter. (especially with longer extension names)

To your concern of not knowing what to disable:
The identity part of the url bar will show the extension name, so that people can remember what to disable later on. And in preferences, the extension will be mentioned as overriding the newTab.



> I understand what this is supposed to do (potentially revert to about:newtab
> or potentially revert to a different add-on) but this wording seems VERY
> confusing to users.
> 
> We are letting the ambiguity of what will happen surface to the user. But we
> should be able to figure out what will happen at the point of creating this
> notification.
> 
> Can we change this to three buttons (most users would only see two)?
> 
> [Keep changes][Disable $extensionName and revert to $extensionName2][Disable
> $extensionName]
> 
> The middle option would only be displayed if we know about a follow-up
> extension that would be controlling the new tab page if the current one gets
> disabled.
> 
> If we really can't do three buttons here, then I would prefer that we change
> the wording of the "Restore Settings" button to [Disable $extensionName and
> revert to $extensionName2]

I see where you are coming from. (A user that knows what extension has control of their newTab)
I expect users to be more aware of the state of their browser as they know and use it, then the names of the extensions. So the choice I want to give is: "I want to get back what I had before" and "I am fine with this change"... 
For option 1 people do not need to know about the specific previous extension, to make the choice to go back to what they had been using before.

Which is what I expressed in the copy above and has been checked and approved by Michelle.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Markus Jaritz [:designakt] (UX) from comment #40)

Thanks for the explanations! :)

Comment 42

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review204214

> just curious, why `doCommand()` and not `click()`

Because the handler is on "command". `click()` works though and does what I want so I changed it.

> Hm, this won't be an issue once bug 1404584 lands.

It looks like the setting is cleared in an `extension.callOnClose()` handler which looks like it's just a way to listen to the shutdown event. I'm not sure that's going to be fixed in that bug but maybe it should be? We should make sure we don't lose track of this either way.

Comment 43

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/5-6/

Comment 44

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/6-7/

Comment 45

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review205252

::: browser/base/content/utilityOverlay.js:467
(Diff revision 7)
>    case "tabshifted":
>      loadInBackground = !loadInBackground;
>      // fall through
>    case "tab":
> -    focusUrlBar = !loadInBackground && w.isBlankPageURL(url);
> +    focusUrlBar = !loadInBackground && w.isBlankPageURL(url)
> +      && !BROWSER_NEW_TAB_WILL_NOTIFY_USER;

jaws: I added this since waiting for the URL bar to get focused allowed some keystroked to pass through. I think we'd need something like this but I'm not sure if this is the best solution.

Comment 46

[Mark Striemer [:mstriemer]]

jaws: I updated the URL bar focusing code mentioned above and that bit could use another look. aswan can take a look at the other changes

Comment 47

[Andrew Swan [:aswan]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

https://reviewboard.mozilla.org/r/195958/#review206058

::: browser/components/extensions/ext-url-overrides.js:47
(Diff revision 7)
> +  let panel = doc.getElementById("extension-notification-panel");
> +
> +  // Setup the command handler.
> +  let handleCommand = async (event) => {
> +    let addon = await AddonManager.getAddonByID(item.id);
> +    if (event.originalTarget.getAttribute("anonid") == "button") {

nit: I think this would be simpler to follow if there were separate handlers for the two buttons but its up to you.

::: browser/components/extensions/ext-url-overrides.js:124
(Diff revision 7)
>      let {extension} = this;
>      let item = ExtensionSettingsStore[action](extension, STORE_TYPE, NEW_TAB_SETTING_NAME);
>      if (item) {
> -      aboutNewTabService.newTabURL = item.value || item.initialValue;
> +      setNewTabURL(item.id, item.value || item.initialValue);
>      }
> +    ExtensionSettingsStore.removeSetting(extension, NEW_TAB_CONFIRMED_TYPE, extension.id);

This gets called whenever the extension is updated, is the intention to re-prompt even if the url hasn't changed across an update?

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

We talked over IRC and will change from BROWSER_NEW_TAB_WILL_NOTIFY_USER to aboutNewTabService.willNotifyUser.

Comment 49

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/7-8/

Comment 50

[Mozilla Autoland]

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 8e91ffed6075 -d eb8da591a522: rebasing 435735:8e91ffed6075 "Bug 1390158 - Notify user of extension controlling New Tab on first access r=aswan,jaws" (tip)
merging browser/base/content/utilityOverlay.js
merging browser/components/customizableui/content/panelUI.inc.xul
merging browser/themes/shared/customizableui/panelUI.inc.css
warning: conflicts while merging browser/themes/shared/customizableui/panelUI.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 51

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please rebase the patch. Thank you.

Comment 52

[Mark Striemer [:mstriemer]]

Comment on attachment 8924732 [details]
Bug 1390158 - Notify user of extension controlling New Tab on first access

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195958/diff/8-9/

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10eb9d8523fa
Notify user of extension controlling New Tab on first access r=aswan,jaws

Comment 54

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/10eb9d8523fa

Comment 55

[Victor Carciu]

Verified with an add-on with icon and addon without icon. Please see attached screenshots and let me know if any other testing is needed here.

Comment 56

[Victor Carciu]

Attachment: Postifix screenshot extension without icon

Comment 57

[Victor Carciu]

Attachment: Postfix screenshot extension with icon

Comment 58

[Mark Striemer [:mstriemer]]

The doorhanger shouldn't be transparent. Is that an artifact of taking a screenshot or do you see it like that normally?

Comment 59

[Mark Striemer [:mstriemer]]

Otherwise it looks good, thanks.

Comment 60

[marius.santa]

The screenshot issue is because of the tool used, marking the issue as resolved.