Closed Bug 289625 Opened 19 years ago Closed 19 years ago

color pickers in mail compose don't mirror focused color

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: allltaken, Assigned: mkaply)

References

Details

(Keywords: fixed1.8)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050407
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050407

The text and background color pickers set the colors ok, but the color picker
boxes on the formatting toolbar don't change to reflect the colors that have
focus. This bug evidently occurred in the past few days, since the April 2 nightly. 


Reproducible: Always

Steps to Reproduce:
1. Open a mail compose window
2. Set text color
3. Set background color

Actual Results:  
The colors of the picker boxes don't change, not even after moving the cursor to
the body area and starting to compose. The text color can be changed, you can
type in a new color, move the cursor back to the area of the first color and
type in the first color, but the picker box doesn't change. In some
circumstances I haven't determined, the text color gets set to the color of the
toolbar (grey in this case).


Expected Results:  
The color picker boxes should reflect the colors that have focus.
I should have said, "The text picker box gets set to the toolbar color," not the
text color. Sorry.
I found the following by trying a few saved builds.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.6) Gecko/20050315
has working color picker that matches color settings

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050217
color picker doesn't match color settings.

I should have seen this in 1.8x sooner.
Yes, this color picker bug is reproducible for me with Mozilla 1.8b1, but not
with Mozilla 1.7.7RC (or Thunderbird 1.0.2), so it seems this is a regression. 
BTW, I don't think this belongs in the Moz. App. Suite / Composer component,
maybe something like Core / MailNews:Composition would be better?
I wasn't sure how to categorize it. The picker failure is in the html composer
window (I just checked) as well as the mail/news editor window, so a core editor
category sounds good to me. I'm not an expert on component categories.
(In reply to comment #0)
> In some circumstances I haven't determined, the text picker box gets set to 
> the color of the toolbar (grey in this case).
 
When I select text with two different text colors the text picker box switches
to transparent (this seems logical) and I get this error in the JS console:

Error: Expected color but found 'mixed'.  Error in parsing value for property
'background-color'.  Declaration dropped.
Source File: chrome://editor/content/editor.xul
Line: 0

The box doesn't switch back to the right (or wrong) color when I deselect the text.

(In reply to comment #4)
> The picker failure is in the html composer
> window (I just checked) as well as the mail/news editor window

You're right, the color picker colors are also not updated in Composer. So maybe
Core:Editor (see http://www.mozilla.org/editor/) would be the right place, but I
am no expert either...
Assignee: composer → mozeditor
Component: Composer → Editor
Product: Mozilla Application Suite → Core
QA Contact: bugzilla
Version: unspecified → Trunk
Can you use builds on http://archive.mozilla.org to narrow down when this broke?

Note that this isn't really the "right" component for this bug, but we have no
"right" one.... :(

Requesting blocking-aviary, since presumably tbird has the same issue...
Flags: blocking-aviary1.1?
I'm on dialup so downloading takes time (40 minutes or so). I could do it if
nobody else has local files or broadband, but it would take a while.
The picker problem is in Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4)
Gecko/20040927. 1.8a1 should be the next one to try.

All the 1.7 versions seem to have good color pickers.
I have local builds and can try to narrow down, but I'll need detailed
instructions on how to reproduce (of the "click here, then click here, then
click here" sort) -- comment 0 isn't enough to really sort out what's going on
for someone who's never used the HTML mail compose...
My settings: In Mail-News composition settings, set "compose in html". In
Preferences, I've got "send in both html and plain text set." The latter
shouldn't make any difference as far as composing is concerned.

Then open a mail compose window or Composer window. Click on the body area, and
then try to set the background and/or text color. You don't need to fill in the
address, etc first.

Then see whether the background picker changes, and whether the text picker
mirrors the color of the text where the cursor is.

The same picker behavior seems to be used in both mail-compose and Composer.
> then try to set the background and/or text color

Steps?

> and whether the text picker mirrors the color of the text where the cursor is.

So I should move the cursor around?  Or what?
Click on the body area to make that the focus. Then click on the little white
box to bring up a color selection menu for the background.

Click on the little black box that's offset from the white one to set the text
color from the color selection menu that's evoked as previously.

Then type something, note that the color has been selected but is not mirrored
in the text-color box (the offset black one), and the background color isn't
either. The background picker box remains white---unless you get lucky and find
a version that works. :-)
Ok, to be really precise, the color picker boxes are on the Text Formatting
toolbar between the font selector and the left A+ symbol that reduces the font size.
In Mozilla 1.8a1 for Win32, the color pickers seemed to work about right. I
think they may not have mirrored the active colors until the user started
typing, but at least then they did. So it broke sometime between 1.8a1 and 1.8a4.
Blocks: 206716
This is a regression from bug 206716.  See bug 206716 comment 25 for what I
suspect the problem is.
Assignee: mozeditor → mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b3?
[The following comment is also posted in bug 206716 which is currently resolved
fixed.  I am putting it here because I'm guessing that the fix/patch will be in
this bug.]


(Disclaimer: I don't have a tree to test this theory right now)
The problem with ComposerCommands.js is that goUpdateCommandState() can call
pokeMultiStateUI() and cmdParams may have a cstring or a string.

doStatefulCommand() also calls pokeMultiStateUI() but cmdParams is always
consistent there.

One solution would be to make all commands pass the same string type (get rid of
all cstrings).

Another option is to make pokeMultiStateUI() able to handle either type (instead
of calling getStringValue() it could call getValueType() and then call the
appropriate string getter method).
OS: Windows 98 → All
Hardware: PC → All
> One solution would be to make all commands pass the same string type

This is probably the better long-term solution, imo.

> Another option is to make pokeMultiStateUI() able to handle either type

This is probably safer short-term....
Suggestion as to what the resulting behavior should look like: It would look
better to the user for color settings including updates of the pickers to go
into effect immediately instead of waiting for chars to be typed, as occurred
before the breakage.
Attached patch Use getvaluetypeSplinter Review
Minimum patch - use GetValueType to decide what type to get.
Attachment #183832 - Flags: review?(brade)
Comment on attachment 183832 [details] [diff] [review]
Use getvaluetype

The patch is fine (too bad the enum types don't better match the api:
eStringType for getCStringValue and eWStringType for getStringValue).

Please change the curly braces to match the rest of the file:
 * only use curly braces when more than one line
 * open curly braces go on their own line

Thanks!
Attachment #183832 - Flags: review?(brade) → review+
Do you think my logic is correct? default to wide string unless it is a CString?
Or should I add a wide string check and default to something else?
I think the logic is fine.  I'm not really sure what the appropriate
behavior/response should be if a command comes through that doesn't return a
string.  I believe what happens today with this patch is fine.  It wouldn't hurt
to do the right thing (do the explicit check for wstring) but I'm not sure it's
worth checking for all of the types.  Maybe add a comment?
ping? is this patch ready to land?
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Attachment #183832 - Flags: superreview?(tor)
The patch is waiting for sr...
Flags: blocking-aviary1.1?
Flags: blocking1.8b4? → blocking1.8b4+
Attachment #183832 - Flags: superreview?(tor) → superreview+
Attachment #183832 - Flags: approval1.8b4?
Attachment #183832 - Flags: approval1.8b4? → approval1.8b4+
on the branch. waiting for trunk to open.
Keywords: fixed1.8
Fix checked in to trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
In Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050825
SeaMonkey/1.0a, the text picker is updated immediately on selecting a color, but
the background picker isn't updated until chars are typed.
Is that a regression from after this bug was fixed, or has it been a problem all
along?
The delayed effectiveness of a color setting was there all along, at least in
previous months and perhaps much longer. See my comment #19.
Probably worth filing a separate bug on...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: