STR: 1) Make sure e10s is enabled 2) Visit https://treeherder.mozilla.org/perf.html#/graphs 3) Click on the "Last 14 Days" <select> dropdown 4) Hover any of the items in the popup ER: The items in the <select> dropdown should have the native appearance on hover. AR: See screenshot.

I suspect the right approach here would be what we do with old-school popups - essentially, use the system colours for highlighting the hovered item.

Comparing to Chrome here, with the following data-uri, they still only use the system colors for the selected item, data:text/html,<style>option { background: orange; color: white; }</style><select><option>one<option>two<option>three I'll attach a patch that drops the filter:invert(100%) part.

Comment on attachment 8833044 [details] Bug 1336125 - Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. https://reviewboard.mozilla.org/r/109272/#review110646 ::: browser/base/content/test/general/browser_selectpopup.js:780 (Diff revision 1) > } > } > > // Press Down to move the selected item to the next item in the > // list and check the colors of this item when it's not selected. > + let menulist = document.getElementById("ContentSelectDropdown"); We get the ContentSelectDropdown on line 752 and then get at the menupopup. I wonder if it'd make more sense to get the menulist first, above 752, and then: ``` let selectPopup = menulist.menupopup; ``` Not a blocker though. Up to you. Feel free to drop. ::: toolkit/modules/SelectParentHelper.jsm:174 (Diff revision 1) > function populateChildren(menulist, options, selectedIndex, zoom, > uaBackgroundColor, uaColor, > parentElement = null, isGroupDisabled = false, > - adjustedTextSize = -1, addSearch = true) { > + adjustedTextSize = -1, addSearch = true, nthChildIndex = 1) { This function signature is getting pretty unwieldy. :/ I think this code is likely in need of a refactor. Not a thing you need to do, just wanted to express myself. :) ::: toolkit/modules/SelectParentHelper.jsm:180 (Diff revision 1) > + let scopedStyleSheet = element.querySelector("#ContentSelectDropdownScopedStylesheet"); > + if (!scopedStyleSheet) { I dunno, this is started to get complicated. We're adding a stylesheet dynamically, and a rule for each <option> (the number of which is unbounded)... also I don't see where we remove the stylesheet or the rules, so it looks like it's just going to accumulate and accumulate. I also don't see where you set the ContentSelectDropdownScopedStylesheet id. Is there not a simpler way? See my comment about -moz-appearance: none. ::: toolkit/themes/linux/global/menu.css:35 (Diff revision 1) > menuitem[customoptionstyling="true"] { > -moz-appearance: none; > } > Can we not just add a `:not([_moz-menuactive="true"])` on these `-moz-appearance: none` rules and get the effect that we want?

Dropping review request until the other issues are either addressed or commented upon. :)

Comment on attachment 8833044 [details] Bug 1336125 - Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. https://reviewboard.mozilla.org/r/109272/#review110646 > This function signature is getting pretty unwieldy. :/ I think this code is likely in need of a refactor. Not a thing you need to do, just wanted to express myself. :) Yeah, I agree that it's ugly and unweildy too. I attempted to fix that as a separate patch, and I was going to move to an options argument that would allow for named arguments, but this function is only called in two places and these optional parameters are only used in one of those places. So I reverted my refactoring because it didn't seem to make the code any cleaner. > I dunno, this is started to get complicated. We're adding a stylesheet dynamically, and a rule for each <option> (the number of which is unbounded)... also I don't see where we remove the stylesheet or the rules, so it looks like it's just going to accumulate and accumulate. > > I also don't see where you set the ContentSelectDropdownScopedStylesheet id. > > Is there not a simpler way? See my comment about -moz-appearance: none. This is a good quesiton, and I appreciate you asking it because I asked the same thing myself. The stylesheet is removed from the populate() function on line 30 with the following code: > // Clear the current contents of the popup > menulist.menupopup.textContent = ""; So each time populate() is called, the stylesheet and options are removed and recreated. This isn't a change in behavior, just showing that state is cleared between invocations. > Can we not just add a `:not([_moz-menuactive="true"])` on these `-moz-appearance: none` rules and get the effect that we want? This won't work because we need -moz-appearance:none; on Windows, and more specific than any other styles. The scoped stylesheet was the lowest impact solution (as it doesn't apply to styles elsewhere in the chrome) and also allowed me to work around the specificty-issue of the inline styles.

Comment on attachment 8833044 [details] Bug 1336125 - Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. https://reviewboard.mozilla.org/r/109272/#review110646 > This is a good quesiton, and I appreciate you asking it because I asked the same thing myself. The stylesheet is removed from the populate() function on line 30 with the following code: > > // Clear the current contents of the popup > > menulist.menupopup.textContent = ""; > > So each time populate() is called, the stylesheet and options are removed and recreated. This isn't a change in behavior, just showing that state is cleared between invocations. Scratch that, I just noticed that the stylesheet isn't a child of menupopup. This changed as I was getting the test to pass. I'll fix that now and resubmit.

Comment on attachment 8833044 [details] Bug 1336125 - Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. Okay, putting back review flag.

Comment on attachment 8833044 [details] Bug 1336125 - Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. https://reviewboard.mozilla.org/r/109272/#review111684 I guess we'll need this to base the other ones on. :)

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87d2810770f3 Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. r=mconley

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=75295018&repo=autoland https://hg.mozilla.org/integration/autoland/rev/7e98eb6b3907

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d229cbc1b312 Apply option styles using a scoped stylesheet to allow for disabling custom styling on the active item. r=mconley