Closed Bug 421351 Opened 14 years ago Closed 13 years ago

allow styling richlistitems with -moz-appearance:menuitem

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: beltzner, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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?
Flags: wanted-next+
Flags: blocking1.9?
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...
I don't think we should block on this.
Flags: blocking1.9? → blocking1.9-
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. 
Blocks: 426727
No longer blocks: 420237
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #381450 - Flags: review?(enndeakin)
Comment on attachment 381450 [details] [diff] [review]
patch

I suppose this will have to do for now. We can revisit this during bug 390647.
Attachment #381450 - Flags: review?(enndeakin) → review+
http://hg.mozilla.org/mozilla-central/rev/180192a5c56f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Why not apply this just to autocomplete richlistitems?
Because we want it for all richlistboxes, see bug 426727.
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.
Attachment #381450 - Flags: review+ → review-
(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?
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.
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.
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?
Attachment #381450 - Flags: review- → review?(enndeakin)
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?
(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.
Attached patch alternative approach (obsolete) — Splinter Review
Attachment #381450 - Attachment is obsolete: true
Attachment #389286 - Flags: review?(neil)
Attachment #381450 - Flags: review?(enndeakin)
Component: XUL Widgets → Widget: Win32
Product: Toolkit → Core
QA Contact: xul.widgets → win32
Summary: properly send focus/hover events for elements in richlistboxes so they can use -moz-appearance: menuItem → allow styling richlistitems with -moz-appearance:menuitem
Version: unspecified → Trunk
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 on attachment 389286 [details] [diff] [review]
alternative approach

Or maybe you should just check aFrame->GetType() != nsWidgetAtoms::menuFrame?
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 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.
Attachment #389286 - Flags: review?(neil) → review+
(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).
Attached patch for check-inSplinter Review
Attachment #389286 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.