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

VERIFIED FIXED in Firefox 62

Status

defect
P2
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: cbadescu, Assigned: mstriemer)

Tracking

(Blocks 1 bug)

61 Branch
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 disabled, firefox59 disabled, firefox60 disabled, firefox61 wontfix, firefox62 verified)

Details

Attachments

(4 attachments)

Reporter

Description

Last year
Posted image 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”.
Assignee

Updated

Last year
Assignee: nobody → mstriemer
Priority: -- → P2
Assignee

Updated

Last year
Severity: normal → enhancement
Iteration: --- → 62.1 - May 21
Comment hidden (mozreview-request)

Comment 3

Last year
mozreview-review
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?
Attachment #8975650 - Flags: review?(dao+bmo)
Assignee

Updated

Last year
Blocks: 1461735
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 6

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

Comment 9

Last year
mozreview-review
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?
Attachment #8975651 - Flags: review?(mak77)

Comment 10

Last year
mozreview-review
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").
Attachment #8975650 - Flags: review?(dao+bmo) → review+
Assignee

Comment 11

Last year
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 14

Last year
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

Last year
mozreview-review
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.
Attachment #8975651 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

Last year
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

Last year
Pushed by mstriemer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/db492c0538da
https://hg.mozilla.org/mozilla-central/rev/d29d89978c45
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Is this something which can ride the trains or should we consider it for backport?
Flags: needinfo?(mstriemer)
Assignee

Comment 24

Last year
I think it's okay to ride the trains. Hidden tabs are already shown in the all tabs menu.
Flags: needinfo?(mstriemer)

Comment 25

Last year
Posted image 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.

Updated

Last year
Status: RESOLVED → VERIFIED

Updated

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