163481 - Color picker not dynamically updating color selection icon

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Tracy Walker [:tracy]]

seen on commercial trunk builds:

windows 2002-08-19-04-trunk
linux 2002-08-29-05-trunk
mac os9 2002-08-19-03-trunk

-open composer
-type some text then highlight it.
-click on the color picker icon 
(notice the colors don't update 'til mouse over)
-in the color picker dialog select a color
(notice that the choice you just made is not shown in the color icon, however
the text field shows the updated value for the color, and the color is actually
selected)
-click "Okay"
the selected text is now the color chosen, but the color picker icon in the tool
bar is not updated
-mouse over the color picker icon and the colors change to what had been chosen.

Comment 1

[John "AntiTuX" Dee]

I did the steps, and on MacOSX, it just takes about 3 seconds to update the color.
Odd, though.

Comment 2

[sujay]

--> cmanske

Comment 3

[Charles Manske]

Something very fundamental is messed up! There are no changes in ColorPicker that
could cause this. We set the background color on the "color swatch" in both the 
toolbar and in the colorpicker using this CSS:
setAttribute("style",("background-color:"+color));
where "color" is the HTML color string.
This problem also exists for the color swatches in the Table Properties and
Page Properties dialog (color shows only after a mouse-over.)

I see a slightly different version of the problem using the color-picker in
the prefs: Appearance -> Colors): the color values in for the colors are not
initialized. If you click on one and change the color, it then appears in the 
color button. But if you switch to a different prefs panel, then switch back,
the colors are no longer displayed on the 4 color buttons.

I'm not sure if this is a CSS or widget problem -- assigning to widgets first.
It occurs in both Modern and Classic themes.

Comment 4

[Charles Manske]

I think the changes to attribute handling for bug 156971 are suspect.
Some more information:
Bring up the Composer ColorPicker by clicking on the toolbar button.
The Color swatch to the right of the color input field is a <spacer> element;
we set the "background-color" to the user's selected color by setting a "style"
attribute on this element. Note that if you click on the "Default" button,
the background color is set to "inherit" and the change shows up immediately.
The next color you click on in the colorpicker will display correctly.
But the next one after that will not. So when a color is already set, changing
it is not triggering the redraw of the spacer element.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm having trouble reproducing this on Linux.  I think I might have managed to
reproduce the problem mentioned in comment 4 about the change after using the
"Default" button once, but it might have just been because I didn't really click
the mouse, and I couldn't reproduce it again.

Comment 6

[Charles Manske]

David: So every time you click on a color in the colorpicker, you see that 
color appear on the color swatch to the right of the input field?
I definitely don't see that on Windows.
Do you see the toolbar symptoms? Type some text, select some, change the color
using the colorpicker. Then move caret into the colored area by clicking in it
or with arrow keys. The text color should update in the toolbar swatch. I don't
see it do that until I move the mouse over it.

Note that I tried to backout to the file versions just before your patch to 
bug 156971, but the resulting build crashes during startup :(.
We did test the morning build from 8/16 and it doesn't show this bug, so I think
your changes are still the most suspicious!

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmm.  I can see it in a release build, but I don't see it in my own tree.  I'll
diff my tree and see if there's anything I missed checking in...

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It also could have something to do with the MOZ_SVG ifdef in
nsCSSFrameConstructor::AttributeChanged.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This fixes it, although I'm not crazy about the patch.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: alternative patch

I might like this one better.  (I'm not really sure, though.)

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 96102 [details] [diff] [review]
alternative patch

r=bbaetz. This patch better reflects whats actually happening

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking.

Comment 13

[Charles Manske]

"alternative patch" tested for all cases described above and it works fine.
If one of you wants to explain what was the problem or why the patch fixes it,
that would be appreciated. :)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem was that I was testing using an SVG-enabled build, and there was
that little MOZ_SVG ifdef that made sure we always knew the style attribute
affected style.  That needs to happen for XUL too.  (We don't use the
AttributeAffectsStyle optimization for HTML, so HTML is OK.)

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 96102 [details] [diff] [review]
alternative patch

sr=bryner

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in, 2002-08-21 17:01 PDT.

Comment 17

[Tracy Walker [:tracy]]

verified fixed as seen on commercial trunk builds:

windows 2002-08-23-04-trunk
linux 2002-08-23-05-trunk
mac os9 2002-08-23-03-trunk