Closed Bug 1408061 Opened 7 years ago Closed 6 years ago

Show that tabs are hidden

Categories

(WebExtensions :: Frontend, enhancement, P2)

Product:
WebExtensions
Component:
Frontend
Type:
enhancement

Tracking

(firefox57 wontfix, firefox61 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox61 --- verified

People

(Reporter: andy+bugzilla, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

hidden-tabs-one-button.mov.gif
467.79 KB, image/gif
Details
Bug 1408061 - Show hidden tabs in all tabs menu
59 bytes, text/x-review-board-request
dao
: review+
Details
PlaceHolder.gif
2.27 MB, image/gif
Details
Bug1408061.gif
259.89 KB, image/gif
Details 
If tabs can be hidden, but are not suspended, they can be running and consuming resources network traffic and so on. We should do our best to let users know that an add-on has hidden tabs.
status-firefox57: --- → wontfix
Priority: -- → P3
Assignee: nobody → mstriemer
Priority: P3 → P2
Attached video tab-hide-menu.mov (obsolete) — 
Here's a patch that shows the hidden tabs in the all tabs menu.

I'm not sure if the code should be optimised, but it probably should be, let me know if you'd like me to do that.

I didn't see any tests for the current all tabs menu so I didn't add any tests that clicks on the hidden tabs but I'd be happy to add one if it seems useful.
Comment on attachment 8958664 [details]
Bug 1408061 - Show hidden tabs in all tabs menu

https://reviewboard.mozilla.org/r/227584/#review236714

This is close, but can you not add a copy of the button that moves with the tabs? We do this for the new tab button because it marks the place where the tab will be added, but generally I think it's preferable to not have buttons move around.

::: browser/base/content/tabbrowser.xml:457
(Diff revision 1)
>          ]]></body>
>        </method>
>  
> +      <method name="_updateHiddenTabsStatus">
> +        <body><![CDATA[
> +          if (this.tabbrowser.visibleTabs.length < this.tabbrowser.tabs.length) {

FYI: this.tabbrowser has been removed, please use gBrowser instead
Attachment #8958664 - Flags: review?(dao+bmo)
I rebased with the gBrowser change and removed the extra button. It is always at the end of the tab bar now.
Attachment #8958665 - Attachment is obsolete: true
Comment on attachment 8958664 [details]
Bug 1408061 - Show hidden tabs in all tabs menu

https://reviewboard.mozilla.org/r/227584/#review238376

::: browser/base/content/browser.xul
(Diff revision 2)
> -                label="&newUserContext.label;">
> -            <menupopup id="alltabs_containersMenuTab" />
> -          </menu>
> -          <menuseparator id="alltabs-popup-separator-2"/>
> -        </menupopup>
> -      </toolbarbutton>

These changes aren't needed anymore, are they?

::: browser/base/content/tabbrowser.xml:465
(Diff revision 2)
>        </method>
>  
> +      <method name="_updateHiddenTabsStatus">
> +        <body><![CDATA[
> +          if (gBrowser.visibleTabs.length < gBrowser.tabs.length) {
> +            this.setAttribute("hashiddentabs", "");

nit: this.setAttribute("hashiddentabs", "true");

::: browser/base/content/tabbrowser.xml:2212
(Diff revision 2)
>            return;
> +        } else if (event.target.getAttribute("id") == "alltabs_hiddenTabsMenu") {
> +          let menu = event.target;
> +          menu.textContent = "";
> +
> +          gBrowser.tabs.forEach(tab => {

nit, this is a bit more lightweight:

for (let tab of gBrowser.tabs) {

::: browser/base/content/tabbrowser.xml:2214
(Diff revision 2)
> +          let menu = event.target;
> +          menu.textContent = "";
> +
> +          gBrowser.tabs.forEach(tab => {
> +            if (tab.hidden) {
> +              menu.appendChild(this._createTabMenuItem(tab, menu));

_createTabMenuItem has been changed in bug 1388502, it doesn't take a second argument anymore

::: browser/base/content/tabbrowser.xml:2278
(Diff revision 2)
>          // clear out the menu popup and remove the listeners
> -        for (let i = this.childNodes.length - 1; i > 0; i--) {
> -          let menuItem = this.childNodes[i];
> +        for (let i = container.childNodes.length - 1; i > 0; i--) {
> +          let menuItem = container.childNodes[i];
>            if (menuItem.tab) {
>              menuItem.tab.mCorrespondingMenuitem = null;
> -            this.removeChild(menuItem);
> +            container.removeChild(menuItem);

You can just use menuItem.remove()

::: browser/base/content/tabbrowser.xml:2282
(Diff revision 2)
>              menuItem.tab.mCorrespondingMenuitem = null;
> -            this.removeChild(menuItem);
> +            container.removeChild(menuItem);
>            }
>            if (menuItem.hasAttribute("usercontextid")) {
> +            // TODO: Should this be container?
>              this.removeChild(menuItem);

ditto

::: browser/base/content/tabbrowser.xml:2287
(Diff revision 2)
>              this.removeChild(menuItem);
>            }
>          }
> +
> +        if (container == this) {
> -        var tabcontainer = gBrowser.tabContainer;
> +          var tabcontainer = gBrowser.tabContainer;

nit: please use let here, or get rid of the variable and use gBrowser.tabContainer directly

::: browser/themes/shared/tabs.inc.css:712
(Diff revision 2)
>    list-style-image: url(chrome://browser/skin/tabbrowser/newtab.svg);
>  }
>  
>  /* All tabs button and menupopup */
>  
> -#alltabs-button {
> +.tabs-alltabs-button {

Revert this?
Attachment #8958664 - Flags: review?(dao+bmo)
Attachment #8958664 - Attachment is obsolete: true
Comment on attachment 8964685 [details]
Bug 1408061 - Show hidden tabs in all tabs menu

https://reviewboard.mozilla.org/r/233376/#review239100
Attachment #8964685 - Flags: review?(dao+bmo) → review+
Comment on attachment 8964685 [details]
Bug 1408061 - Show hidden tabs in all tabs menu

https://reviewboard.mozilla.org/r/233376/#review239364

::: browser/base/content/browser.xul:706
(Diff revision 1)
>              <menupopup id="alltabs_containersMenuTab" />
>            </menu>
>            <menuseparator id="alltabs-popup-separator-2"/>
> +          <menu id="alltabs_hiddenTabs"
> +                label="&hiddenTabs.label;">
> +            <menupopup id="alltabs_hiddenTabsMenu" />

nit: remove the space before />
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f9ca8ea57a8a
Show hidden tabs in all tabs menu r=dao
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f9ca8ea57a8a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attached image PlaceHolder.gif — 
I have noticed that after I install https://addons.mozilla.org/en-US/firefox/addon/hide-tab/ and I check the option “Suspend tabs when hiding”, the hidden tabs have a placeholder instead of their icons if you restart the browser with the tabs hidden.

Is this expected?

Also, the placeholder will not be displayed if you load the suspended page that has a placeholder and you hide it again. (the option “Suspend tabs when hiding” is still checked in this case.)
Flags: needinfo?(mstriemer)
It just looked at the code for that extension and it just calls `browser.tabs.discard()` after hiding a tab. Doing so on a visible tab immediately removes its favicon. I think this is an issue with the discard function rather than hidden tabs.
Flags: needinfo?(mstriemer)
See Also: → 1453432
See Also:
Bug 1427822
Bug 1451799
(bugs similar to Bug 1453432)
Attached image Bug1408061.gif — 
This issue is verified as fixed on Firefox 61.0a1 (20180412001050) under Win 7 64-bit and Mac OS X 10.13.2.

The preference “extensions.webextensions.tabhide.enabled” is set to false by default.

After the preference is set to true, the hidden tabs are displayed in the “Hidden Tabs” list.

Please see the attached video
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: