Closed Bug 471921 Opened 11 years ago Closed 11 years ago

use proper CSS instead of first-tab, last-tab and afterselected attributes

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
We can use :first-of-type for [first-tab], :last-of-type for [last-tab] and [selected] + tab for [afterselected]. These CSS features weren't implemented or didn't even exist when the tab widgets were implemented.

Unfortunately we can't get rid of beforeselected (yet?), which leaves some complexity in tabbox.xml and tabbrowser.xml.
Attachment #355169 - Flags: review?(gavin.sharp)
Blocks: 471923
Comment on attachment 355169 [details] [diff] [review]
patch

>diff --git a/widget/src/xpwidgets/nsWidgetAtomList.h b/widget/src/xpwidgets/nsWidgetAtomList.h

>-WIDGET_ATOM(firsttab, "first-tab")

>-WIDGET_ATOM(lasttab, "last-tab")

Oh, thanks for noticing that.
No longer blocks: 471923
Attached patch patch (obsolete) — Splinter Review
updated to trunk
Attachment #355169 - Attachment is obsolete: true
Attachment #355286 - Flags: review?(gavin.sharp)
Attachment #355169 - Flags: review?(gavin.sharp)
we should get this into 1.9.1 so that theme authors don't copy that stuff anymore
Attachment #355286 - Attachment is obsolete: true
Attachment #355943 - Flags: review?(gavin.sharp)
Attachment #355286 - Flags: review?(gavin.sharp)
Attached patch stop setting the attributes (obsolete) — Splinter Review
this can land later (moz2?)
Attachment #355944 - Flags: review?(gavin.sharp)
this illustrates an rtl styling bug that attachment 355943 [details] [diff] [review] fixes
Attachment #355943 - Flags: review?(gavin.sharp) → review+
Comment on attachment 355943 [details] [diff] [review]
stop using the attributes (checked in)

http://hg.mozilla.org/mozilla-central/rev/7e5854c047bd
Attachment #355943 - Attachment description: stop using the attributes → stop using the attributes (checked in)
Attachment #355943 - Flags: approval1.9.1?
Blocks: 456757
(In reply to comment #3)
> Created an attachment (id=355943) [details]
> stop using the attributes
> 
> we should get this into 1.9.1 so that theme authors don't copy that stuff
> anymore

This patch still has potential impact on theme and extension developers since it changes the behaviour of the selected attribute. If it lands on 1.9.1 it should do so before b3
Keywords: late-compat
(In reply to comment #7)
> (In reply to comment #3)
> > Created an attachment (id=355943) [details] [details]
> > stop using the attributes
> > 
> > we should get this into 1.9.1 so that theme authors don't copy that stuff
> > anymore
> 
> This patch still has potential impact on theme and extension developers since
> it changes the behaviour of the selected attribute. If it lands on 1.9.1 it
> should do so before b3

Or that change can just be left out.
Attachment #356798 - Flags: approval1.9.1?
Attachment #355943 - Flags: approval1.9.1?
Keywords: late-compat
(In reply to comment #7)
> This patch still has potential impact on theme and extension developers since
> it changes the behaviour of the selected attribute.

I don't see any reason why anyone would care about the "false" case, so I decided I was fine with breaking it. I suppose the win isn't big enough to offset even a very-low-probability addon conflict, so I guess you're right that we should omit that change, but the idea of having to care about such a minor detail still bothers me!
(In reply to comment #10)
> (In reply to comment #7)
> > This patch still has potential impact on theme and extension developers since
> > it changes the behaviour of the selected attribute.
> 
> I don't see any reason why anyone would care about the "false" case, so I
> decided I was fine with breaking it. I suppose the win isn't big enough to
> offset even a very-low-probability addon conflict, so I guess you're right that
> we should omit that change, but the idea of having to care about such a minor
> detail still bothers me!

It can be used to detect whether a tab has ever been visited before. In fact I noticed it because it broke some userChrome styling I was using to achieve that very effect. I have seen extensions and themes that offer the same sort of thing and my guess is they use the attribute. I'm not against taking the change in 1.9.1, just saying that if we did so then it should be in for b3 to give themes and extensions a chance to react.
Ah, interesting. I guess the problem is really that I'm just not imaginative enough! :) Thanks for bringing it up.
I noticed the same that my code in userChrome.css in no longer working:
/* Change color of not selected inactive tabs */
.tabbrowser-tab:not([selected]) {
 color:#c09!important; font-style:normal!important;font-size:8pt!important; font-weight:bold!important;
}

I use it a lot to open many tabs at once (forum threads) to see which are still unread if read them one by one.
The only problem was that dragging a tab left or right or using undo closed tab caused all tabs to get a selected="false" attribute.
Blocks: 480813
Attachment #355944 - Attachment is obsolete: true
Attachment #355944 - Flags: review?(gavin.sharp)
Comment on attachment 355944 [details] [diff] [review]
stop setting the attributes

filed bug 480813 for this
Attachment #356798 - Attachment description: stop using the attributes, branch versio → stop using the attributes, branch version
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 356798 [details] [diff] [review]
stop using the attributes, branch version

a191=beltzner
Attachment #356798 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Blocks: 469584
You need to log in before you can comment on or make changes to this bug.