1438363 - Show a doorhanger when an extension hides a tab for the first time

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P1)

Description

[Mark Striemer [:mstriemer]]

When an extension hides a tab for the first time users might not know what happened to the tab. A doorhanger should appear to tell the user what has happened and how to find their hidden tabs.

You can see the flow in the mocks [1].

[1] https://mozilla.invisionapp.com/share/82EIATQAF#/screens/266445461

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Required to ship the new tab hiding api without being behind a pref. Tracking61+

Comment 2

[Mark Striemer [:mstriemer]]

Bumping to P1 since this is targeting 61.

Comment 3

[Mark Striemer [:mstriemer]]

Attachment: Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 4

[Mark Striemer [:mstriemer]]

Attachment: new-arrow-icon.png

@dao I updated the icon for the all tabs button which is used in a few different places. It should match the photon guidelines now and the weight is slightly different.

I'm not sure if you're the right person to look at that but I figured I'd ask. I looked at the places I found references to it and they all look as good or better but the customize section I couldn't really tell. There's an image attached for that one, maybe I can get your thoughts?

I made some minor updates to the browser.xul and PanelUI.inc.xul. I'm not even sure those are worth you looking at, though.

Comment 5

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review245444

::: browser/components/extensions/parent/ext-tabs.js:42
(Diff revision 2)
> +    descriptionMessageId: "tabHideControlled.message",
> +    getLocalizedDescription: (doc, message, addonDetails) => {
> +      let image = doc.createElement("image");
> +      image.src = "chrome://global/skin/icons/arrow-dropdown-16.svg";
> +      image.setAttribute("class", "extension-controlled-icon");
> +      image.style.margin = "0 -2px 1px -1px";

why is this inlined here rather than being done with a class and rules in extensions.inc.css?

::: browser/components/extensions/parent/ext-tabs.js:46
(Diff revision 2)
> +      image.setAttribute("class", "extension-controlled-icon");
> +      image.style.margin = "0 -2px 1px -1px";
> +      return BrowserUtils.getLocalizedFragment(doc, message, addonDetails, image);
> +    },
> +    learnMoreMessageId: "tabHideControlled.learnMore",
> +    learnMoreLink: "extension-hiding-tabs",

Hm this page is 404 for me:
https://support.mozilla.org/1/firefox/60.0a1/Darwin/en-US/extension-hiding-tabs

