418552 - Use native styling for drop down controls on Vista in XUL and content area

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

We should use the native styling for drop down controls in Vista in the content area.

A related bug for chrome is Bug 390737, On Vista, every menulist should be a menubutton.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Example of native drop down controls in the content area on Vista

Here is what drop down controls look like in IE.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Nominating for blocking to pick up wanted.

Comment 3

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

Not sure that this blocks, but it's definitely wanted.

Comment 4

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

Attachment: patch, v1.

Note that this also fixes bug 418537 (match native appearance of text fields).

Tossing this out for review.  There are a few things in here that I'm not sure about, and some that may well be unnecessary.  The set of changes here:

1) the change to PaintFocus is probably not needed (see blow for 1px hack)

2) addition of the atoms to GkAtoms is also not needed (I really wanted CSS keywords, bt I didn't know that at the time)

3) the menulist.css change I -hope- can be overridden by the theme.  If this isn't commented out, then we get the ugly focus highlight; as it is in the patch, we just get the outline, which looks much nicer.

4) We change the DROPDOWN rendering to use the actual Combobox theme, as opposed to just using the border from the Edit theme.  The frame exists under XP, and is the correct one, so this should be safe.  Most other changes are vista-only.

5) A StandardGetState method is introduced to avoid duplicating a pile of frame state -> theme state code.

6) For vista textfields, we use the EDITBORDER_NOSCROLL style, to get the slightly rounded border and highlight-on-hover effect.

7) For dropdowns, if we're in HTML content, we draw just the border, otherwise we draw border + background.  This is consistent with IE and other vista apps.  When just the border is drawn, we have a problem, because it's a pretty tight border around the content -- the 1px rectangle drawn by PaintFocus was overpainting it!  This was the original reason for the PaintFocus change, but even that doesn't help, because if the widget does have focus, then the dotted outline will draw over the border (in HTML content).  Instead, for HTML dropdowns, we add 1px of padding on all sides.

8) .. to match that 1px padding, we have to expand the DROPDOWN_BUTTON by 1px; it's supposed to paint over the border.  If we just apply the 1px padding, then the button paints just inside the border and is wrong.  This is a -bit- heavy-handed, but I'm pretty sure it's correct.  I don't know of any other way to achieve this same effect, given that the frames are supposed to actually overlap.  There may be some subtle invalidation issues here, but I think they're minor and ignorable (and probably won't be noticable, since on any state change of the dropdown, the rest of the button will need to be invalidated anyway)

9) To handle open dropdowns (whether HTML or not) retaining the active state, if an element doesn't have the mozmenuactive attribute, we check for the standard 'open' attribute when determining openness.

10) We use the proper textfield style on vista to get the slightly rounded border.

Comment 5

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

Attachment: screenshot of dropdowns in chrome

Comment 6

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

Attachment: screenshot of dropdowns in content, as well as edit boxes

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (mFocused != this)
+    return;

This is going to change the rendering of unfocused comboboxes on Mac and GTK when the combobox is not native-themed because the author is styling it. So you really should get rid of it.

