1455300 - The “Hidden Tabs” option is not displayed in “History” from the menu bar

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[CosminB]

Attachment: HiddenTabs.gif

[Affected versions]:
- Firefox 61.0a1 (20180419100148)

[Affected platforms]:
- Win 7 64-bit
- Mac OS 10.13.2

[Steps to reproduce]:
1.Flip “extensions.webextensions.tabhide.enabled” to true.
2.Open multiple tabs.
3.Hide some tabs.
4.Open the menu bar.
5.Click on “History”.
6.Observe the list.

[Expected results]:
- The “Hidden Tabs” option is displayed as presented in https://mozilla.invisionapp.com/share/82EIATQAF#/screens/280879353 

[Actual results]:
- The “Hidden Tabs” option is not visible in “History”.

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

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

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

https://reviewboard.mozilla.org/r/243888/#review249934

::: browser/base/content/tabbrowser.xml:2032
(Diff revision 1)
> -      </method>
> +              filterFn: (tab) => !tab.pinned && !tab.hidden,
> +            });
> +          }
> +          return this._allTabsPopup;
> +        ]]></getter>
> +      </property>

<field name="allTabsPopup" readonly="true"><![CDATA[
          new TabsPopup({
            className: "alltabs-item",
            doc: document,
            gBrowser,
            filterFn: (tab) => !tab.pinned && !tab.hidden,
          })
        ]]></field>

