1177370 - No TabAttrModified event when visuallyselected attribute changes

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Geoff Lankow (:darktrojan)]

I need to know when a tab is visually selected rather than just selected, but TabAttrModified doesn't fire then.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1177370-1.diff

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8626086 [details] [diff] [review]
1177370-1.diff

Review of attachment 8626086 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't an attribute, it's a property tacked onto the tab.

Comment 3

[Geoff Lankow (:darktrojan)]

Except it is an attribute, that's what all the CSS hangs from.

Comment 4

[Geoff Lankow (:darktrojan)]

What do you think, George?

Comment 5

[George Wright (:gw280) (needinfo me!)]

It seems to me like you would need to fire TabAttrModified in the setter for _visuallySelected, but seeing as _selected doesn't have a similar TabAttrModified signal, I'm not sure why this is necessary.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Sorry, my bad, I see that TabAttrModified is fired in tabbrowser.xml at the callsites rather than in the setter. I guess that I don't have any opposition to Geoff's patch, but as I'm not a peer of this module I don't really have enough of an understanding to make a call (nor the authority).

Comment 7

[Geoff Lankow (:darktrojan)]

Needinfoing as per IRC.

Comment 8

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

Comment on attachment 8626086 [details] [diff] [review]
1177370-1.diff

Review of attachment 8626086 [details] [diff] [review]:
-----------------------------------------------------------------

I understand the need for this, but I'm not thrilled with sprinkling these kinds of event-firing things all over the place. As we iterate on tab switching and work on performance, it's possible that some of this logic will change, and this sort of thing might fall through the cracks.

A "better" solution that I suggest is that you override the _visuallySelected property on the tabbrowser-tab binding. Flesh it out with the code from tabbox.xml that does the same _visuallySelected work, and then at the end of the function, have it fire the tab attribute modified event.

It's not amazing, but I think it's more resilient.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1177370-2.diff

I don't really like the thought of having the same code in two places, but I don't see much choice. I could have it walk up the prototype chain to call the overridden function, but that's probably bad for performance and I can't see any precedent for doing so. I've stuck a warning on both places, just in case.

Comment 10

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

Comment on attachment 8636003 [details] [diff] [review]
1177370-2.diff

Review of attachment 8636003 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too jazzed about it either, but well, that's XBL - no calling into the parent class. :/

Comment 11

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

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

Comment 12

[Geoff Lankow (:darktrojan)]

Try looks good! Pushed https://hg.mozilla.org/integration/fx-team/rev/4c5da00ecca1

Comment 13

[Guilherme Lima]

Tomcat, apparently none of the bugs from https://hg.mozilla.org/mozilla-central/rev/1f77b78797d6 got marked as fixed.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c5da00ecca1

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Guilherme Lima from comment #13)
> Tomcat, apparently none of the bugs from
> https://hg.mozilla.org/mozilla-central/rev/1f77b78797d6 got marked as fixed.

thanks! was running now the tool again and seems this worked this time :)