+  if (!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {

Is drawing the background color really the right thing to do on Vista if the control is focused but disabled?

> 3) the menulist.css change I -hope- can be overridden by the theme.

What do you mean?

+void
+nsNativeThemeWin::StandardGetState(nsIFrame* aFrame, PRUint8 aWidgetType,
+                                   PRInt32& aState, PRBool wantFocused)

Wouldn't it make more sense to return aState as the result? And don't make wantFocused a default parameter, that makes it a little bit harder to use and there's no need for it here.

+      if (mIsVistaOrLater)
+        aPart = TFP_EDITBORDER_NOSCROLL;

So *scrollable* multiline textboxes get this NOSCROLL style? Does that make sense?

+
+  if (mIsVistaOrLater) {
+    if (aWidgetType == NS_THEME_DROPDOWN_BUTTON) {
+      nsIContent* content = aFrame->GetContent();
+      PRBool isHTML = content && content->IsNodeOfType(nsINode::eHTML);
+      if (isHTML) {
+        tr.pos.y -= 1.0;
+        tr.size.width += 1.0;
+        tr.size.height += 2.0;
+
+        cr.pos.y -= 1.0;
+        cr.size.width += 1.0;
+        cr.size.height += 2.0;
+      }
+    }
+  }

I don't think this is actually going to work right unless you override GetWidgetOverflow as well so layout knows that the dropdown button draws outside itself.

Want to define an Inflate method on gfxRect?

I'm not sure that this is the simplest way to do the combobox drawing you're trying to do, I just don't have enough of this in my head.

+      PRBool isHover = CheckBooleanAttr(aFrame, nsWidgetAtoms::mozmenuactive);
+      if (!isHover) {
+        PRInt32 eventState = GetContentState(aFrame, aWidgetType);
+        if (eventState & NS_EVENT_STATE_HOVER)
+          isHover = PR_TRUE;
+      }

Want to factor this into shared code? There are 3 occurrences.

+  PRBool mIsVistaOrLater;

PRPackedBool ... doesn't matter here but people get the wrong idea.

Comment 8

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

(In reply to comment #7)
> +  if (mFocused != this)
> +    return;
> 
> This is going to change the rendering of unfocused comboboxes on Mac and GTK
> when the combobox is not native-themed because the author is styling it. So you
> really should get rid of it.

Ok, will do.  It's really sort of crappy styling though, why do unfocused comboboxes need a hardcoded bgcolor border drawn?

> +  if (!mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
> 
> Is drawing the background color really the right thing to do on Vista if the
> control is focused but disabled?

That was intended to be combined with the first change -- so I'll get rid of this as well (there was no need to check for mFocused == this any more).

> > 3) the menulist.css change I -hope- can be overridden by the theme.
> 
> What do you mean?

Sorry, I meant that the menulist.css change would need to happen for the vista theme, but I don't want to check it in to menulist.css -- and I was hoping that themers could do that type of override for the element.  (I don't see any reason why not, I guess.)

> +void
> +nsNativeThemeWin::StandardGetState(nsIFrame* aFrame, PRUint8 aWidgetType,
> +                                   PRInt32& aState, PRBool wantFocused)
> 
> Wouldn't it make more sense to return aState as the result? And don't make
> wantFocused a default parameter, that makes it a little bit harder to use and
> there's no need for it here.

Will do.
 
> +      if (mIsVistaOrLater)
> +        aPart = TFP_EDITBORDER_NOSCROLL;
> 
> So *scrollable* multiline textboxes get this NOSCROLL style? Does that make
> sense?

Yeah; the difference between NOSCROLL and VSCROLL, HSCROLL, and HVSCROLL styles is just whether any side gets a vertical/horizontal line with a sharp corner, as opposed to getting a slightly rounded border.  I don't want to pull that information out (though, I guess I -could-, since I have the frame right there), but in practice I don't think it will matter as the scroll bar will be placed in the right spot and will just overlay the corner bit.  I can do some screenshots.

> +  if (mIsVistaOrLater) {
> +    if (aWidgetType == NS_THEME_DROPDOWN_BUTTON) {
> +      nsIContent* content = aFrame->GetContent();
> +      PRBool isHTML = content && content->IsNodeOfType(nsINode::eHTML);
> +      if (isHTML) {
> +        tr.pos.y -= 1.0;
> +        tr.size.width += 1.0;
> +        tr.size.height += 2.0;
> +
> +        cr.pos.y -= 1.0;
> +        cr.size.width += 1.0;
> +        cr.size.height += 2.0;
> +      }
> +    }
> +  }
> 
> I don't think this is actually going to work right unless you override
> GetWidgetOverflow as well so layout knows that the dropdown button draws
> outside itself.

Ah, I missed GetWidgetOverflow.  Ok.

> Want to define an Inflate method on gfxRect?

I can, though the expansion above isn't constant -- it's by 1px in the horizontal direction and by 2px top and bottom.  That may be very nitpicky.

> I'm not sure that this is the simplest way to do the combobox drawing you're
> trying to do, I just don't have enough of this in my head.

Yeah, I've sort of had to do some learn-by-failing figuring out of the details of our native theme stuff here :(

> +      PRBool isHover = CheckBooleanAttr(aFrame, nsWidgetAtoms::mozmenuactive);
> +      if (!isHover) {
> +        PRInt32 eventState = GetContentState(aFrame, aWidgetType);
> +        if (eventState & NS_EVENT_STATE_HOVER)
> +          isHover = PR_TRUE;
> +      }
> 
> Want to factor this into shared code? There are 3 occurrences.

Will do.

> +  PRBool mIsVistaOrLater;
> 
> PRPackedBool ... doesn't matter here but people get the wrong idea.
> 

Yep.  Ok, will put a new patch up today.  Thanks!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> Ok, will do.  It's really sort of crappy styling though, why do unfocused
> comboboxes need a hardcoded bgcolor border drawn?

I don't know but it's beyond the scope of this bug.

> > > 3) the menulist.css change I -hope- can be overridden by the theme.
> > 
> > What do you mean?
> 
> Sorry, I meant that the menulist.css change would need to happen for the vista
> theme, but I don't want to check it in to menulist.css -- and I was hoping
> that themers could do that type of override for the element.  (I don't see any
> reason why not, I guess.)

OK so that's not really intended to be part of your patch?

