1442582 - Remove the tabbrowser-tabpanels binding

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P2)

Comment 1

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ba0a2eef7c9cbebe81d74662281a9d72616a208

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1442582 - Remove the tabbrowser-tabpanels binding.

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

Comment 3

[Dão Gottwald [:dao]]

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

Comment 4

[Dão Gottwald [:dao]]

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

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

Comment 5

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfdcd27838acc7fdd5f5f76da4c3d2c3f589adc

Comment 6

[Dão Gottwald [:dao]]

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

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

Comment 7

[Dão Gottwald [:dao]]

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

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

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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

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

This is great, thank you!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d679c9a9afa0
Remove the tabbrowser-tabpanels binding. r=mconley

Comment 10

[Dão Gottwald [:dao]]

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

[Pulsebot]

Backout by [e-mail]:
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 12

[Dão Gottwald [:dao]]

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

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

Comment 13

[Dão Gottwald [:dao]]

I'm getting all kinds of failures that I didn't get last week:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=160d6494e22dfa09956a772d958e573d6097b7b6

Comment 14

[Dão Gottwald [:dao]]

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

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

Comment 15

[Dão Gottwald [:dao]]

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

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

Comment 16

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224630/diff/7-8/

Comment 17

[Dão Gottwald [:dao]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224630/diff/8-9/

Comment 18

[Brian Grinstead [:bgrins]]

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?

Comment 19

[Dão Gottwald [:dao]]

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.

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: Bug 1442582 - Remove the tabbrowser-tabpanels binding.

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

Comment 21

[Brian Grinstead [:bgrins]]

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.

Comment 22

[Brian Grinstead [:bgrins]]

I can try moving it into the "select" listener if you'd prefer it there.

Comment 23

[Dão Gottwald [:dao]]

(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.

Comment 24

[Brian Grinstead [:bgrins]]

(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.

Comment 25

[Brian Grinstead [:bgrins]]

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)

Comment 26

[Dão Gottwald [:dao]]

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/

Comment 27

[Dão Gottwald [:dao]]

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 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8994363 [details]
Bug 1442582 - Remove the tabbrowser-tabpanels binding.

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b30047d863c
Remove the tabbrowser-tabpanels binding. r=dao

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6b30047d863c