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

VERIFIED FIXED

Status

()

Core
Widget: Win32
P2
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: faaborg, Assigned: vlad)

Tracking

({polish})

Trunk
x86
Windows Vista
polish
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 304389 [details]
Example of native drop down controls in the content area on Vista

Here is what drop down controls look like in IE.
OS: Mac OS X → Windows Vista
Version: unspecified → Trunk
(Reporter)

Comment 2

10 years ago
Nominating for blocking to pick up wanted.
Flags: blocking1.9?
Not sure that this blocks, but it's definitely wanted.
Flags: wanted1.9+
(Reporter)

Updated

10 years ago
Blocks: 333484
Assignee: nobody → vladimir
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Created attachment 305899 [details] [diff] [review]
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.
Attachment #305899 - Flags: review?(roc)
Created attachment 305900 [details]
screenshot of dropdowns in chrome
Created attachment 305901 [details]
screenshot of dropdowns in content, as well as edit boxes
+  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.
(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!
(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.
Created attachment 306202 [details] [diff] [review]
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.
Attachment #305899 - Attachment is obsolete: true
Attachment #306202 - Flags: review?(roc)
Attachment #305899 - Flags: review?(roc)
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Summary: Use native styling for drop down controls on Vista in the content area → Use native styling for drop down controls on Vista in XUL and content area
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);
Attachment #306202 - Flags: superreview+
Attachment #306202 - Flags: review?(roc)
Attachment #306202 - Flags: review+
Created attachment 306440 [details] [diff] [review]
updated with roc's comments

Updated, and requesting approval!
Attachment #306440 - Flags: approval1.9b4?
Comment on attachment 306440 [details] [diff] [review]
updated with roc's comments

a1.9b4=beltzner
Attachment #306440 - Flags: approval1.9b4? → approval1.9b4+
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 15

10 years ago
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.
That didn't get checked in; was a leftover from an earlier change in menulist, where I had the highlight background color commented out.
Status: RESOLVED → VERIFIED
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

10 years ago
shouldn't the locationbar drop down look native?

Comment 20

10 years ago
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.

Updated

10 years ago
Depends on: 420580
Created attachment 308122 [details]
bug?

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

Comment 22

10 years ago
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
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 :(

Updated

10 years ago
Depends on: 422488

Comment 24

10 years ago
Filed bug 422510 for the dropmarker background problem in chrome on Vista.
This is alleged as a possible reason bug 393791 has become a lot worse in recent builds (see comments there).
You need to log in before you can comment on or make changes to this bug.