1571285 - Scrollbars in trees are light with dark theme

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Richard Marti (:Paenglab)]

I don't know since when but I think since the tree de-XBL, the scrollbar-color does no more apply to the scrollbars in trees.

Good visible in the History sidebar with the dark theme enabled.

Comment 1

[Richard Marti (:Paenglab)]

Alexander, Brian, could this be that this no more works because the scrollbars are in the shadow-root?

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fe250cbde5794f31494bcba8dec4b6bb9f4aee0b&tochange=94b01ff1b1618e9d677a85783be045c9f464f30d

Comment 3

[Jorg K (CEST = GMT+2)]

Hmm, no activity here for almost three months. Don't you care that the Firefox UI looks inconsistent?

Comment 4

[Jorg K (CEST = GMT+2)]

Looks like this fell through the triaging cracks. I'll take the liberty to assign a priority now: Visible regression that's in multiple released versions, that looks like a P1 to me. Sean, do you agree?

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: bad-scroll-bar.png

Clearly visible.

Comment 6

[Sean Voisen (:svoisen)]

Hey Cameron — Will you have bandwidth to take a look at this soon? Looks like a regression from the previous work to cache computed styles.

Comment 7

[Cameron McCormack (:heycam)]

Taking a look.

Comment 8

[Cameron McCormack (:heycam)]

We want to set the scrollbar colors in the sidebar with this rule:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/themes/shared/places/sidebar.inc.css#18-20

We handle scrollbar-color by walking up to the scrollable element to get its value. This is why we don't need to consider different inherited values for scrollbar style caching:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/servo/components/style/properties/longhands/inherited_ui.mako.rs#80-83

This works for regular scrollframes, but the xul:tree component uses an explicit scrollbar in its shadow tree:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/toolkit/content/widgets/tree.js#601-603

which means we never find the scrollable element here:

https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/layout/base/nsLayoutUtils.cpp#10085-10090

Scrollbar elements outside of NAC are rare, so I think the simplest thing is to inhibit the scrollbar style caching when we're not NAC.

Comment 9

[Cameron McCormack (:heycam)]

When the scrollbar itself is not NAC, that is. (The child scrollbar part elements are NAC regardless.)

Comment 10

[Cameron McCormack (:heycam)]

Actually it's not the caching of scrollbar styles that causes a problem here. It's the rules in minimal-xul.css that override the values of most inherited properties on scrollbar parts, including scrollbar-color! We don't want to do that for non-NAC scrollbar part elements. If we stick :-moz-native-anonymous on those rules, then we can leave nsLayoutUtils::StyleForScrollbar to find the scrollbar, and not its parent, for the scrollbar styles.

Comment 11

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a23013ee389695a7ce34e8caf05e7c7174c5763

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1571285 - Correctly style dark scrollbars in tree components.

Comment 13

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6c14fafe84631b3a4e876e5fbad199b40f4dbbb

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

69 regression, wontfix 71.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9f93a02d6be
Correctly style dark scrollbars in tree components. r=emilio

Comment 16

[Daniel Varga [:dvarga]]

Backed out changeset b9f93a02d6be (Bug 1571285) for causing reftest failures at build/tests/reftest/tests/layout/reftests/svg/sizing/scrollbars-01.svg.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&searchStr=reftest&revision=b9f93a02d6bede03725cab3f237abac8b1d317f8

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280087806&repo=autoland&lineNumber=8351

Backout log: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&searchStr=reftest&revision=45f8fa7f52144d1be1ea8457ce195d58842c31bf

Reftest analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/T-iDWmuGT5q6mKh9cm89Yg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Browser-chrome failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280092633&repo=autoland&lineNumber=2693

Mochitest failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280088285&repo=autoland&lineNumber=5413

[task 2019-12-07T00:58:55.240Z] 00:58:55     INFO - TEST-START | widget/tests/test_scrollbar_colors.html
[task 2019-12-07T00:58:55.301Z] 00:58:55     INFO - GECKO(2764) | [Parent 2764, Main Thread] WARNING: we only accept nsIURI interface type, patch welcome: file /builds/worker/workspace/build/src/dom/ipc/PropertyBagUtils.cpp, line 112
[task 2019-12-07T00:58:55.480Z] 00:58:55     INFO - TEST-INFO | started process screentopng
[task 2019-12-07T00:58:56.003Z] 00:58:56     INFO - TEST-INFO | screentopng: exit 0
[task 2019-12-07T00:58:56.004Z] 00:58:56     INFO - TEST-UNEXPECTED-FAIL | widget/tests/test_scrollbar_colors.html | Pixel count of color 255,255,0 - got 8740, expected 8100
[task 2019-12-07T00:58:56.004Z] 00:58:56     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:325:16
[task 2019-12-07T00:58:56.004Z] 00:58:56     INFO -     @widget/tests/test_scrollbar_colors.html:95:9
[task 2019-12-07T00:58:56.004Z] 00:58:56     INFO -     Async*@widget/tests/test_scrollbar_colors.html:74:17
[task 2019-12-07T00:58:56.005Z] 00:58:56     INFO - TEST-PASS | widget/tests/test_scrollbar_colors.html | Pixel count of color 0,0,255 
[task 2019-12-07T00:58:56.005Z] 00:58:56     INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-12-07T00:58:56.005Z] 00:58:56     INFO - TEST-UNEXPECTED-FAIL | widget/tests/test_scrollbar_colors.html | Pixel count of color 0,255,255 - got 540, expected 1180
[task 2019-12-07T00:58:56.005Z] 00:58:56     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:325:16
[task 2019-12-07T00:58:56.005Z] 00:58:56     INFO -     @widget/tests/test_scrollbar_colors.html:95:9
[task 2019-12-07T00:58:56.006Z] 00:58:56     INFO -     Async*@widget/tests/test_scrollbar_colors.html:74:17
[task 2019-12-07T00:58:56.006Z] 00:58:56     INFO - GECKO(2764) | MEMORY STAT | vsize 2598MB | residentFast 202MB | heapAllocated 27MB

