Closed Bug 431078 Opened 13 years ago Closed 10 years ago
Editable menulists in Vista are rendered in the style of non-editable menulists
869 bytes, application/vnd.mozilla.xul+xml
30.73 KB, image/png
2.05 KB, image/png
[for checkin] render editable menulists with CBP_DROPBORDER instead of CBP_DROPFRAME [r=jmathies+dao]
5.29 KB, patch
|Details | Diff | Splinter Review|
In chrome, non-editable menulists should be rendered in a new button-like style in Vista, while editable menulists should be rendered in the normal textbox style. However, Gecko renders both editable and non-editable menulists in the new button-like style in Vista. This does not apply to HTML content, as "menulists" in HTML (<select>) are always rendered in the textbox style.
Attached a patch which fixes the problem. The problem was that menulist's -moz-appearance was being inherited. I also checked to see if this problem was on OS X, Ubuntu, and Kubuntu, but everything worked normal on those with default settings and no code changes. I also verified in the other menulist.css files for other platforms and they all have similar settings that I came up with, winstripe was the only one missing it.
Attachment #536266 - Flags: review?(dao)
Component: Widget: Win32 → Themes
Product: Core → Toolkit
QA Contact: win32 → themes
Comment on attachment 536266 [details] [diff] [review] Editable menulists were not using -moz-appearance: menulist-textfield; This doesn't really look as expected... The win32 widget support for menulist-textfield seems to be broken / incomplete. We either need that fixed or should use -moz-appearance: textfield here.
(In reply to comment #3) > We either need that fixed or should use -moz-appearance: textfield here. Or option 3: We could keep using -moz-appearance: menulist. :) The menulist variations do not hinge solely on editable/non-editable: 1) Rendered like a textfield with a discrete button: Classic or pre-Aero or editable or non-editable in HTML content. And with a different dropmarker style if using Aero and the outer frame is custom-styled. 2) Rendered like a button: non-editable Aero in XUL If we want to expose these variations in CSS, then we should do it right and not part-way. For example, we would need to do something about the dropmarker. Either create another menulist-button appearance (e.g., menulist-textfield-button) or do what Linux does and hide the dropmarker element in the case of the button-ish style and draw the dropmarker directly in the menulist DrawWidgetBackground. The status quo is to not expose the menulist variations in CSS on Windows: we use only the menulist and menulist-button appearances and let the native theme code sort out how it should all be rendered. Exposing these variations in CSS seems cumbersome to me (and what would the benefit be?), so I am in favor of maintaining this status quo. Yes, this would leave the menulist-textfield unimplemented for Windows, but does the custom theme community care about this one? The current native theme code for the menulist frame and dropmarker does a check to see if it's in HTML content and uses the textfield-like style if it is. So the quick fix would be to do the same for non-editable menulists.
Component: Themes → Widget: Win32
Product: Toolkit → Core
QA Contact: themes → win32
Sorry to steal this bug from you, Brian! I had recently been poking around menulist-related code for another bug, so this patch was already kicking around in my head. This implements the quick fix that I advocated for in comment 5. This leaves the menulist-textfield appearance unimplemented, which I think is the sane thing to do.
No problem at all, I wasn't going to do another fix until tonight.
Comment on attachment 539255 [details] [diff] [review] render editable menulists with CBP_DROPBORDER instead of CBP_DROPFRAME I'm fine with the code change here but I'd like Dao to approve the look and feel changes.
Comment on attachment 539255 [details] [diff] [review] render editable menulists with CBP_DROPBORDER instead of CBP_DROPFRAME (I'm not a super-reviewer.)
Attachment #539255 - Flags: superreview?(dao) → review+
Resynchronized with m-c's tip and updated commit message.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.