77873 - Fix color button XUL in Composer

Status: VERIFIED FIXED

(SeaMonkey :: Composer, enhancement, P3)

Description

[[e-mail]]

I think that buttons should have titles on them.
The Last Picked Colour button is an example of how to do this.
I think that the other colour buttons should also look like this.
The page colour buttons look particularly odd when they are disabled.

Comment 1

[kinmoz]

over to cmanske

Comment 2

[Charles Manske]

What buttons? Toolbar? dialogs?

Comment 3

[[e-mail]]

I was particularly thinking of the page colour dialog but it's not the only one.

Comment 4

[Charles Manske]

In the Page Properties dialog, the colors on the buttons should always show the
actual page colors when enabled (when "Use Custom Colors" is checked).
When "Reader's default colors" is checked, the color buttons should be disabled,
and since there's no text on them, removing the "color swatch" seems logical.
Kathy, Simon: What do you think?

Comment 5

[Kathleen :Brade]

I think that all color swatches that bring up the color picker/palette should 
have a drop-down triange.  That would be a "theme" issue?

Comment 6

[Charles Manske]

I agree. I actually tried to do that, but it displays poorly in 6.0.
Yes, that would be a themes issue.
I think Neil's suggestion to incoporate the text into the button is interesting,
but I'm inclinded to leave it for next revision. Too many other bugs to fix
for moz1.0/NS6.5!

Comment 7

[[e-mail]]

Attachment: Initial removal of stacks

Comment 8

[[e-mail]]

You may want to move the labels inside the buttons to match the last colour.

Comment 9

[Charles Manske]

Your simplifications of the buttons are good, but I am seeing some drawing
problems in the new buttons: the spring that is the "color-well" seems to be wider 
than the parent button. I think we need to play with the CSS padding, borders, 
etc. in Composer's themes files to make this look right.
Note that there are other bugs and discussions to try to unify the color-picker 
buttons used by Browser and Composer, so we probably should a consult a wider 
audience before finishing this. I think putting the text on the button, as in
the "last-picked" button my not be an acceptable for all users. But your 
changes from the "stack" XUL constructions are still good to do.

Comment 10

[Charles Manske]

Also, in any future revisions of the patch, don't include 'class="dialog"' in
and of the buttons -- see bug 95526.

Comment 11

[[e-mail]]

I had a look at the Modern theme and I got some nice looking buttons by
completely removing the button.color-button styles (in EditorDialogs.css).

However I was unable to get the colour picker dialog to open, so I don't know
what the last-picked colour looks like.

I was unable to look at the buttons at all in the Classic theme. Perhaps I need
a more recent build of Mozilla.

Comment 12

[[e-mail]]

Even after taking the margins off the color wells I had to make the buttons a
little wider but I think it works quite well.