> Yeah; the difference between NOSCROLL and VSCROLL, HSCROLL, and HVSCROLL
> styles is just whether any side gets a vertical/horizontal line with a sharp
> corner, as opposed to getting a slightly rounded border.  I don't want to
> pull that information out (though, I guess I -could-, since I have the frame
> right there), but in practice I don't think it will matter as the scroll bar
> will be placed in the right spot and will just overlay the corner bit.  I can
> do some screenshots.

I don't get it, if it doesn't matter why do they even provide these styles? Getting the information shouldn't be all that hard as long as the different styles have the same metrics so we only need to get the scrollbar info when painting. You'd dig in to find the scrollframe, QI to nsIScrollableFrame and call GetActualScrollbarSizes.

> > Want to define an Inflate method on gfxRect?
> 
> I can, though the expansion above isn't constant -- it's by 1px in the
> horizontal direction and by 2px top and bottom.  That may be very nitpicky.

Yeah OK forget Inflate. Yuck.

Comment 10

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

Attachment: patch, v2.

Ok, updated with all review comments, and a few things fixed.  Edit box size was restored, which fixes problems with the urlbar.

I didn't do the hscroll/vscroll stuff; I did some checking with it, and it looks fine-as is.  Plus, IE doesn't seem to use the other styles, and I couldn't find any other windows app that seemed to.  The scrollbar isn't in the right spot in input boxes now, but it's not even without this change -- there's a 1px gap between the scrollbar and the border of the textbox, where there shouldn't be.  I'll make a separate bug and see if I can fix that, but it looks fine as-is.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 306202 [details] [diff] [review]
patch, v2.

The menulist.css change is spurious.

+    return TS_ACTIVE;
+  else if (wantFocused && eventState & NS_EVENT_STATE_FOCUS)
+    return TS_FOCUSED;
+  else if (eventState & NS_EVENT_STATE_HOVER)
+    return TS_HOVER;

Don't need else after return.

+  PRBool hover = CheckBooleanAttr(aFrame, nsWidgetAtoms::mozmenuactive);
+  if (!hover) {
+    PRInt32 eventState = GetContentState(aFrame, aWidgetType);
+    if (eventState & NS_EVENT_STATE_HOVER)
+      hover = PR_TRUE;
+  }
+
+  return hover;

Better as

  return CheckBooleanAttr(...) ||
         (GetContentState(...) & NS_EVENT_STATE_HOVER);

Comment 12

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

Attachment: updated with roc's comments

Updated, and requesting approval!

Comment 13

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

Comment on attachment 306440 [details] [diff] [review]
updated with roc's comments

a1.9b4=beltzner

Comment 14

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

Checked in.

Comment 15

[Alfred Kayser]

What is the purpose of the change in menulist.css?
After the patch it contains:
  background-color: transparent;
  background-color: Highlight;
One of these is too many in this town.

Comment 16

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

That didn't get checked in; was a leftover from an earlier change in menulist, where I had the highlight background color commented out.

Comment 17

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

Attachment: fixed

Verified!

Comment 18

[Leman Bennett [Omega]]

Native text field selected state does not stay colorized(focused?) once clicked on. Correct behavior in Explorer/IE shows that the fields stay colorized once focused.

If this is being worked on, please disregard.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022906 Minefield/3.0b4pre ID:2008022906

Comment 19

[Alvaro Muñoz]

shouldn't the locationbar drop down look native?

Comment 20

[James Ross]

You should probably file separate bugs on specific appearance issues, if they don't already exist; also, the URL bar and drop-down haves always been a bit of a special case.

Comment 21

[Dão Gottwald [:dao]]

Attachment: bug?

Did this bug add the hover effect that you see in the screen shot? Looks wrong for disabled selects.

Comment 22

[Victor Bielawski]

On Vista, the content menulist dropmarker background is visible on top of the menulist background in xul menulists.
Screenshot: http://img153.imageshack.us/img153/5763/menulistbuttonbugnu8.png
(This behaviour is correct on XP.)

>Did this bug add the hover effect that you see in the screen shot? Looks wrong
>for disabled selects.
This bug did add that. Same on Vista, only it's more visible: http://img232.imageshack.us/img232/8473/menulistdisabledbugnm8.png

Comment 23

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

Please, please, file new bugs for any issues related to this... it's impossible to track all this stuff in an already-FIXED bug. :)

The Vista hover effect is a bug; I filed bug 421987 for that.  The dropdown issue is actually a huge pain to fix; I'm not sure if we'll be able to.  It doesn't look that awful, I don't think :(

Comment 24

[Victor Bielawski]

Filed bug 422510 for the dropmarker background problem in chrome on Vista.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is alleged as a possible reason bug 393791 has become a lot worse in recent builds (see comments there).