407931 - Switch from buttonstyle to mode

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Toolkit uses toolbar[mode="icons"] instead of *[buttonstyle="pictures"] and similar changes are needed for text-only toolbars.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Obviously I've only tried this out on Windows...

Comment 2

[jag (Peter Annema)]

Comment on attachment 292618 [details] [diff] [review]
Proposed patch

Index: suite/themes/modern/jar.mn

This change is part of a different bug, right?

Also, why are you removing the #nav-bar-buttons bits?

Comment 3

[Philip Chee]

(In reply to comment #2)
> Index: suite/themes/modern/jar.mn
> This change is part of a different bug, right?
Bug 240393

> Also, why are you removing the #nav-bar-buttons bits?
Bug 407744 removed the #nav-bar-buttons

Comment 4

[[e-mail]]

(In reply to comment #3)
> (In reply to comment #2)
> > Also, why are you removing the #nav-bar-buttons bits?
> Bug 407744 removed the #nav-bar-buttons
That change was supposed to be part of this patch. It's needed for bug 394288. I must have edited that out instead of the jar.mn change by mistake.

Comment 5

[Philip Chee]

Oops I confused toolbar_button_box with nav-bar-buttons

Comment 6

[[e-mail]]

Attachment: Updated patch

OK, this version should have all the missing bits.

Comment 7

[Philip Chee]

> +toolbar[mode="text"] > .toolbarbutton-1 > ....

[18:13:10] <stefanh> NeilAway: the child selectors
[18:36:59] <NeilAway> stefanh: also the worst that can happen is that during customisation you see both icons and text

Neil, I think that this is unacceptable. For example if the toolbar is currently in smallicon+icononly mode and you enter customization, the toolbar buttons will become large, acquire text labels and the padding and margins will change. I find this rather unpleasant. In Minefield, the iconsize/mode of the toolbar buttons do not change when customization is entered.

I hope that you will reconsider and make the first child selector a descendant selector instead e.g.:

toolbar[mode="text"]  .toolbarbutton-1 > ....

I thought of adding additional rules such as:
toolbar[mode="text"] > * >  .toolbarbutton-1 > ....

But decided that this would be too unwieldy. In addition it is theoretically possible that an extension could do something like:

<toolbaritem>
  <toolbarbutton .../>
  <toolbarbutton .../>
</toolbaritem>

So descendant selectors please? I shall be in your debt forever (or until next Thursday, whichever comes first).

Comment 8

[Stefan [:stefanh]]

Comment on attachment 292958 [details] [diff] [review]
Updated patch

I can't make this work on mac, no matter how I try I can't change the pref.

Comment 9

[Stefan [:stefanh]]

(In reply to comment #8)
> (From update of attachment 292958 [details] [diff] [review])
> I can't make this work on mac, no matter how I try I can't change the pref.
> 
Uh, forget about it (old tree + missed a few bits from the patch)

Comment 10

[[e-mail]]

Attachment: Alternative version

This version uses descendent selectors so it should be a bit smaller.

Comment 11

[Philip Chee]

(In reply to comment #10)
> This version uses descendent selectors so it should be a bit smaller.
Thanks for that Neil.

> +                       chromeclass="chromeclass-toolbar-additional"
Sorry for sounding dense, but how does chromeclass="foo" work? I've looked for css rules that use this, and for inherit directives in xbl bindings and failed to find anything relevant. In fact I can't find anything in LXR that uses |chromeclass=| .

Comment 12

[[e-mail]]

Comment on attachment 293331 [details] [diff] [review]
Alternative version

It occurs to me that switching to descendant selectors deserves a new superreview.

Comment 13

[[e-mail]]

(In reply to comment #11)
>Sorry for sounding dense, but how does chromeclass="foo" work?
http://mxr.mozilla.org/seamonkey/source/toolkit/content/xul.css#40

Comment 14

[[e-mail]]

Attachment: Fixed version

jag pointed out what Ratty meant...

Comment 15

[Ian Neal]

Comment on attachment 293384 [details] [diff] [review]
Fixed version

Looks good to me and not spotted in problems in use over the last few days.

Comment 16

[[e-mail]]

Fix checked in.