No TabAttrModified event when visuallyselected attribute changes

RESOLVED FIXED in Firefox 42

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Firefox 42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
I need to know when a tab is visually selected rather than just selected, but TabAttrModified doesn't fire then.
(Assignee)

Comment 1

3 years ago
Created attachment 8626086 [details] [diff] [review]
1177370-1.diff
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-
(Assignee)

Comment 3

3 years ago
Except it is an attribute, that's what all the CSS hangs from.
(Assignee)

Comment 4

3 years ago
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).
(Assignee)

Comment 7

3 years ago
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-
Flags: needinfo?(mconley)
(Assignee)

Comment 9

3 years ago
Created attachment 8636003 [details] [diff] [review]
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.
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+

Comment 13

3 years ago
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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Updated

3 years ago
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.