Index: EditorDialog.css
===================================================================
RCS file: /cvsroot/mozilla/themes/modern/editor/EditorDialog.css,v
retrieving revision 1.29
diff -u -r1.29 EditorDialog.css
--- EditorDialog.css    2001/08/04 06:20:05     1.29
+++ EditorDialog.css    2001/08/24 13:21:57
@@ -164,12 +164,11 @@

 button.color-button {
   margin: 2px;
-  width: 32px;
-  min-width: 32px;
+  width: 42px;
+  min-width: 42px;
 }

 .color-well {
-  margin: 4px 10px 4px 10px;
   border: 1px inset #B4C3D4;
   width: 20px;
   height: 12px;

Index: EditorDialog.css
===================================================================
RCS file: /cvsroot/mozilla/themes/classic/editor/EditorDialog.css,v
retrieving revision 1.17
diff -u -r1.17 EditorDialog.css
--- EditorDialog.css    2001/08/04 06:19:31     1.17
+++ EditorDialog.css    2001/08/24 13:22:21
@@ -86,7 +86,6 @@
   width: 20px;
   height: 12px;
   border: 1px inset #CCCCCC;
-  margin: 2px;
 }

 .color-well[default="true"] {
@@ -96,9 +95,8 @@

 button.color-button {
   /* need to override large default min-width */
-  min-width : 30px;
-  width : 30px;
-  height: 16px;
+  min-width : 40px;
+  width : 40px;
   margin: 2px;
 }

Comment 13

[Charles Manske]

We should do this relatively soon. Changing Summary to better describe the task.
Niel: Your basic XUL work to replace the current "stack" XUL is good, but do 
we agree that we shouldn't put the text associated with the button on the 
button itself (as we do for "Last Picked Color")?
The "Last Picked Color" design will change with my planned simplification of
the ColorPicker dialog, which will become a menulist popup.

Comment 14

[Christian Reis]

In my opinion, the labels on the buttons make for easier clicking and a more
pleasant appearance, and don't distract from the general idea. I guess somebody
is going to say the button doesn't suggest choosing a new colour on it's label
and as such is a bad idea.

(For one, mpt probably disagrees.)

Comment 15

[Charles Manske]

Generally, having the text describing the purpose of the color yields very wide
buttons. We use it only for that one instance in Composer's color picker dialog.
We intend to make this color picker more like the Browser's, so unless we change 
the policy of "text next to color button" to "text on color button" across all
buttons, the "text next to color button" will prevail.

Comment 16

[[e-mail]]

Attachment: Combined patch [also updated for bitrot]

Comment 17

[Syd Logan]

Charles, you need a review for this?

Comment 18

[Syd Logan]

spam composer change

Comment 19

[Charles Manske]

Neil did the work. I will be testing and reviewing it for 0.9.5

Comment 20

[Charles Manske]

Neil: I applied your patch. The button looks fine in the Color Picker, but I'm
still having problems in the other dialogs. I'll attach an image to show what
I'm seeing in Modern theme.

Comment 21

[Charles Manske]

Attachment: Example of button problem.

Comment 22

[[e-mail]]

Attachment: What I get

Comment 23

[[e-mail]]

Attachment: <spring> changed into <spacer>

Comment 24

[[e-mail]]

Comment on attachment 50064 [details] [diff] [review]
<spring> changed into <spacer>

Forgot the skins again :-(

Comment 25

[[e-mail]]

Attachment: <spring> changed into <spacer>

Comment 26

[Charles Manske]

I don't think the <spring> needs to be changed to <spacer>
The problem I see with the buttons is related to the CSS width being "42"
I measured the width of the buttons on my display and they were 42, which seems
to wide. Neil: I think your might have some DPI preference set so your buttons
don't look as wide?
It also seems that the default CSS for "button" (e.g, "min-width: 6em") is 
causing us problems. I'm working on better CSS now.

Comment 27

[Charles Manske]

Attachment: Updated patch with CSS that seems to work

Comment 28

[Charles Manske]

My patch on 9/21 is essentially the same as Neil's on 9/06 (updated for "bitrot")
but with new simpler CSS that seems to work for Modern. Unfortunately, I can't 
test Classic today. This new CSS simply reduces the min-width on the outer 
button and lets it size as close as possible around the inner 'color-well'
(that shows the color); a spring is a good element to use for that, as Neil 
originally suggested.
r=cmanske on all dialog changes. Neil should test and review my 
suggested CSS changes.

Comment 29

[[e-mail]]

Attachment: patch updated as per bug 99876

Comment 30

[Charles Manske]

Comment on attachment 50511 [details] [diff] [review]
patch updated as per bug 99876

Oh, I see now! I wasn't aware that "spacer" was replacing "spring".
Neil: So the CSS I suggested works for you?

Comment 31

[[e-mail]]

The CSS looks good. Sorry I wasn't able to test the CSS earlier.

Comment 32

[Joe Hewitt (gone)]

sr=hewitt

Comment 33

[Charles Manske]

checked in.

Comment 34

[James Baxter]

Verified in 10-01 Trunk build