::: browser/components/extensions/parent/ext-tabs.js:1262
(Diff revision 2)
>                  hidden.push(tabTracker.getId(tab));
>                }
>              }
>            }
> +          if (hidden.length > 0) {
> +            let win = Services.wm.getMostRecentWindow("navigator:browser");

This doesn't seem right if the tab being hidden is not in the foreground window.  I'm not sure what's right though.  Please file a followup issue for this.

::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:79
(Diff revision 2)
> +
> +  await extension.unload();
> +}
> +
> +add_task(async function test_doorhanger_keep() {
> +  await doorhangerTest(async function(extension) {

nit: can just return doorhangerTest (and this function does not need to be async)

Comment 7

[Mark Striemer [:mstriemer]]

Hi Joni,

We need a SUMO article to describe how tab hiding works. Is this something that has come across your radar? It would be similar to the page created in bug 1414029 for the homepage and new tab overrides.

The URL we're expecting it to be at is https://support.mozilla.org/1/firefox/60.0a1/Darwin/en-US/extension-hiding-tabs. Would you be able to at least create a placeholder for this and work on some content for it?

Thanks!

Comment 8

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review245778

::: browser/components/extensions/parent/ext-tabs.js:40
(Diff revision 3)
> +    popupnotificationId: "extension-tab-hide-notification",
> +    descriptionId: "extension-tab-hide-notification-description",
> +    descriptionMessageId: "tabHideControlled.message",
> +    getLocalizedDescription: (doc, message, addonDetails) => {
> +      let image = doc.createElement("image");
> +      image.src = "chrome://global/skin/icons/arrow-dropdown-16.svg";

This can also be moved to css right?  eg
https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/toolkit/themes/shared/extensions/extensions.inc.css#78-80

I'm no front-end expert though, this is also fine by me.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review245838

::: browser/base/content/browser.xul:669
(Diff revision 3)
>                       ondragexit="newTabButtonObserver.onDragExit(event)"
>                       cui-areatype="toolbar"
>                       removable="true"/>
>  
>        <toolbarbutton id="alltabs-button"
> -                     class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button"
> +                     class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button badged-button"

This doesn't look like it should be part of this bug? I don't see you setting a badge anywhere.

::: browser/themes/shared/customizableui/panelUI.inc.css:345
(Diff revision 3)
>  }
>  
> +.extension-controlled-icon.alltabs-icon {
> +  /* This icon has a lot of extra space to the sides, reduce that a little. */
> +  margin: 0 -1px 1px;
> +}

I don't think this file is the right place for this. Admittedly "panelUI" is a misleading name that may wrongly suggest that this a dumping ground for any piece of UI that happens to be a panel.

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review245838

> This doesn't look like it should be part of this bug? I don't see you setting a badge anywhere.

It was added in the audio indicator bug and is needed to have the doorhanger display correctly. I'll remove it since it will already be added by that once this lands.

> I don't think this file is the right place for this. Admittedly "panelUI" is a misleading name that may wrongly suggest that this a dumping ground for any piece of UI that happens to be a panel.

I made a new CSS file in the extensions namespace and included it in browser.xul. I moved all of the panel rules for extensions from here to that file.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review246088

::: browser/components/extensions/extension-panel.css:1
(Diff revision 4)
> +@namespace xhtml "http://www.w3.org/1999/xhtml";

Please remove this.

::: browser/components/extensions/extension-panel.css:35
(Diff revision 4)
> +}
> +
> +.extension-controlled-notification > .popup-notification-body-container > .popup-notification-body > .popup-notification-warning,
> +.extension-controlled-notification > .popup-notification-body-container > .popup-notification-icon {
> +  display: none;
> +}

Please move this file to browser/themes/shared/, and then you just can %include it in browser/themes/shared/browser.inc.css rather than loading it separately in browser.xul.

Comment 14

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review246088

> Please move this file to browser/themes/shared/, and then you just can %include it in browser/themes/shared/browser.inc.css rather than loading it separately in browser.xul.

That was much nicer than including it in browser.xul. Thanks.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

https://reviewboard.mozilla.org/r/239672/#review246964

::: browser/components/customizableui/content/panelUI.inc.xul:751
(Diff revision 5)
> +                     dropmarkerhidden="true"
> +                     checkboxhidden="true">
> +    <popupnotificationcontent orient="vertical">
> +      <description id="extension-tab-hide-notification-description"/>
> +    </popupnotificationcontent>
> +  </popupnotification>

Can we also move this to browser.xul or some separately included file? Can be a followup bug.

Thanks!

Comment 17

[Joni Chan]

I've set up a placeholder link now. We'll work on content in a couple of weeks.

Comment 18

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 19

[Mark Striemer [:mstriemer]]

Comment on attachment 8970918 [details]
Bug 1438363 - Show a doorhanger when an extension first hides a tab

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63f79d4510d9
Show a doorhanger when an extension first hides a tab r=aswan,dao

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/63f79d4510d9

Comment 22

[CosminB]

Attachment: Bug1438363.gif

This bug is verified as fixed on Firefox 61.0a1 (20180503095637) under Win 7 64-bit and Mac OS 10.13.3.

Hiding for the first time some tabs will display a pop-up with 2 options "Keep Tabs Hidden" and "Disable Extension".

Comment 23

[Will Bamberg [:wbamberg]]

I've noted this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/hide

Please let me know if you need anything else.

Comment 24

[Mark Striemer [:mstriemer]]

Looks good, thanks!

Comment 25

[Joni Chan]

Here's a link to the draft you can edit. Please comment with any suggested changes and we'll publish it.

https://docs.google.com/document/d/1FIWnksOLfHBSrYjXJJtoH0VmVT7mlaQoYtkZV1Aups8/edit?usp=sharing

Comment 26

[Mark Striemer [:mstriemer]]

Thanks Joni, I've added some comments.