66354 - Pull-down menu text not visible after selecting an option

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[shrirang khanzode]

See on al ltrunk builds (20010123)

Steps :
1 Launch Composer
2 On the Composition Toolbar, click on the 'Image' button
3 Observe the 'Image Properties' window open up
4 Click on the 'More Properties' button 
5 In the pane titled 'Aligh text to image, click and select any option
6 Observe that the text color of the selection option does not change to white 
  and is black and is not visible under the blue background(with default color 
  settings)

Comment 1

[:deanis74]

I see this with the Classic skin, only.

Comment 2

[rubydoo123]

reassign to cmanske

Comment 3

[rubydoo123]

moving to mozilla0.9.1

Comment 4

[Charles Manske]

Attachment: Patch for fixes to 66354 and 66417 -- CSS changes

Comment 5

[Charles Manske]

Attachment: Editor changes needed to fix 66354

Comment 6

[Charles Manske]

Resetting milestone to 0.8 since fix is ready.
Beppe reviewed the slight changes I made to the image files to make the align
states more visible when selected.
Kathy and Simon: Can I please get reviews.

Comment 7

[Simon Fraser [no longer active]]

Have the Mac classic skin changes been tested?

Comment 8

[Simon Fraser [no longer active]]

The patches in this bug and bug 66417 contain some of the same changes. Which am 
I to reveiw?

Comment 9

[Charles Manske]

Both, please! Since I already requested review for 66417 at the same time,
I didn't want to try to disect the newer diff -- thought it might interfere with
applying the patch.
No to question about checking on Mac.
Is Kathy still out? Do you have any time to test the patch?

Comment 10

[Simon Fraser [no longer active]]

I meant which changes should I review for which bug? In general, it's best to 
keep patches discrete.

sr=sfraser

Comment 11

[Blake Ross]

r=blake

Comment 12

[Charles Manske]

Text color is set to appropriate value (black for Modern, white for Classic)
to make it visible when the align menulist has the focus.
Images were tweaked so the top, middle, and bottom attributes show a bit more
clearly when selected.
There will always be a potential problem with visibility of selected text
because a user selects that color via OS settings. While most users will have a
dark colored selection (which this fix assumes), some may not. This problem
exists for all text in menulists, not just this example.

Comment 13

[Joe Hewitt (gone)]

I hate to reopen this bug, I but I see a much cleaner solution to the problem
than the one that was checked in.  cmanske e-mailed me about this bug earlier
in the week but I didn't have a chance to look closely at it then.

The menulist in question seems to be built in a somewhat overcomplicated way. 
There is a far simpler way to do it, the key being the use of the "data"
attribute on the menulist.  See the forthcoming patch.

Comment 14

[Joe Hewitt (gone)]

taking

Comment 15

[Joe Hewitt (gone)]

A point for future reference when changing colors in classic - don't use colors
like "white" -- classic uses the CSS2 system colors wherever possible.  In this
case, the correct color to use for the menulist text would be HighlightText.

Comment 16

[Joe Hewitt (gone)]

Attachment: patch to fix (css, js, and xul)

Comment 17

[Joe Hewitt (gone)]

cmanske and sfraser, can you review this again?

Comment 18

[Charles Manske]

r=cmanske

Comment 19

[Joe Hewitt (gone)]

hyatt sez sr=hyatt

Comment 20

[Joe Hewitt (gone)]

fixed

Comment 21

[sujay]

verified in 3/22 build.