421351 - allow styling richlistitems with -moz-appearance:menuitem

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

Vista uses menuItem styling in their dropdown and richlistbox-like UIs. Bug 420237 asks for the location bar to use this type of styling on Vista, and a similar one will soon as for that styling to be used in the Download and Add-ons Managers.

Right now the focus/hover events aren't being sent in a way that allows this. Vlad's first stab at changing that caused bug 420325 and will be reversed, but we need to figure out the right way to do it.

Enn: can you help?

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Some background: because menus have to handle both hover and keyboard movement, there is a 'mozmenuactive' attribute that is set on the menu item that's currently the actively highlighted one.  That doesn't work inside the autocomplete popup, since it's not a real menu (it's a richlistbox inside a panel).  What would be the best way to make this work?

We could make the native theme code check whether the frame is a menu frame and only use mozmenuactive in that case, otherwise check hover; but I think that doing so wouldn't let you move the highlight with the cursor keys.  I don't really want to add a special hack in here for richlistboxes in the native theme code, but we don't have any way of saying "draw this like a hovered menuitem" in CSS...

Comment 2

[(not currently active) Ted Mielczarek]

I don't think we should block on this.

Comment 3

[Recall]

Mike, I found something that might help on a google search after seeing this bug

http://icant.co.uk/forreview/tamingselect/

It mentions drawing the selection boxes with javascript, which then allows you to use custom css for styling. 

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Comment 5

[Neil Deakin]

Comment on attachment 381450 [details] [diff] [review]
patch

I suppose this will have to do for now. We can revisit this during bug 390647.

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/180192a5c56f

Comment 7

[[e-mail]]

Why not apply this just to autocomplete richlistitems?

Comment 8

[Dão Gottwald [:dao]]

Because we want it for all richlistboxes, see bug 426727.

Comment 9

[Neil Deakin]

Comment on attachment 381450 [details] [diff] [review]
patch

(In reply to comment #8)
> Because we want it for all richlistboxes, see bug 426727.

If that's the case, then I retract my review. I thought this was a workaround needed just for the autocomplete widget. richlistitems in general shouldn't be using moz-menuactive nor should they be using -moz-appearance: menuitem.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> nor should they be using -moz-appearance: menuitem.

Bug 426727 limits this to very specific conditions. Have you actually looked at that patch and found it problematic?

Comment 11

[Neil Deakin]

Well I'm a bit confused here. Are you thinking that this applies only to autocomplete richlistitems, richlistitems in 'very specific conditions' or 'all' richlistitems?

For reference, my logic is that there are no richlistitems that are menuitems, thus there are no richlistitems that should be handled as such.

Comment 12

[Dão Gottwald [:dao]]

The specific conditions are Windows Vista or 7 using the default theme, where it would apply to all richlistitems. The menuitem appearance is very close if not identical to the styling of items in the Windows Explorer under these circumstances. I think it just lacks a few non-critical states.

Comment 13

[Philip Chee]

Err guys. This bug is marked fixed and the patch has been pushed but it has a r-. Would it be OK to unresolve this bug?

Comment 14

[Neil Deakin]

I don't think it would be a good idea to add a workaround that would have to be changed anyway when 390647 is implemented.

Is seems like a better workaround would be to just change nsNativeThemeWin::IsMenuActive to just check for the selected attribute if the tag was richlistitem.

The patch here only changes the appearance once one has selected the item, yet the initial bug report mentions hovering. The list view in Vista explorer shows the translucent blue on the selected item as well as the hovered item, but maybe we're trying to emulate some other UI?

> I think it just lacks a few non-critical states.

Which states are these?

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> The patch here only changes the appearance once one has selected the item, yet
> the initial bug report mentions hovering.

Hovering selects the item in the autocomplete case.

> > I think it just lacks a few non-critical states.
> 
> Which states are these?

The hovered-but-not-selected state, which I think is expected to be very faint. There may also be differences when the richlistbox isn't focused or when selecting multiple items, but I can't test it on Vista right now.

Comment 16

[Dão Gottwald [:dao]]

Attachment: alternative approach

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 389286 [details] [diff] [review]
alternative approach

>+  nsIContent* content = aFrame->GetContent();
>+  if (content && content->IsNodeOfType(nsINode::eXUL)) {

content is guaranteed to be non-null here.

>+    nsIAtom *tag = content->Tag();
>+    nsCOMPtr<nsIAtom> richlistitemAttom = do_GetAtom("richlistitem");
>+    if (tag == richlistitemAttom)

You can just add richlistitem to nsWidgetAtomList.h and do
if (content->Tag() == nsWidgetAtoms::richlistitem)

Comment 18

[[e-mail]]

Comment on attachment 389286 [details] [diff] [review]
alternative approach

Or maybe you should just check aFrame->GetType() != nsWidgetAtoms::menuFrame?

Comment 19

[Dão Gottwald [:dao]]

aFrame->GetType() != nsWidgetAtoms::menuFrame seems to be too restrictive. When I do that, menu arrows, checkmarks and bullets don't change their color anymore.

Comment 20

[[e-mail]]

Comment on attachment 389286 [details] [diff] [review]
alternative approach

Ah yes, arrows aren't menu frames. OK, so in that case r=me with mstange's nit fixed and using NodeInfo()->Equals to do the test.

Comment 21

[Markus Stange [:mstange]]

(In reply to comment #20)
> and using NodeInfo()->Equals to do the test.

Just out of curiosity, what's the advantage of that? "Tag() ==" is shorter and clearer (and also what the rest of the file uses).

Comment 22

[Dão Gottwald [:dao]]

Attachment: for check-in

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f353d2b637b3