1460456 - Add some scrollbar color properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1460456 part 1 - Change the semantics of 'auto' color value.

Review commit: https://reviewboard.mozilla.org/r/242994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242994/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1460456 part 2 - Rename CaretColor to ColorOrAuto for reusing.

Review commit: https://reviewboard.mozilla.org/r/242996/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242996/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

Review commit: https://reviewboard.mozilla.org/r/242998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242998/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

Review commit: https://reviewboard.mozilla.org/r/243686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243686/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: demo screenshot

FWIW, this is what it currently looks like. The corner isn't colored because it is not rendered by widget code. I'll try making it a widget in a separate bug.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: demo screenshot with twitter dark mode color scheme

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

So I also realized one down side of using individual properties is that authors become unable to specify colors when user interact with the scrollbars (e.g. hover / click). But it's probably fine, and we probably should just derive those effect based on platform convention.

Current (Windows-only) implementation doesn't consider disabled scrollbar either, which we can fix later.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

I thought about whether this should be implemented in widget code or toolkit code (via setting properties on the scrollbar part elements). There are several reasons I decided to do it in widget code:

1) Setting properties on scrollbar part elements would require us to trigger some code on the scrollbar NAC tree for changes to those color properties. Since I don't think we can trigger restyle inside painting code, we probably need to trigger a reflow, which isn't great. If we want to avoid the extra overhead, we may need to introduce some new mechanism to invalidate scrollbar only, which doesn't seem to be worth it either.

2) Shape of scrollbars is pretty much platform-dependent, so putting the rendering part in widget code is probably better on keeping them close to their native implementation.

3) It seems that we may want to change how scrollbars are implemented at some point (bug 645563), so doing it in toolkit code may require non-trivial rework on the implementation, while doing it in widget code would be mostly orthogonal to that. (I suppose that even when we use a single frame for scrollbar, we would still need to invoke widget code for rendering native widgets). Although fixing bug 645563 would resolve (1), having this feature blocked on a long-standing refactor stuff doesn't seem to be a very efficient way going forward.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Also, as can be seen from the patch, currently only scrollbar-{face,track}-color are implemented, because this seems to be the most basic parts of scrollbars. Other stuffs are also platform-dependent, and probably derivable.

The Windows implementation here also tries to derive color for arrow, and it mostly looks good. We may or may not want to introduce a separate property for it.

Comment 10

[Cameron McCormack (:heycam)]

I assume that we are only targeting our own frontend use initially (hence them being marked chrome-only), and that we'll work out whether we need any further changes before exposing them to the web?

You might want to get feedback from :bgrins or others about exactly what styling features they need, if you haven't already.

Comment 11

[Cameron McCormack (:heycam)]

Handling this in the widget code sounds fine to me, btw.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8975393 [details]
Bug 1460456 part 1 - Change the semantics of 'auto' color value.

https://reviewboard.mozilla.org/r/242994/#review249512

::: layout/style/StyleComplexColor.h:31
(Diff revision 1)
>    // Whether the complex color represents a computed-value time auto
> -  // value. This is only a flag indicating that this value should not
> -  // be interpolatable with other colors, while other fields still
> -  // represents the actual used color of this value.
> +  // value. This is a flag indicating that this value should not be
> +  // interpolatable with other colors. When this flag is set, other
> +  // fields represent a currentcolor. Properties can decide whether
> +  // that should be used.

It might be better to update the comment above `struct StyleComplexColor` itself to say that this type can also represent an "auto" value, and to say that this auto value is only valid for some properties?

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8975394 [details]
Bug 1460456 part 2 - Rename CaretColor to ColorOrAuto for reusing.

https://reviewboard.mozilla.org/r/242996/#review249516

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

https://reviewboard.mozilla.org/r/242998/#review249520

r=me if we return "auto" from nsComputedDOMStyle for these properties.