Comment 17

[Cameron McCormack (:heycam)]

The failures are because the addition of the :-moz-native-anonymous to the scrollbar part UA style sheet rules increased their specificity, meaning other rules with bare tag name selectors like scrollbar are overridden. If we implemented :where(), I would use that. Alternatives:

1. Change all the other bare tag name scrollbar part rules to include something to bump up their specificity, e.g. turn scrollbar into scrollbar:not([foo]).
2. Add a bunch of !importants to those other rules.
3. As a special case make :-moz-native-anonymous have specificity 0. (Although there are other uses of this pseudo-class and I haven't checked what effect this would have on them.)
4. Add some new pseudo-class like :-moz-native-anonymous-specificity-0 with specificity 0, to make it more obvious.
5. Implement :where().
6. Implement a UA sheet only :-moz-where() that only takes a compound selector inside it, like :-moz-any does.

(5) would be best but I don't really have time for that right now. None of the others seem that great! Any preference Emilio?

Comment 18

[Cameron McCormack (:heycam)]

I think (4) might be least disruptive, but let me know what you think.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Bug 1571285 - Specificity hack.

Comment 20

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50e796f7a78bcfe986d67ab9e9cca9c206ff5d5c

Comment 21

[Emilio Cobos Álvarez (:emilio)]

We talked about this on IRC:

00:15 <@emilio> heycam: ping?
00:15 <heycam> emilio: hey
00:15 <@emilio> heycam: hey, which rules are fighting for specificity with scrollbars?
00:15 <@emilio> heycam: we only seem to have a `textarea > scrollbar` rule in UA sheets other than that, right?
00:15 <heycam> no there are others
00:16 — heycam grabs his other patch
00:16 <@emilio> heycam: Also mobile/android/themes/geckoview/content.css, I guess
00:17 <heycam> it's rules that have a single lone tag that are affected.  I didn't actually look at rules with more complex selectors but it's possible I'd need to look at some more
00:17 <heycam> emilio: https://paste.mozilla.org/A5gJr8a4
00:17 <heycam> is what I was testing with
00:18 <heycam> https://paste.mozilla.org/jHA57YGn#L for better highlighting...
00:18 → @birtles (opped) joined  
00:19 <heycam> emilio: oh yeah I missed the geckoview one
00:19 <heycam> (I didn't account for "xul|" when searching)
00:20 <heycam> emilio: incidentally how much work do you think implementing full :where() would be
00:21 <@emilio> heycam: so on one hand I think most of those rules should have `:-moz-native-anonymous`... On the other hand I guess that risks breaking the tree scrollbars even more...
00:21 <heycam> emilio: yeah I think I'm most inclined to take an option that leaves the specificity as is
00:21 <heycam> for that reason
00:21 <@emilio> heycam: I think the hardest part is doing the invalidation right when complex selectors are involved without too much over-invalidation
00:22 <@emilio> heycam: I think it's possible, but a bit annoying...
00:22 <@emilio> heycam: but probably not too terrible?
00:23 <heycam> emilio: yeah I figured the invalidation is where the trickiness would be.
00:23 <@emilio> heycam: also potentially tricky would be making the bloom filter setup work, though that's not too bad I suspect
00:23 <heycam> so I'm not going to look at that before I'm off in a couple of weeks :)
00:24 <heycam> though might make a good project for Erik?  since you've already got him looking at some invalidation related stuff
00:24 <@emilio> heycam: maybe!
00:24 <@emilio> heycam: he was going to look at constructable stylesheets, so maybe after that
00:24 <@emilio> ?
00:24 <@emilio> heycam: talking about invalidation... r? https://phabricator.services.mozilla.com/D55591 ;)
00:25 <heycam> emilio: yep your reviews are weighing on my mind, will look soon!
00:25 <heycam> :)
00:25 <@emilio> heycam: but yeah, I guess the zero-specificity pseudo-class hack makes sense given the circumstances...
00:25 <heycam> emilio: thanks / sorry :)
00:25 <@emilio> heycam: np :-)
00:27 heycam → heycam|away
00:29 <@emilio> heycam|away: r+'d your patch with a couple suggestions

TLDR the specificity hack seems ok (if unfortunate) for now.

Comment 22

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d31ddbd872a14aa84c8bad6f0a7dc9f9eeadaf9e&group_state=expanded

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7be7da6bb645
Correctly style dark scrollbars in tree components. r=emilio

Comment 24

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/7be7da6bb645

Comment 25

[Julien Cristau [:jcristau]]

Is this worth uplifting to beta?

Comment 26

[Cameron McCormack (:heycam)]

It's a minor issue so I am fine with it riding the trains.