Scrollbars in trees are light with dark theme
Categories
(Core :: Layout, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox71 | --- | wontfix |
firefox72 | --- | wontfix |
firefox73 | --- | fixed |
People
(Reporter: Paenglab, Assigned: heycam)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [layout:triage-discuss])
Attachments
(2 files, 1 obsolete file)
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.
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
Alexander, Brian, could this be that this no more works because the scrollbars are in the shadow-root?
Updated•5 years ago
|
![]() |
||
Comment 2•5 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fe250cbde5794f31494bcba8dec4b6bb9f4aee0b&tochange=94b01ff1b1618e9d677a85783be045c9f464f30d
Updated•5 years ago
|
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Hmm, no activity here for almost three months. Don't you care that the Firefox UI looks inconsistent?
Comment 4•5 years ago
|
||
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 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
Taking a look.
Assignee | ||
Comment 8•5 years ago
|
||
We want to set the scrollbar colors in the sidebar with this rule:
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:
This works for regular scrollframes, but the xul:tree
component uses an explicit scrollbar
in its shadow tree:
which means we never find the scrollable element here:
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.
Assignee | ||
Comment 9•5 years ago
|
||
When the scrollbar
itself is not NAC, that is. (The child scrollbar part elements are NAC regardless.)
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Comment hidden (obsolete) |
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
Comment 15•5 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b9f93a02d6be Correctly style dark scrollbars in tree components. r=emilio
Comment 16•5 years ago
|
||
Backed out changeset b9f93a02d6be (Bug 1571285) for causing reftest failures at build/tests/reftest/tests/layout/reftests/svg/sizing/scrollbars-01.svg.
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280087806&repo=autoland&lineNumber=8351
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
Assignee | ||
Comment 17•5 years ago
|
||
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:
- Change all the other bare tag name scrollbar part rules to include something to bump up their specificity, e.g. turn
scrollbar
intoscrollbar:not([foo])
. - Add a bunch of
!important
s to those other rules. - 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.) - Add some new pseudo-class like
:-moz-native-anonymous-specificity-0
with specificity 0, to make it more obvious. - Implement
:where()
. - 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?
Assignee | ||
Comment 18•5 years ago
|
||
I think (4) might be least disruptive, but let me know what you think.
Assignee | ||
Comment 19•5 years ago
|
||
Comment hidden (obsolete) |
Comment 21•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7be7da6bb645 Correctly style dark scrollbars in tree components. r=emilio
Comment 24•5 years ago
|
||
bugherder |
Assignee | ||
Comment 26•5 years ago
|
||
It's a minor issue so I am fine with it riding the trains.
Updated•5 years ago
|
Description
•