Closed Bug 1470865 Opened Last year Closed Last year

Unmuted tabs are not displayed under the “Hidden Tabs”

Categories

(Firefox :: Tabbed Browser, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox61 --- unaffected
firefox62 --- verified
firefox63 --- verified

People

(Reporter: cbadescu, Assigned: mstriemer)

References

Details

Attachments

(4 files)

Attached image Unmuted.gif
[Affected versions]:
- Firefox 62.0a1 (20180624220331)

[Affected platforms]:
- Win 7 64-bit
- Mac OS X 10.13.3
- Ubuntu 14.04 32-bit

[Steps to reproduce]:
1.Install https://addons.mozilla.org/en-US/firefox/addon/hide-tab/ 
2.Hide some tabs that play audio content.
3.Click on the “v” button.
4.Mute some tabs.
5.Wait until the muted tabs are moved in the “Hidden Tabs” menu.
6.Click on the “Hidden Tabs”.
7.Unmute some tabs.
8.Click on the “<” arrow to go back.

[Expected results]:
- The unmuted tabs are displayed again under the “Hidden Tabs” menu.

[Actual results]:
- The unmuted tabs are not displayed after you navigate back.
- You need to reopen the all tabs menu to see the unmuted tab again.

Please see the attached video .
Mark, can you please take a look?
Flags: needinfo?(mstriemer)
Priority: -- → P2
Assignee: nobody → mstriemer
Flags: needinfo?(mstriemer)
This will handle adding rows for tabs that start to match the criteria for a certain section. The likely case would be muting or unmuting hidden tabs, but could also be hiding or unhiding tabs.

This doesn't handle the case where a new tab opens while the menu is still open.
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review266992

::: browser/modules/TabsList.jsm:88
(Diff revision 2)
> +  _addElement(elementOrFragment) {
> +    if (this.insertBefore) {
> +      this.insertBefore.parentNode.insertBefore(elementOrFragment, this.insertBefore);
> +    } else {
> +      this.containerNode.appendChild(elementOrFragment);
> +    }

Can this be simplified to:

this.containerNode.insertBefore(elementOrFragment, this.insertBefore);

?

insertBefore() appends to the end if the second argument is null.

::: browser/modules/TabsList.jsm:118
(Diff revision 2)
>  
>    _tabAttrModified(tab) {
>      let item = this.tabToElement.get(tab);
>      if (item) {
>        if (!this.filterFn(tab)) {
> -        // If the tab is no longer in this set of tabs, hide the item.
> +        // The tab no longer matches our criteria, hide it.

s/hide/remove/

::: browser/modules/TabsList.jsm:130
(Diff revision 2)
> +      this._addTab(tab);
> +    }
> +  }
> +
> +  _addTab(newTab) {
> +    // We want to keep the order of tabToElement, so recreate it.

Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.
Attachment #8991508 - Flags: review?(dao+bmo)
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review266992

> Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.

The order is used to decide where to put the new entry. Maintaining the order will keep the tabs in order, it also seem intuitive for the filtered set of tabs to be in order still.
(In reply to Mark Striemer [:mstriemer] from comment #6)
> Comment on attachment 8991508 [details]
> Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu
> 
> https://reviewboard.mozilla.org/r/256410/#review266992
> 
> > Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.
> 
> The order is used to decide where to put the new entry.

This gets the order from gBrowser.tabs, not from tabToElement, as far as I can tell.

> Maintaining the
> order will keep the tabs in order, it also seem intuitive for the filtered
> set of tabs to be in order still.

The primary Map use case is to keep track of relationships. Order is a secondary feature and not generally expected from Maps, and if you really care about this you should probably use a different data structure such as an array, where you can insert items in the middle without rebuilding everything.
Attachment #8991508 - Flags: review?(dao+bmo)
You're right, maintaining the order isn't necessary for the current code. I've removed the rebuilding of the Map to maintain the order.
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review268220

::: browser/modules/TabsList.jsm:133
(Diff revision 4)
> +        // If there is a new row, insert it before this one.
> +        let row = this.tabToElement.get(tab);
> +        row.parentNode.insertBefore(newRow, row);
> +        newRow = null;
> +      }
> +    }

This loop looks equivalent to (but slower than) e.g.:

let newRow = this._createRow(newTab);
let nextTab = newTab.nextSibling;
while (nextTab && !this.filterFn(nextTab)) {
  nextTab = nextTab.nextSibling;
}
let nextRow = nextTab && this.tabToElement.get(nextTab);
nextRow.parentNode.insertBefore(newRow, nextRow);
Attachment #8991508 - Flags: review?(dao+bmo)
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review268418
Attachment #8991508 - Flags: review?(dao+bmo) → review+
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8880495b4615
Dynamically show hidden audio tabs in all tabs menu r=dao
https://hg.mozilla.org/mozilla-central/rev/8880495b4615
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attached image Bug1470865.gif
This issue is verified as fixed on Firefox 63.0a1(20180807220134) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.
Status: RESOLVED → VERIFIED
Is this something we ought to uplift to beta?
Flags: needinfo?(mstriemer)
Dão, do you want to request uplift?
Flags: needinfo?(dao+bmo)
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Approval Request Comment
[Feature/Bug causing the regression]: bug 1446101
[User impact if declined]: Recently unmuted audio tabs won't re-appear in the all tabs menu when it is open
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Small code change that has been on Nightly for a while
[String changes made/needed]: No

Sorry, I thought I had submitted this. I'm not sure this is necessary for uplift but it shouldn't hurt so could be worth it.
Flags: needinfo?(mstriemer)
Attachment #8991508 - Flags: approval-mozilla-beta?
Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Fix for minor regression, let's uplift for beta 19.
Attachment #8991508 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(dao+bmo)
Confirmed issue on 62.0b18 on macOS 10.12 using steps in Comment 0.
Verified fix on macOS 10.12, Ubuntu 18.04LTS, Windows 10x64 and can confirm the issue is no longer reproducible.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.