Closed Bug 1386015 Opened 7 years ago Closed 7 years ago

[e10s] <option> elements with inherited colors generates very inefficient styling in the parent

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Performance Impact medium
Tracking Status
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: Felipe, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached file selecttest3000.html
I've been profiling some real-world testcases related to bug 1118086, and when there's a <select> element that specifies a color, and the <option> elements inherit that, we spend the largest chunk of time when constructing the menu in CSS rule matching: nsCSSRuleProcessor::RulesMatching, SelectorMatches, nsNthIndexCache::GetNthIndex etc.

That is because we build a stylesheet to style the menu popup that gets one rule per item to specify its color, like so (for a 3000 items example):

> #ContentSelectDropdown > menupopup {
>   color: rgb(0, 0, 255);
> }
> 
> #ContentSelectDropdown > menupopup > :nth-child(1):not([_moz-menuactive="true"]) {
>   color: rgb(0, 0, 255);
> }
> 
> #ContentSelectDropdown > menupopup > :nth-child(2):not([_moz-menuactive="true"]) {
>   color: rgb(0, 0, 255);
> }
> 
> #ContentSelectDropdown > menupopup > :nth-child(3):not([_moz-menuactive="true"]) {
>   color: rgb(0, 0, 255);
> }
> 
> // ... snip ...
> 
> #ContentSelectDropdown > menupopup > :nth-child(3000):not([_moz-menuactive="true"]) {
>   color: rgb(0, 0, 255);
> }

We can't just set the color directly to it through the style attribute, due to the :not([_moz-menuactive="true"]) part, but this giant ruleset could be simplified down to:

> #ContentSelectDropdown > menupopup {
>   color: rgb(0, 0, 255);
> }
> 
> #ContentSelectDropdown > menupopup > :not([_moz-menuactive="true"]) {
>   color: inherit;
> }

Neither of the testcases from bug 1118086 shows this, but the one from 1383205 did.

I'm attaching a testcase with 3000 items, and in my machine, this change cuts the display time from 3100ms to 700ms.
Whiteboard: [qf]
(actually directly marking this as p1 as it blocks a p1)
Whiteboard: [qf] → [qf:p1]
(In reply to :Felipe Gomes (needinfo me!) from comment #1)
> (actually directly marking this as p1 as it blocks a p1)

(I updated the bug it blocks to be qf:p2, so updating this one as well.)
Whiteboard: [qf:p1] → [qf:p2]
I'll take it.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
I went through the code that produces it and got stuck at [0].

it seems that the code is already by default trying to not create a css rule per option *unless* there's a property on option that requests the color to be set.
So, if I read it properly, the thing :Felipe noticed means that for each of the 3000 option objects the `color` property is set.

Do you know where is it set and why do we set a color for each option separately?

My understanding is that we want to keep the code in SelectParentHelper because each option may want to have a special color.
But by default I assume there should be no per-option property, and I'm not sure where this code sets it wrongly.


[0] http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/modules/SelectParentHelper.jsm#294
Flags: needinfo?(jaws)
At [1] is where we set the color of the <select>.

Then at [2] is where we set the color of each child <option>.

To fix this bug all that we should need to do is pass the color of the select to populateChildren(), then only create the rule-specific style if the color of the <option> is different than the <select>.

background-color and text-shadow don't inherit by default so we don't need to do this special case for them.

Please add a test case to /browser/base/content/test/forms/browser_selectpopup_colors.js or create a new test file since that test is getting long already.

The test can work like as follows:

> const SELECT_INHERITED_COLORS_ON_OPTIONS_DONT_GET_UNIQUE_RULES_IF_RULE_SET_ON_SELECT =
>   "<html><head><style>" +
>   "  select { color: blue; }" +
>   "  .redColor { color: red; }"
>   "</style></head><body><select id='one'>" +
>   '  <option>{"color": "rgb(0, 0, 255)", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>' +
>   '  <option class="redColor">{"color": "rgb(255, 0, 0)", "backgroundColor": "rgba(0, 0, 0, 0)"}</option>' +
>   '  <option selected="true">{"end": "true"}</option>' +
>   "</select></body></html>";

> add_task(async function test_select_inherited_colors_on_options_dont_get_unique_rules_if_rule_set_on_select() {
>   let options = {
>     selectColor: "rgb(0, 0, 255)",
>     selectBgColor: "rgb(255, 255, 255)",
>     leaveOpen: true
>   };
> 
>   await testSelectColors(SELECT_INHERITED_COLORS_ON_OPTIONS_DONT_GET_UNIQUE_RULES_IF_RULE_SET_ON_SELECT, 3, options);
> 
>   let stylesheetEl = document.getElementById("ContentSelectDropdownStylesheet");
>   let sheet = stylesheetEl.sheet;
>   /* Check that there are no rulesets for the first option, but that
>      one exists for the second option and sets the color of that
>      option to "rgb(255, 0, 0)" */
> });


[1] http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/modules/SelectParentHelper.jsm#63-69

[2] http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/modules/SelectParentHelper.jsm#291-295
Flags: needinfo?(jaws)
In that last code snippet, at the end of the test you'll need to close the popup and remove the tab since `leaveOpen: true` is used.

>  await hideSelectPopup(selectPopup, "escape");
>  await BrowserTestUtils.removeTab(gBrowser.selectedTab);
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> At [1] is where we set the color of the <select>.
> 
> Then at [2] is where we set the color of each child <option>.
> 
> To fix this bug all that we should need to do is pass the color of the
> select to populateChildren(), then only create the rule-specific style if
> the color of the <option> is different than the <select>.

and we need to specify the color: inherit in the menuitem (like mentioned in comment 0), because either menuitem doesn't inherit color from menupopup intentionally, or there's something existing that overrides it.


> background-color and text-shadow don't inherit by default so we don't need
> to do this special case for them.

Yeah, this happens right now, but I believe that this is a bug. In the non-e10s case we do inherit background-color in the <option> elements, and also Chrome does that too.  (except on OSX where neither Chrome nor Safari supports any CSS styling)
Ok, I don't know who's the right reviewer, but since Felipe's :nick doesn't autocomplete in mozreview, I'm going with jaws. :)
Comment on attachment 8896515 [details]
Bug 1386015 - Do not generate styling for each element with inherited color.

https://reviewboard.mozilla.org/r/167770/#review173526

Nice work.

r=me with the messages added to the is() calls.

::: browser/base/content/test/forms/browser_selectpopup_colors.js:517
(Diff revision 2)
> +      }
> +    }
> +    return false;
> +  }
> +
> +  is(hasMatchingRule(sheet.cssRules, option1), false);

