Closed Bug 1438363 Opened 6 years ago Closed 6 years ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

defect

Tracking

(firefox61+ verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 + verified

People

(Reporter: mstriemer, Assigned: mstriemer)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

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
Required to ship the new tab hiding api without being behind a pref. Tracking61+
Blocks: 1455040
Bumping to P1 since this is targeting 61.
Priority: P3 → P1
Attachment #8970918 - Flags: review?(dao+bmo)
Attached image 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 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)
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!
Flags: needinfo?(jsavage)
Blocks: 1457000
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.
Attachment #8970918 - Flags: review?(aswan) → review+
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 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 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 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 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!
Attachment #8970918 - Flags: review?(dao+bmo) → review+
I've set up a placeholder link now. We'll work on content in a couple of weeks.
Flags: needinfo?(jsavage)
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63f79d4510d9
Show a doorhanger when an extension first hides a tab r=aswan,dao
Depends on: 1458832
Depends on: 1458833
https://hg.mozilla.org/mozilla-central/rev/63f79d4510d9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attached image 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".
Status: RESOLVED → VERIFIED
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.
Flags: needinfo?(mstriemer)
Looks good, thanks!
Flags: needinfo?(mstriemer)
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
Flags: needinfo?(mstriemer)
Thanks Joni, I've added some comments.
Flags: needinfo?(mstriemer)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.