::: browser/base/content/tabbrowser.xml:2143
(Diff revision 1)
>        <handler event="popuphidden">
>        <![CDATA[
> -        if (event.target.getAttribute("id") == "alltabs_containersMenuTab") {
> -          return;
> -        }
> -
> +        if (event.target.getAttribute("id") == "alltabs_hiddenTabsMenu") {
> +          this.hiddenTabsPopup.cleanup();
> +        } else if (event.target == this) {
> +          this.allTabsPopup.cleanup();

Can TabsPopup.jsm have its own popuphidden handler instead?

::: browser/modules/TabsPopup.jsm:28
(Diff revision 1)
> +   *                   A function to filter which tabs are added to the popup.
> +   */
> +  constructor({className, doc, gBrowser, popup, filterFn}) {
> +    this.className = className;
> +    this.doc = doc;
> +    this.gBrowser = gBrowser;

You can just use doc.defaultView.gBrowser here instead of that extra parameter.

::: browser/modules/TabsPopup.jsm:82
(Diff revision 1)
> +  }
> +
> +  /*
> +   * Cleanup the listeners, cleanup() should call this for you.
> +   */
> +  cleanupListeners() {

_cleanupListeners

::: browser/modules/TabsPopup.jsm:143
(Diff revision 1)
> +
> +    let classNames = "menuitem-iconic menuitem-with-favicon";
> +    if (this.className) {
> +      classNames += ` ${this.className}`;
> +    }
> +    item.setAttribute("class", classNames);

nit: use classList.add

::: browser/modules/TabsPopup.jsm:148
(Diff revision 1)
> +    item.setAttribute("class", classNames);
> +    this._setMenuitemAttributes(item, tab);
> +
> +    this.tabToMenuitem.set(tab, item);
> +
> +    item.addEventListener("command", this._handleCommand);

nit: item.addEventListener("command", this) and call _handleCommand from handleEvent

::: browser/modules/TabsPopup.jsm:170
(Diff revision 1)
> +    }
> +
> +    if (tab.hasAttribute("pending"))
> +      item.setAttribute("pending", tab.getAttribute("pending"));
> +    else
> +      item.removeAttribute("pending");

This seems unused. Can you please file a followup on removing this?

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

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

Comment 5

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 6

[Mark Striemer [:mstriemer]]

I updated this to register popupshowing/popuphidden listeners in TabsPopup which made `popup` required and helped clean things up a bit.

I noticed a race condition between the menuitem elements being added and the code to set the visible tab status, so that got moved into an `onPopulate` callback.

This is handling the case where a tab no longer matches the filter but not when a tab starts matching it. I figured the simplest way to deal with that would be to cleanup/populate again and that didn't seem worth it.

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

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

Comment 8

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

https://reviewboard.mozilla.org/r/243440/#review251052

Just a couple questions

::: browser/base/content/browser-menubar.inc:349
(Diff revision 3)
>                            hidden="true"/>
>                  <menuitem id="historyRestoreLastSession"
>                            label="&historyRestoreLastSession.label;"
>                            command="Browser:RestoreLastSession"/>
> +                <menu id="hidden-tabs-menu"
> +                      class="hiddenTabsMenu"

There must be a reason why this, historyUndoMenu and historyUndoWindowMenu have a class that is used only for getElementsByClassName, when they also have an id. But I can't think of one. Is the id not enough? May we fix all of these in case?

::: browser/base/content/browser-places.js:612
(Diff revision 3)
>    this.__proto__.__proto__ = PlacesMenu.prototype;
> +  let hiddenTabsMenu = document.getElementById("hidden-tabs-menu").firstChild;
> +  this._hiddenTabsPopup = new TabsPopup({
> +    filterFn: (tab) => tab.hidden,
> +    popup: hiddenTabsMenu,
> +  });

IIUC, once HistoryMenu has been created, this will constantly listen to tab changes. Is that ok from a perf point of view, or should we rather populate a static list of tabs on popupshowing?
Also, only few users will use the hiding feature (through WebExt) while this menu sounds like a cost for everyone.
Maybe we should create this object only on the first hiddenTabsMenu popupshowing, rather than always?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

https://reviewboard.mozilla.org/r/243888/#review252134

::: browser/modules/moz.build:96
(Diff revision 3)
>  with Files("SitePermissions.jsm"):
>      BUG_COMPONENT = ("Firefox", "Site Identity and Permission Panels")
>  
>  with Files("OpenInTabsUtils.jsm"):
>      BUG_COMPONENT = ("Firefox", "Tabbed Browser")
>  

Please create an entry for TabsPopup.jsm here with BUG_COMPONENT = ("Firefox", "Tabbed Browser").

Comment 11

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

https://reviewboard.mozilla.org/r/243440/#review251052

> There must be a reason why this, historyUndoMenu and historyUndoWindowMenu have a class that is used only for getElementsByClassName, when they also have an id. But I can't think of one. Is the id not enough? May we fix all of these in case?

I couldn't tell why it wasn't using the id. It seems better to use the id to me, I can update them.

> IIUC, once HistoryMenu has been created, this will constantly listen to tab changes. Is that ok from a perf point of view, or should we rather populate a static list of tabs on popupshowing?
> Also, only few users will use the hiding feature (through WebExt) while this menu sounds like a cost for everyone.
> Maybe we should create this object only on the first hiddenTabsMenu popupshowing, rather than always?

It will only listen for tab changes when the hidden tabs menu is active. The `TabsPopup` constructor just listens for "popupshowing" on the `popup` element.

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

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

Comment 13

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 14

[Mark Striemer [:mstriemer]]

I addressed the review comments, but I also just tried out the Synced Tabs button in this menu for the first time and it just opens a panel off the app menu. That might be a better solution here. I have a patch that is branched off of this one to update the all tabs menu to a photon panel. I'm going to run this past UX tomorrow and possibly flip the dependency around on these patches.

I thought we'd need to keep the old code from the all tabs menu to handle this menu but if it just opens the other menu we can delete the old code instead.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

https://reviewboard.mozilla.org/r/243440/#review252876

LGTM, modulo the UX decision that you are handling apart.

Comment 16

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 17

[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 4bcf34f9e4353bc079131951e7acd46660077b71 -d f53afd7c7680: rebasing 465471:4bcf34f9e435 "Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml r=dao"
merging browser/base/content/browser.js
merging browser/base/content/tabbrowser.xml
merging browser/modules/moz.build
warning: conflicts while merging browser/modules/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 18

[Mark Striemer [:mstriemer]]

Comment on attachment 8975650 [details]
Bug 1455300 - Part 1: Extract tab menuitem code from tabbrowser.xml

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

Comment 19

[Mark Striemer [:mstriemer]]

Comment on attachment 8975651 [details]
Bug 1455300 - Part 2: Include hidden tabs in the History menu

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

Comment 20

[Mark Striemer [:mstriemer]]

The history menu will change in the photon re-style but I think it makes sense to land this as-is since the photon patch depends on it and it's a logical stepping point for that patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db492c0538da
Part 1: Extract tab menuitem code from tabbrowser.xml r=dao
https://hg.mozilla.org/integration/autoland/rev/d29d89978c45
Part 2: Include hidden tabs in the History menu r=mak

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/db492c0538da
https://hg.mozilla.org/mozilla-central/rev/d29d89978c45

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something which can ride the trains or should we consider it for backport?

Comment 24

[Mark Striemer [:mstriemer]]

I think it's okay to ride the trains. Hidden tabs are already shown in the all tabs menu.

Comment 25

[marius.santa]

Attachment: hidden tabs.jpg

Issue reproduced in Firefox 61.0a1 (20180419100148).
Retested and verified in Firefox 62.0a1 (20180529100118) on Windows 10 64Bit, MacOS 10.13.3.