According to comment 30 in bug 1368555, we should rename current menulist-button to -moz-menulist-button then implement new menulist-button which behaves like menulist.

Hi mats, could you review the patches for me? Thanks. Part 1 is to add a new value menulist-button-orig to -moz-appearance. I tried to name it as -moz-menulist-button but it seemed the stylo parser will ignore the -moz prefix and generate a const MENULIST_BUTTON which will conflict with the menulist-button. Other prefixed appearance value has a platform prefix follow by -moz such as -moz-mac-help-button. But menulist-button-orig should work on all platforms. So I am not sure whether there is a better naming for it? Part 2-4 are to change the original menulist-button implementation based on the pref "layout.css.webkit-appearance.enabled" on each platform. menulist-button will behave like menulist when pref is on else behave like menulist-button-orig. menulist-button-orig has the same behavior as original menulist-button. Part 5 is to change the appearance of select > button (dropmarker) to menulist-button-orig. Not sure it this is reasonable. Or we can add a pref control here as well.

Hi baku, could you review the part 6 patch for me? Thanks. I would access the pref "layout-css.webkit-appearance.enabled" (will be landed in bug 1429700) in previous patches. I will hit the MOZ_CRASH [1] if I don't add the pref in the white list. [1] https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#789

> Part 1 is to add a new value menulist-button-orig to -moz-appearance. I > tried to name it as -moz-menulist-button but it seemed the stylo parser will > ignore the -moz prefix and generate a const MENULIST_BUTTON which will > conflict with the menulist-button. Other prefixed appearance value has a > platform prefix follow by -moz such as -moz-mac-help-button. But > menulist-button-orig should work on all platforms. So I am not sure whether > there is a better naming for it? Once we decide the naming, I will open a bug and add the new value for stylo part.

Comment on attachment 8942101 [details] Bug 1428676 Part 6 - Add the pref "layout.css.webkit-appearance.enabled" to gEarlyPrefs list. https://reviewboard.mozilla.org/r/212346/#review218140

Comment on attachment 8942097 [details] Bug 1428676 Part 2 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Mac. https://reviewboard.mozilla.org/r/212338/#review220300 ::: widget/cocoa/nsNativeThemeCocoa.mm:2652 (Diff revision 1) > case NS_THEME_MENULIST_TEXTFIELD: > DrawDropdown(cgContext, macRect, eventState, aWidgetType, aFrame); > break; > > - case NS_THEME_MENULIST_BUTTON: > + case NS_THEME_MENULIST_BUTTON: { > + if (mozilla::Preferences::GetBool("layout.css.webkit-appearance.enabled")) { Preferences::GetBool is a bit slow. Please add a nsLayoutUtils convenience function that caches the result with AddBoolVarCache. See for example: https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/layout/base/nsLayoutUtils.cpp#681 ::: widget/cocoa/nsNativeThemeCocoa.mm:3247 (Diff revision 1) > break; > } > > case NS_THEME_MENULIST: > case NS_THEME_MENULIST_BUTTON: > + case NS_THEME_MENULIST_BUTTON_ORIG: I don't think we should expose the name 'menulist-button-orig' on the web, even temporarily. I think '-moz-menulist-button' is acceptable -- please consult with Stylo experts (not me) how to implement that. r- for now because of this naming issue.

Comment on attachment 8942098 [details] Bug 1428676 Part 3 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Linux. https://reviewboard.mozilla.org/r/212340/#review220304

Comment on attachment 8942099 [details] Bug 1428676 Part 4 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Windows. https://reviewboard.mozilla.org/r/212342/#review220306

Comment on attachment 8942100 [details] Bug 1428676 Part 5 - Change the appearance of dropmarker from menulist-button to menulist-button-orig. https://reviewboard.mozilla.org/r/212344/#review220308

Comment on attachment 8942096 [details] Bug 1428676 Part 1 - Add a value menulist-button-orig to -moz-appearance. https://reviewboard.mozilla.org/r/212336/#review220302 Canceling remaining reviews until the naming issue is resolved.

(Note that :louis's bugzilla account is now disabled, presumably as part of the Taipei office closure & the fact that his account used a @mozilla.com email address. It's possible he might still be active on this bug as a volunteer, but not via this same bugzilla account, probably.)

Updating owner per our standup discussion this morning. Thanks, Jonathan, for taking this on and pushing -webkit-appearance work forward!

