72440 - "menulist menu > menupopup" does not use the same style as "menulist > menupopup"

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:
1) Open mailnews
2) Go to Preferences -> Account settings
3) Go to "Copies and Folders" for some account
4) Open the "Place a copy in:" menulist
5) Mouse over one of the options with an arrow next to it.

ACTUAL RESULTS:
The second-level popup is a different background color from the top-level one.

EXPECTED RESULTS:
The colors should be the same.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK.  This can be solved by changing the style rule for "menulist > menupopup" to
also apply to "menulist > menupopup > menu menupopup" (that seems to be specific
enough not to incur much of a performance hit..).  Hewitt, what do you think?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix this in modern/classic/blue

Comment 3

[HJ]

Confirming that this path indeed does the trick. Thanks Boris.

Comment 4

[Boris Zbarsky [:bzbarsky]]

One thing.  It may be cleaner to just collapse 

menulist > menupopup, 
menulist > menupopup > menu menupopup

into just 

menulist menupopup

I'm not sure whether this entails a perf hit or not, however....

Comment 5

[Joe Hewitt (gone)]

In general, we try to avoid using the descendant combinator (e.g. "menulist 
menu") because its not very efficient.  The presence of such a selector means 
that every time style is resolved on a menu, the style system will have to walk 
the tree all the way up to the root, or until it finds a menulist ancestor.  
This is sloooow.

While this patch is definitely a correct fix, I'd like to avoid it.  In those 
rare cases where menulists contain menus (like in mailnews), it's more 
efficient to slap some sort of class on there, like "menulist-popup" or 
something like that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: patch using .menulist-menupopup and also fixing the mailnews XUL to deal

Comment 7

[Boris Zbarsky [:bzbarsky]]

HJ, try this one?  use <menupopup class="menulist-menupopup"> in your XUL for
the useragent thing for the <menupopup>s in <menu>s....

Hewitt, how does that look?

Comment 8

[Joe Hewitt (gone)]

Patch looks good.  Only two comments:

1. you can remove the class="push" from the menulists
2. on those comma-separated css selectors, put each item after the command on a
new line

do that and you have sr=hewitt

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... as I was checking where all those menus/menulists were overlaid into (to
make sure that I did not change something that was just a menu into
menulist-look), I found that the new folder dialog _did_ use just a menu where a
menulist would be much more appropriate.  Attaching two versions of the patch.
Both implement items 1 and 2.  The first one also changes new folder to use
<menulist>, while the second does not.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: update patch version 1 -- changes the new folder dialog to use <menulist>

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: updated patch version 2 -- do not touch the new folder dialog

Comment 12

[Boris Zbarsky [:bzbarsky]]

Come to think of it, the new folder dialog should be a separate bug.  So
disregard version 1.  Timeless, version 2 has sr=.  care to r=?

Comment 13

[timeless]

r=timeless. feel free to aim for 80 col boundary

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ok... since I have to merge in Blake's changes anyway, should I go ahead and fix
all the whitespace in this file?  And wrap at 80 columns as I do it?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: patch with blake's XUL changes

Comment 16

[timeless]

these don't seem to line up...
@@ -265,7 +276,7 @@
@@ -281,7 +292,7 @@
r=timeless (you don't need an r= to change the whitespace for the previous 
blocks)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: patch with that whitespace fix

Comment 18

[HJ]

Ok, and thanks Boris,
What do i need to change in the css file?

Comment 19

[Boris Zbarsky [:bzbarsky]]

HJ, you should just make sure you put 'class="menulist-menupop"' on all the 
second-level menulists you have (the ones inside <menu> elements).  Then once 
this patch is checked in they will have the right color.

There is no need for you to edit CSS.

Hewitt, could you sr the last patch?

Comment 20

[Joe Hewitt (gone)]

sr=hewitt

Comment 21

[Peter "jag" Annema]

Checked in for Boris, marking fixed.

Comment 22

[HJ]

Thanks Boris, it seems to work!

Comment 23

[Patty Mac]

Marking verified for all platforms (2001-04-16-04-Mtrunk)

Comment 24

[Patty Mac]

Change the status to "verified".