::: gfx/src/nsITheme.h:77
(Diff revision 1)
>                                    uint8_t aWidgetType,
>                                    const nsRect& aRect,
>                                    const nsRect& aDirtyRect) = 0;
>  
>    /**
> +   * Get the used color of the given widget when it's specified to auto.

"specified as auto"

::: layout/style/nsComputedDOMStyle.cpp:1199
(Diff revision 1)
> +void
> +nsComputedDOMStyle::SetValueForWidgetColor(nsROCSSPrimitiveValue* aValue,
> +                                           const StyleComplexColor& aColor,
> +                                           uint8_t aWidgetType)
> +{
> +  if (!aColor.mIsAuto) {

Shouldn't we return "auto" rather than the color that the widget used, given that the spec says "normal" (as the keyword is currently named) is a computed value?

If we do keep this code that looks up the widget-provided color, we should make sure to return a fixed color when fingerprint resistance is turned on.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8975396 [details]
Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

https://reviewboard.mozilla.org/r/243686/#review249522

::: widget/windows/nsNativeThemeWin.cpp:1967
(Diff revision 1)
> +  // Fallback to background color for now. Do we want to derive from
> +  // scrollbar-face-color somehow?

Might be worth investigating how other browsers behave when the face color is specified and the track color isn't.

But I think some kind of derived color is probably going to be better than the background color, otherwise it's going to be hard to see where the scrolled element's background stops and the scrollbar begins.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

https://reviewboard.mozilla.org/r/242998/#review249520

I was thinking about this as well, but unfortunately we probably cannot. CSSOM spec explicitly listed `caret-color` (which is the only other usage of `auto | <color>`) in the Resolved Values section to indicate that it should be returning the used value.

Actually, it is tricky to define this from the perspective of spec. You don't want `currentcolor` keyword to be kept in resolved value for webcompat and consistency, so keeping `auto` as keyword doesn't look sensible then.

That's why I decided to add the method to get auto color from widget instead.

> Shouldn't we return "auto" rather than the color that the widget used, given that the spec says "normal" (as the keyword is currently named) is a computed value?
> 
> If we do keep this code that looks up the widget-provided color, we should make sure to return a fixed color when fingerprint resistance is turned on.

For auto keyword, see the argument above.

For fingerprint resistance, I can probably add a check in this function to return transparent directly when that flag is set. It's unclear to me whether code can still draw scrollbar on canvas to get the auto color, though.

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

https://reviewboard.mozilla.org/r/242998/#review249520

OK, that is fair enough.  I suppose it is pretty similar to system colors being exposed.

> For auto keyword, see the argument above.
> 
> For fingerprint resistance, I can probably add a check in this function to return transparent directly when that flag is set. It's unclear to me whether code can still draw scrollbar on canvas to get the auto color, though.

Although this makes me now wonder, do we actually handle system colors in some way when fingerprint resistance is turned on, and if so where?

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

https://reviewboard.mozilla.org/r/242998/#review249520

> Although this makes me now wonder, do we actually handle system colors in some way when fingerprint resistance is turned on, and if so where?

Hmm... It's probably worth filing a bug for investigation, I guess.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

https://reviewboard.mozilla.org/r/242998/#review249520

> Hmm... It's probably worth filing a bug for investigation, I guess.

Looking at the code, my guess is that pref "ui.use_native_colors" and "ui.use_standins_for_native_colors" would control that. We probably should respect those prefs somehow, although it isn't clear to me how immediately.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975393 [details]
Bug 1460456 part 1 - Change the semantics of 'auto' color value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242994/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975394 [details]
Bug 1460456 part 2 - Rename CaretColor to ColorOrAuto for reusing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242996/diff/1-2/

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975395 [details]
Bug 1460456 part 3 - Add scrollbar-{face,track}-color properties.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242998/diff/1-2/

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8975396 [details]
Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243686/diff/1-2/

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8975396 [details]
Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

FYI, we're working on remoting all of this code.. and the use of style contexts, graphics contexts, and frame pointers is going away. If you could avoid adding more functionality like this in the future would appreciate it.

Also note we're considering dumping native theming entirely, so a lot of this new functionality may be temporary in nature.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8975396 [details]
Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

https://reviewboard.mozilla.org/r/243686/#review251778

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Thanks for the review.

Could you elaborate a bit more about what do you mean by "remoting" the code, and "dumping native theming"? Do you have any pointer for the context?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/caf50de5e89b
part 1 - Change the semantics of 'auto' color value. r=heycam
https://hg.mozilla.org/integration/autoland/rev/97bd8f520796
part 2 - Rename CaretColor to ColorOrAuto for reusing. r=heycam
https://hg.mozilla.org/integration/autoland/rev/92f8c55b8279
part 3 - Add scrollbar-{face,track}-color properties. r=heycam
https://hg.mozilla.org/integration/autoland/rev/60382bab3a60
part 4 - Implement custom scrollbar color on Windows. r=jimm

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf4962739d38
followup - Add the new properties into animation-type-longhand on a CLOSED TREE.

Comment 29

[Jim Mathies [:jimm]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26)
> Thanks for the review.
> 
> Could you elaborate a bit more about what do you mean by "remoting" the
> code, and "dumping native theming"? Do you have any pointer for the context?

sure - 

https://docs.google.com/document/d/1eKlYHx2X8aqkcxl6hbQcKUvg3rhASRlKnj_o5HFjZ5Q/edit#
https://bugzilla.mozilla.org/show_bug.cgi?id=1381938#c2

Comment 30

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/caf50de5e89b
https://hg.mozilla.org/mozilla-central/rev/97bd8f520796
https://hg.mozilla.org/mozilla-central/rev/92f8c55b8279
https://hg.mozilla.org/mozilla-central/rev/60382bab3a60
https://hg.mozilla.org/mozilla-central/rev/bf4962739d38

Comment 31

[Chris Mills [:cmills]]

Can you give us a description of what this bug has added, and therefore what we need to document on MDN? It's not very clear from the bug - thanks!

Comment 32

[Xidorn Quan [:xidorn] UTC+11]

This bug adds scrollbar-face-color and scrollbar-track-color two properties, and the spec is https://drafts.csswg.org/css-scrollbars-1/#scrollbar-color-properties

They are currently behind a pref and only implemented on Windows and macOS. I planned to support it on Linux as well soon.

Comment 33

[Will Bamberg [:wbamberg]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1460456#c10, is it correct that this is not yet exposed to the web? And if so perhaps we should not document it yet?

Comment 34

[Xidorn Quan [:xidorn] UTC+11]

That's right that it is not yet exposed to the web.

Comment 35

[Chris Mills [:cmills]]

Bah, sorry, I missed that comment. Yeah, if this is not even exposed to the web yet, let's leave it for now.

Comment 36

[Sebastian Zartner [:sebo]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34)
> That's right that it is not yet exposed to the web.

They obviously *do* get exposed to the web when the preference is set to true. Therefore I've at least mentioned them already at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Experimental_features#scrollbar-properties.

Sebastian