Closed Bug 1177370 Opened 5 years ago Closed 4 years ago

No TabAttrModified event when visuallyselected attribute changes

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 1 obsolete file)

I need to know when a tab is visually selected rather than just selected, but TabAttrModified doesn't fire then.
Attached patch 1177370-1.diff (obsolete) — Splinter Review
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8626086 - Flags: review?(ttaubert)
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.
Attachment #8626086 - Flags: review?(ttaubert) → review-
Except it is an attribute, that's what all the CSS hangs from.
What do you think, George?
Flags: needinfo?(gwright)
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.
Flags: needinfo?(gwright)
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).
Needinfoing as per IRC.
Flags: needinfo?(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.
Attachment #8626086 - Flags: review-
Attached patch 1177370-2.diffSplinter Review
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.
Attachment #8626086 - Attachment is obsolete: true
Attachment #8636003 - Flags: review?(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. :/
Attachment #8636003 - Flags: review?(mconley) → review+
Tomcat, apparently none of the bugs from https://hg.mozilla.org/mozilla-central/rev/1f77b78797d6 got marked as fixed.
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/4c5da00ecca1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1187219
(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 :)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.