Please include the third argument, a message explaining what this is expecting.

is(actual, expected, message)

::: browser/base/content/test/forms/browser_selectpopup_colors.js:518
(Diff revision 2)
> +  is(hasMatchingRule(sheet.cssRules, option2, {
> +    color: "red"
> +  }), true);

Please include a message here too.

::: toolkit/modules/SelectParentHelper.jsm:83
(Diff revision 2)
>      if (ruleBody) {
>        sheet.insertRule(`#ContentSelectDropdown > menupopup {
>          ${ruleBody}
>        }`, 0);
> +      sheet.insertRule(`#ContentSelectDropdown > menupopup > :not([_moz-menuactive="true"]) {
> +         color: inherit;

I looked into why we need to set color:inherit; here and it's because we have a rule for:

menulist > menupopup > menuitem {
  color: -moz-FieldText;
}
Attachment #8896515 - Flags: review?(jaws) → review+
:jaws - I had to update the test a bit since it didn't work correctly. I'm now waiting for the try run before landing it.
(the el.matches(selector) didn't work correctly between contexts, so now I"m just testing for selectors with `:nth-child(${index})`.

The patch improves the time of opening the select with 3000 options on the reference laptop from ~11-12 seconds including "Not Responding" prompt, to below 2 seconds. :)
Comment on attachment 8896515 [details]
Bug 1386015 - Do not generate styling for each element with inherited color.

I started testing the patch on Mac and Windows, and there were a few things that needed a bit more of a refactor as well.

Here's the list:

1) On Mac, we did not populate the menuitems with the background-color of the <select> if `background-color` was the only rule set on the select. If you added `color` to the <select>, both were suddenly populated into the menu.
2) The `-moz-appearance` was removed even if only `color` has been applied to <option> resulting in the option with color being of different background on Mac than other options.
3) `text-shadow` would add 2 CSS rules per element, which doubled the cost
4) There were several checks that were checking against wrong values to prevent color and background ending up in the same color.

I fixed those and also added the same check for `text-shadow` as I previously did for `color`.
In result all three style rules that are applicable to <select> are now optimized and do not create redundant CSS rules.

This speeds up the example with 3000 elements quite drastically on Windows and Mac from 10-12s to sub 2s. (linux doesn't have custom styling enabled).

Asking for re-review.
Attachment #8896515 - Flags: review+ → review?(jaws)
Attachment #8896515 - Flags: review?(jaws)
Comment on attachment 8896515 [details]
Bug 1386015 - Do not generate styling for each element with inherited color.

https://reviewboard.mozilla.org/r/167770/#review174458

::: toolkit/modules/SelectParentHelper.jsm:109
(Diff revision 14)
>      }
>  
>      if (customStylingEnabled &&
>          selectTextShadow != "none") {
>        ruleBody += `text-shadow: ${selectTextShadow};`;
> +      sheet.insertRule(`#ContentSelectDropdown > menupopup > [_moz-menuactive="true"] {

Can you file a bug to add a class to the menuitems and menucaptions so that we don't need to write such a long selector with the wildcard at the end of it? This way the selector can just be `.content-select-item[_moz-menuactive="true"]` which should be faster than what we are doing now.

::: toolkit/modules/SelectParentHelper.jsm:123
(Diff revision 14)
> +    if (selectBackgroundSet) {
>        menulist.menupopup.setAttribute("customoptionstyling", "true");

Why are we now only setting "customoptionstyling" if a select background has been set? Won't we need this if `color` or `text-shadow` is used?

::: toolkit/modules/SelectParentHelper.jsm:397
(Diff revision 14)
> +    if (customStylingEnabled &&
> +        (optionBackgroundSet || selectBackgroundSet)) {
>        item.setAttribute("customoptionstyling", "true");

Why don't we want to do this for `color` or `text-shadow`?
Attachment #8896515 - Flags: review?(jaws) → review+
Filed bug 1390958.

> Why are we now only setting "customoptionstyling" if a select background has been set? Won't we need this if `color` or `text-shadow` is used?

"customoptionstyling" sets the `moz-appearance: none`. I tested it on Linux, Windows 10 and MacOS and it seems that the only change it causes is the change in background color (and maybe padding).
In result, the old behavior was changing the background-color of the item when you set the `text-shadow` or `color` of if (by removing the moz-appearance).

For example, setting `color: blue` on the select would change the background color of the popupmenu. And then it trickled onto the next problem:

> Why don't we want to do this for `color` or `text-shadow`?

That was inconsistent with how the custom styling works in Edge and Chrome and overall looked weird.

An Example:

```
  <select id='one'>
    <option>1</option>
    <option>2</option>
    <option style="color: blue">3</option>
    <option>4</option>
    <option>5</option>
    <option>6</option>
    <option>7</option>
    <option>8</option>
  </select>
```

On MacOS, in this example the 3rd option would suddenly have white background (due to customstyling on the whole select), and its onhover state would be different from other elements.

Let me know if you accept this change.
Flags: needinfo?(jaws)
Okay, thanks for explaining it. Can you put a more concise comment in the patch near the first place we set the "customoptionstyling" attribute? Maybe say, "customoptionstyling is only necessary for custom background colors".
Flags: needinfo?(jaws)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31a7f0775f92
Do not generate styling for each element with inherited color. r=jaws
https://hg.mozilla.org/mozilla-central/rev/31a7f0775f92
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8896515 [details]
Bug 1386015 - Do not generate styling for each element with inherited color.

Approval Request Comment
[Feature/Bug causing the regression]: it's been like that forever
[User impact if declined]: the performance of opening a long select lists with any styling is very bad
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really. 
[Why is the change risky/not risky?]: It landed in Nightly quite a while ago with no regressions reported
[String changes made/needed]: none
Attachment #8896515 - Flags: approval-mozilla-beta?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #32)
> [Feature/Bug causing the regression]: it's been like that forever

I want to emphasize that this has always been an e10s problem, and it doesn't affect non-e10s. So this has been true since Firefox 48 for users who received e10s, but as more and more people qualify for e10s, more will get exposed to the bug.
Comment on attachment 8896515 [details]
Bug 1386015 - Do not generate styling for each element with inherited color.

Improving perf on long select lists sounds good to me. And this has had a while to bake on nightly. 
Let's land this for beta 12.
Attachment #8896515 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out for browser_selectpopup_colors.js failures on OSX/Win. Maybe we should let this ride the 57 train given that the next opportunity for 56 is the RC build next week.
https://hg.mozilla.org/releases/mozilla-beta/rev/776744d895c9

https://treeherder.mozilla.org/logviewer.html#?job_id=130840620&repo=mozilla-beta
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf) → needinfo?(felipc)
arghh after investigation I found that what happened is that, on 56, the text shadow rule is serialized as:

text-shadow: 1px 1px 2px rgb(0, 0, 0)

whereas the test was expecting it to be in the opposite order:

text-shadow: rgb(0, 0, 0) 1px 1px 2px


I'm unsure about taking this only on RC, so I guess let it ride on 57.
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #37)
> arghh after investigation I found that what happened is that, on 56, the
> text shadow rule is serialized as: [...]
> whereas the test was expecting it to be in the opposite order:

FWIW, mozregression says that behavior changed (intentionally) in bug 1377541:
 https://hg.mozilla.org/mozilla-central/rev/9599a3a41754

(This doesn't make a difference for upliftability, except to emphasize that a test-tweak would be fine here if we did want to uplift.  But, agreed, it's pretty late in the game for any sort of uplift.)
Performance Impact: --- → P2
Whiteboard: [qf:p2]
You need to log in before you can comment on or make changes to this bug.