[e10s] <select> dropdowns have strange colours on Perfherder

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

50 Branch
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [webcompat])

Attachments

(2 attachments)

Posted image Problem
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
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?
Attachment #8833044 - Flags: review?(mconley) → review-
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.
Attachment #8833044 - Flags: review?(mconley)
Duplicate of this bug: 1336599
Whiteboard: [webcompat]
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. :)
Attachment #8833044 - Flags: review?(mconley) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/d229cbc1b312
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(jaws)
See Also: → 1338931
Depends on: 1347145
You need to log in before you can comment on or make changes to this bug.