Remove the tabbrowser-tabpanels binding

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: dao, Assigned: bgrins)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
(Reporter)

Comment 3

a year ago
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

Hmm, I think we can take a more radical approach: remove the tabbrowser-tabpanels binding
Attachment #8955461 - Attachment is obsolete: true
Attachment #8955461 - Flags: review?(mconley)
(Reporter)

Updated

a year ago
Blocks: 1397874
Summary: Avoid creating an async tab switcher when attempting to select a tab that is already selected → Remove the tabbrowser-tabpanels binding
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Priority: -- → P1
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Blocks: 1443849

Comment 8

a year ago
mozreview-review
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/224630/#review231738

This is great, thank you!
Attachment #8955461 - Flags: review?(mconley) → review+

Comment 9

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d679c9a9afa0
Remove the tabbrowser-tabpanels binding. r=mconley
(Reporter)

Comment 10

a year ago
mozreview-review
Comment on attachment 8955461 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/224630/#review231746

::: testing/mochitest/browser-test.js:247
(Diff revision 4)
>    // The selector for just this element
>    function immediateSelector(element) {
> -    if (element.localName == "notificationbox" && element.parentNode &&
> -        element.parentNode.classList.contains("tabbrowser-tabpanels")) {
> +    if (element.localName == "notificationbox" &&
> +        element.parentNode &&
> +        element.parentNode.parentNode &&
> +        element.parentNode.parentNode.classList.contains("tabbrowser-tabbox")) {

This should have been element.parentNode.id == "tabbrowser-tabpanels" since bug 1442651 landed in the meantime.

Not sure if this will cause a failure. I'd push a followup but autoland makes this hard. If this fails gracefully, I'll push a followup to inbound tomorrow.

Comment 11

a year ago
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af2dffa14ef9
Backed out changeset d679c9a9afa0 for mochitest browser chrome failures at browser_ext_tabs_create.js on a CLOSED TREE
Comment hidden (mozreview-request)
(Reporter)

Comment 13

a year ago
I'm getting all kinds of failures that I didn't get last week:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=160d6494e22dfa09956a772d958e573d6097b7b6
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Depends on: 1445949
(Reporter)

Updated

a year ago
Depends on: 1445948
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
No longer blocks: 1443849
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Blocks: 1476769
(Assignee)

Comment 18

8 months ago
I still see failures on a rebased version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02408fa0717d4cb4f2e18d5f00c8173dc66d60e&selectedJob=188819863.

Dão, is there any chance you could take a look at this and try to get it re-landed?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 19

8 months ago
I couldn't figure out the failures last time I checked (let alone why I got suddenly a lot more of them). I was going to take another look but never got around to it... and I probably won't in the near future, so unassigning for now. I'll leave the needinfo in place as a reminder, but feel free to pick this up in the meantime.
Assignee: dao+bmo → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 21

8 months ago
It's not actually clear to me that the `_getSwitcher().requestTab` code needs to run unconditionally as opposed to moving it into the existing "select" callback which only fires when the panel actually changes, but this patch keeps the current semantics.
(Assignee)

Comment 22

8 months ago
I can try moving it into the "select" listener if you'd prefer it there.
(Reporter)

Comment 23

8 months ago
(In reply to Brian Grinstead [:bgrins] from comment #21)
> It's not actually clear to me that the `_getSwitcher().requestTab` code
> needs to run unconditionally as opposed to moving it into the existing
> "select" callback which only fires when the panel actually changes,

Right, that wasn't clear to me either. My patch tried to change that and perhaps this uncovered some deeper AsyncTabSwitcher issues.

(In reply to Brian Grinstead [:bgrins] from comment #22)
> I can try moving it into the "select" listener if you'd prefer it there.

Certainly would be nice to get this cleaned up or at least understand why things need to be set up this way.
(Assignee)

Comment 24

8 months ago
(In reply to Dão Gottwald [::dao] from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > It's not actually clear to me that the `_getSwitcher().requestTab` code
> > needs to run unconditionally as opposed to moving it into the existing
> > "select" callback which only fires when the panel actually changes,
> 
> Right, that wasn't clear to me either. My patch tried to change that and
> perhaps this uncovered some deeper AsyncTabSwitcher issues.
> 
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> > I can try moving it into the "select" listener if you'd prefer it there.
> 
> Certainly would be nice to get this cleaned up or at least understand why
> things need to be set up this way.

Just confirmed locally it needs to happen before the select event fires or else the test fails. I'll see if I can figure out why.
(Assignee)

Comment 25

8 months ago
My best guess at the moment is that the async switcher causes some focus-related side effects such that when you click on an already-selected tab but something else is focused (another window or a popup notification), we still need to call it.

I can spend a bit more time looking into it - but I think this change is an improvement and could be landed on its own (no more XBL binding flip at runtime, more explicit about what code runs when)
(Reporter)

Comment 26

8 months ago
mozreview-review
Comment on attachment 8994363 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/258942/#review266110

Okay, let's file a followup.

::: browser/base/content/tabbrowser.js:4400
(Diff revision 1)
>          hist.add(2 /* unblockByVisitingTab */ );
>          tab.finishMediaBlockTimer();
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {

Can you please move this to the top of the _setupEventListeners method?

::: browser/base/content/tabbrowser.js:4402
(Diff revision 1)
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {
> +      if (event.target == this.tabpanels) {
> +        gBrowser.updateCurrentBrowser();

s/gBrowser/this/

::: browser/base/content/tabbrowser.js:4408
(Diff revision 1)
> +      }
> +    });
> +
> +    this.tabpanels.addEventListener("preselect", (event) => {
> +      if (gMultiProcessBrowser) {
> +        gBrowser._getSwitcher().requestTab(event.detail);

s/gBrowser/this/
Attachment #8994363 - Flags: review?(dao+bmo) → review+
(Reporter)

Updated

8 months ago
Assignee: nobody → bgrinstead
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

8 months ago
Attachment #8955461 - Attachment is obsolete: true
(Reporter)

Comment 27

8 months ago
mozreview-review
Comment on attachment 8994363 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

https://reviewboard.mozilla.org/r/258942/#review266114

::: browser/base/content/tabbrowser.js:4400
(Diff revision 1)
>          hist.add(2 /* unblockByVisitingTab */ );
>          tab.finishMediaBlockTimer();
>        }
>      });
> +
> +    this.tabpanels.addEventListener("select", (event) => {

another nit: the parentheses around event can go away
Comment hidden (mozreview-request)

Comment 29

8 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b30047d863c
Remove the tabbrowser-tabpanels binding. r=dao
(Assignee)

Updated

8 months ago
Blocks: 1478112

Comment 30

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b30047d863c
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
See Also: → bug 1469191
Depends on: 1469191
See Also: bug 1469191
status-firefox60: affected → wontfix
status-firefox61: --- → wontfix
status-firefox62: --- → wontfix
You need to log in before you can comment on or make changes to this bug.