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

Status: RESOLVED FIXED

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

Description

[Louis Chang [:louis]]

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.

Comment 1

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 1 - Add a value menulist-button-orig to -moz-appearance.

Review commit: https://reviewboard.mozilla.org/r/212336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212336/

Comment 2

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 2 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Mac.

Review commit: https://reviewboard.mozilla.org/r/212338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212338/

Comment 3

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 3 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Linux.

Review commit: https://reviewboard.mozilla.org/r/212340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212340/

Comment 4

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 4 - Rename menulist-button to menulist-button-orig and make behavior of menulist-button like menulist on Windows.

Review commit: https://reviewboard.mozilla.org/r/212342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212342/

Comment 5

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 5 - Change the appearance of dropmarker from menulist-button to menulist-button-orig.

Review commit: https://reviewboard.mozilla.org/r/212344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212344/

Comment 6

[Louis Chang [:louis]]

Attachment: Bug 1428676 Part 6 - Add the pref "layout.css.webkit-appearance.enabled" to gEarlyPrefs list.

Review commit: https://reviewboard.mozilla.org/r/212346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212346/

Comment 7

[Louis Chang [:louis]]

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.

Comment 8

[Louis Chang [:louis]]

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

Comment 9

[Louis Chang [:louis]]

> 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 10

[Andrea Marchesini [:baku]]

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 11

[Mats Palmgren (inactive)]

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 12

[Mats Palmgren (inactive)]

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 13

[Mats Palmgren (inactive)]

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 14

[Mats Palmgren (inactive)]

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 15

[Mats Palmgren (inactive)]

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.

Comment 16

[Daniel Holbert [:dholbert]]

(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.)

Comment 17

[Maire Reavy [:mreavy]]

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

Comment 18

[Jonathan Watt [:jwatt]]

(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.

Comment 19

[Jonathan Watt [:jwatt]]

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 20

[Jonathan Watt [:jwatt]]

Attachment: pt 1. Add a '-moz-menulist-button' value to '-moz-appearance'

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: pt 3. Change our existing consumers of 'menulist-button' to use '-moz-menulist-button'

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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 23

[Emilio Cobos Álvarez (:emilio)]

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 24

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Stefan Hindli [:stefan_hindli]]

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:

Comment 27

[Pulsebot]

Backout by [e-mail]:
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

Comment 28

[Jonathan Watt [:jwatt]]

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.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Jonathan Watt [:jwatt]]

(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.

Comment 32

[Dorel Luca [:dluca]]

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

Comment 33

[Jonathan Watt [:jwatt]]

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 34

[Jonathan Watt [:jwatt]]

Attachment: pt 4. Move pref layout.css.webkit-appearance.enabled to StaticPrefList.h

Comment 35

[Jonathan Watt [:jwatt]]

Attachment: pt 5. Honer the webkit pref when handling 'menulist-button' on Mac

Comment 36

[Jonathan Watt [:jwatt]]

Attachment: pt 6. Honor the webkit pref when handling 'menulist-button' on Linux

Comment 37

[Jonathan Watt [:jwatt]]

Attachment: pt 7. Honor the webkit pref when handling 'menulist-button' on Windows

Comment 38

[Jonathan Watt [:jwatt]]

Attachment: pt 2. Test content can't use '-moz-appearance: -moz-menulist-button'. r=emilio

Comment 39

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 40

[Pulsebot]

Pushed by [e-mail]:
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

Comment 41

[Jonathan Watt [:jwatt]]

(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.

Comment 42

[Eliza Balazs [:ebalazs_]]

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

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428676 - Rename -moz-menulist-button to -moz-menulist-button-arrow. r=spohl,mstange

This should be less confusing. This is not supported outside of chrome:// or
user-agent stylesheets so we can name this however we want.

Depends on D65604

Comment 44

[Phabricator Automation]

Comment on attachment 9131244 [details]
Bug 1428676 - Rename -moz-menulist-button to -moz-menulist-button-arrow. r=spohl,mstange

Revision D65605 was moved to bug 1620307. Setting attachment 9131244 [details] to obsolete.