(In reply to Mats Palmgren (:mats) (on vacation) from comment #11) > I don't think we should expose the name 'menulist-button-orig' on the web, > even temporarily. By way of update, it's restricting this visibility that's holding this up. Emilio has taken up the case to make it possible to do this without too much yak shaving. > I think '-moz-menulist-button' is acceptable Louis' point was that it's not currently possible to have this name and 'menulist-button' at the same time. It may be after Emilio has finished hacking up the stylo code, but if not I don't think this should block since the value will only be visible/usable in our code.

Okay, we're now able to both hide the visibility of values from non-UA sheets, and to have both a prefixed and unprefixed value of the same name. That vastly simplifies the upcoming patches. Thanks, Emilio! \o/ I'm still working on getting the `-webkit-appearance: menulist-button` theming compatible enough, but for now I'll put the initial patches up to add a -moz-menulist-button value, and convert our code over to use that value. Now that Emilio's cleared the way to resolve the outstanding issues with those changes there's no point in them continuing to sit in my tree.

Comment on attachment 8995471 [details] [diff] [review] pt 1. Add a '-moz-menulist-button' value to '-moz-appearance' Review of attachment 8995471 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/properties/longhands/box.mako.rs @@ +514,5 @@ > "computed::Appearance::None", > products="gecko", > alias="-webkit-appearance:layout.css.webkit-appearance.enabled", > spec="Nonstandard (https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-appearance)", > + needs_context=True, Just remove the line (True is default) ::: servo/components/style/values/specified/box.rs @@ +19,5 @@ > use values::specified::{AllowQuirks, Number}; > use values::specified::length::{LengthOrPercentage, NonNegativeLength}; > > #[cfg(feature = "gecko")] > +fn is_for_ua_sheet(context: &ParserContext) -> bool { nit: Let's call this `in_ua_or_chrome_sheet`? this is not just UA sheets. @@ +30,5 @@ > fn moz_display_values_enabled(context: &ParserContext) -> bool { > use gecko_bindings::structs; > use stylesheets::Origin; > context.stylesheet_origin == Origin::UserAgent || > context.chrome_rules_enabled() || Let's reuse the function above here, so this becomes: in_ua_or_chrome_sheet(context) || [pref] And same in the function below.

Comment on attachment 8995472 [details] [diff] [review] pt 3. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button' Review of attachment 8995472 [details] [diff] [review]: ----------------------------------------------------------------- We may want to double-check with the frontend people if they'd be fine with the behavior change, once we've settled on that, so we can potentially remove the just-added value.

Comment on attachment 8995471 [details] [diff] [review] pt 1. Add a '-moz-menulist-button' value to '-moz-appearance' Review of attachment 8995471 [details] [diff] [review]: ----------------------------------------------------------------- Oh, also, could you add the `-moz-menulist-button` value to test_non_content_accessible_values.html? Thanks!

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/662bfc31a950 pt 1. Add a '-moz-menulist-button' value to '-moz-appearance'. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1270a9ec3d pt 2. Test content can't use '-moz-appearance: -moz-menulist-button'. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba5975115fc pt 3. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button'. r=emilio

Backed out 3 changesets (Bug 1428676) for bc failures in browser/base/content/test/static/browser_parsable_css.js on a CLOSED TREE Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=191148293 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191148293&repo=mozilla-inbound&lineNumber=1691 15:49:52 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for resource://gre-resources/forms.css: Error in parsing value for ‘-moz-appearance’. Declaration dropped. - 15:49:52 INFO - Stack trace: 15:49:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:257 15:49:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:456 15:49:52 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103 15:49:52 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094 15:49:52 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996 15:49:52 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-display-comboboxcontrol-frame’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-dropdown-list’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-dropdown-list’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘overflow-clip-box’. Declaration dropped." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/(?:res|gre-resources)\\/forms\\.css$/i","errorMessage":"/Unknown property.*overflow-clip-box/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘overflow-clip-box’. Declaration dropped." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/(?:res|gre-resources)\\/forms\\.css$/i","errorMessage":"/Unknown property.*overflow-clip-box/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-button-content’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-button-content’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-number-wrapper’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-number-text’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-number-spin-box’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-number-spin-up’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-number-spin-down’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-autofill’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-autofill-preview’. Ruleset ignored due to bad selector." on resource://gre-resources/forms.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-use-shadow-tree-root’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Expected media feature name but found ‘-moz-is-glyph’." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(autocomplete-item|svg)\\.css$/","errorMessage":"/Expected media feature name but found \\u2018-moz.*/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Expected media feature name but found ‘-moz-is-resource-document’." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(autocomplete-item|svg)\\.css$/","errorMessage":"/Expected media feature name but found \\u2018-moz.*/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-foreign-content’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-text’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-marker-anon-child’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘appearance’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Ignored error "Unknown property ‘user-select’. Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true} 15:49:52 INFO - Not taking screenshot here: see the one that was previously logged 15:49:52 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | All the styles (207) loaded without errors. - Got 1, expected 0 15:49:52 INFO - Stack trace: 15:49:52 INFO - chrome://mochikit/content/browser-test.js:test_is:1305 15:49:52 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:457 15:49:52 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103 15:49:52 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094 15:49:52 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996 15:49:52 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 15:49:52 INFO - Not taking screenshot here: see the one that was previously logged 15:49:52 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Unused whitelist item: {"sourceName":"/(?:res|gre-resources)\\/forms\\.css$/i","errorMessage":"/Error in parsing value for â-moz-appearanceâ\\. Declaration dropped\\./i","isFromDevTools":false} - 15:49:52 INFO - Stack trace:

Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/795a00117363 Backed out 3 changesets for bc failures in browser/base/content/test/static/browser_parsable_css.js on a CLOSED TREE

Meh. The error message I copied from the error console had collapsed white space, and I didn't notice because it turns out this test doesn't actually run when run locally in a debug build. I'll fix that and reland those patches.

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6b8bc93e51d pt 1. Add a '-moz-menulist-button' value to '-moz-appearance'. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebcaa67c3f7 pt 2. Test content can't use '-moz-appearance: -moz-menulist-button'. r=emilio

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/f856cee67cd2 pt 3. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button'. r=emilio

(In reply to Emilio Cobos Álvarez (:emilio) from comment #23) > Comment on attachment 8995472 [details] [diff] [review] > pt 3. Change our existing consumers of 'menulist-button' to use > '-moz-menulist-button' > > We may want to double-check with the frontend people if they'd be fine with > the behavior change, once we've settled on that, so we can potentially > remove the just-added value. I don't think we even need to check - '-moz-menulist-button' (and soon to be "old" 'menulist-button') shows only the arrow part of the dropdown. The new/Chrome 'menulist-button' behavior shows the entire menulist including the border around the control and the selected item. The frontend definitely won't want their menu arrows to to change in that way, not least because they're often used in places where the entire dropdown isn't supposed to look like a traditional dropdown.

https://hg.mozilla.org/mozilla-central/rev/e6b8bc93e51d https://hg.mozilla.org/mozilla-central/rev/4ebcaa67c3f7 https://hg.mozilla.org/mozilla-central/rev/f856cee67cd2

To fix the widget code to handle both the new and old behavior based on the "layout.css.webkit-appearance.enabled" I originally went with the a fall-through approach in the switch statements (as is Louis' original work on this bug). However, that causes a bunch of 'hg blame' churn, and will result in more churn when we remove the pref at some point in the future. (It also makes the code harder to follow in certain places.) For example, changes look like this: diff --git a/widget/cocoa/nsNativeThemeCocoa.mm b/widget/cocoa/nsNativeThemeCocoa.mm --- a/widget/cocoa/nsNativeThemeCocoa.mm +++ b/widget/cocoa/nsNativeThemeCocoa.mm @@ -3310,28 +3310,32 @@ nsNativeThemeCocoa::ComputeWidgetInfo(ns return Some(WidgetInfo::NativeTitlebar( UnifiedToolbarParams{unifiedToolbarHeight, isMain})); } case StyleAppearance::Statusbar: return Some(WidgetInfo::StatusBar(IsActive(aFrame, YES))); case StyleAppearance::Menulist: + case StyleAppearance::MenulistButton: case StyleAppearance::MenulistTextfield: { - ControlParams controlParams = ComputeControlParams(aFrame, eventState); - controlParams.focused = controlParams.focused || IsFocused(aFrame); - controlParams.pressed = IsOpenButton(aFrame); - DropdownParams params; - params.controlParams = controlParams; - params.pullsDown = false; - params.editable = aWidgetType == StyleAppearance::MenulistTextfield; - return Some(WidgetInfo::Dropdown(params)); + if (aWidgetType != StyleAppearance::MenulistButton || + StaticPrefs::layout_css_webkit_appearance_enabled()) { + ControlParams controlParams = ComputeControlParams(aFrame, eventState); + controlParams.focused = controlParams.focused || IsFocused(aFrame); + controlParams.pressed = IsOpenButton(aFrame); + DropdownParams params; + params.controlParams = controlParams; + params.pullsDown = false; + params.editable = aWidgetType == StyleAppearance::MenulistTextfield; + return Some(WidgetInfo::Dropdown(params)); + } + MOZ_FALLTHROUGH; } - case StyleAppearance::MenulistButton: case StyleAppearance::MozMenulistButton: return Some(WidgetInfo::Button( ButtonParams{ComputeControlParams(aFrame, eventState), ButtonType::eArrowButton})); case StyleAppearance::Groupbox: return Some(WidgetInfo::GroupBox()); In some places, particularly the GTK code, the cases are not next to each other (and there are reasons to group them apart) so it would make it even messier there. It seems preferable to simply change the value of aWidgetType at the top of all the relevant widget methods when the pref is set. Quite apart from avoiding code churn it is simpler and less error prone to implement. I discussed this with Emilio and he agrees that's an acceptable approach.

Comment on attachment 8996677 [details] [diff] [review] pt 7. Honor the webkit pref when handling 'menulist-button' on Windows Review of attachment 8996677 [details] [diff] [review]: ----------------------------------------------------------------- Can't wait to remove the pref in release and clean up all this :)

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/aca8f9010599 pt 4. Move pref layout.css.webkit-appearance.enabled to StaticPrefList.h. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2251bbc4c4 pt 5. Honor the webkit pref when handling 'menulist-button' on Mac. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7c376aeba4 pt 6. Honor the webkit pref when handling 'menulist-button' on Linux. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/25535abbb54e pt 7. Honor the webkit pref when handling 'menulist-button' on Windows. r=emilio

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39) > Can't wait to remove the pref in release and clean up all this :) Indeed.

https://hg.mozilla.org/mozilla-central/rev/aca8f9010599 https://hg.mozilla.org/mozilla-central/rev/7f2251bbc4c4 https://hg.mozilla.org/mozilla-central/rev/9e7c376aeba4 https://hg.mozilla.org/mozilla-central/rev/25535abbb54e