Closed Bug 1428676 Opened 2 years ago Closed 2 years ago

-moz-appearance menulist-button should behave like -webkit-appearance: menulist-button

Categories

(Core :: Layout: Form Controls, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: lochang, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webkitcompat] [webcompat])

Attachments

(7 files, 6 obsolete files)

29.01 KB, patch
emilio
: review+
Details | Diff | Splinter Review
3.54 KB, patch
emilio
: review+
Details | Diff | Splinter Review
2.05 KB, patch
emilio
: review+
Details | Diff | Splinter Review
8.57 KB, patch
emilio
: review+
Details | Diff | Splinter Review
10.33 KB, patch
emilio
: review+
Details | Diff | Splinter Review
10.45 KB, patch
emilio
: review+
Details | Diff | Splinter Review
1.09 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
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.
Summary: -moz-appearance meulist-button should behave like -webkit-appearance: menulist-button → -moz-appearance menulist-button should behave like -webkit-appearance: menulist-button
Depends on: 1429700
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
Attachment #8942101 - Flags: review?(amarchesini) → review+
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.
Attachment #8942097 - Flags: review?(mats) → review-
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
Attachment #8942098 - Flags: review?(mats)
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
Attachment #8942099 - Flags: review?(mats)
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
Attachment #8942100 - Flags: review?(mats)
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.
Attachment #8942096 - Flags: review?(mats)
(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.)
Depends on: 1429713
No longer depends on: 1429700
Updating owner per our standup discussion this morning.  Thanks, Jonathan, for taking this on and pushing -webkit-appearance work forward!
Assignee: lochang → jwatt
Priority: P3 → P2
(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.
Attachment #8942096 - Attachment is obsolete: true
Attachment #8942097 - Attachment is obsolete: true
Attachment #8942098 - Attachment is obsolete: true
Attachment #8942099 - Attachment is obsolete: true
Attachment #8942100 - Attachment is obsolete: true
Attachment #8942101 - Attachment is obsolete: true
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.
Attachment #8995471 - Flags: review?(emilio) → review+
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.
Attachment #8995472 - Flags: review?(emilio) → review+
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
Attachment #8995472 - Attachment description: pt 2. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button' → pt 3. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button'
Keywords: leave-open
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:
Flags: needinfo?(jwatt)
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.
Flags: needinfo?(jwatt)
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.
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.
Blocks: 1480073
Attachment #8996674 - Flags: review?(emilio) → review+
Attachment #8996675 - Flags: review?(emilio) → review+
Attachment #8996676 - Flags: review?(emilio) → review+
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 :)
Attachment #8996677 - Flags: review?(emilio) → review+
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.
Keywords: leave-open
Blocks: 1481565
Blocks: 1481615
You need to log in before you can comment on or make changes to this bug.