Add some scrollbar color properties

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

({dev-doc-needed})

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(6 attachments)

No description provided.
Priority: -- → P2
Posted image 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.
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.
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.
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.
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.
Handling this in the widget code sounds fine to me, btw.
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?
Attachment #8975393 - Flags: review?(cam) → review+
Comment on attachment 8975394 [details]
Bug 1460456 part 2 - Rename CaretColor to ColorOrAuto for reusing.

https://reviewboard.mozilla.org/r/242996/#review249516
Attachment #8975394 - Flags: review?(cam) → review+
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.
Attachment #8975395 - Flags: review?(cam) → review+
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 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 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 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 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.
Keywords: dev-doc-needed
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.
Attachment #8975396 - Flags: review?(jmathies) → review+
Comment on attachment 8975396 [details]
Bug 1460456 part 4 - Implement custom scrollbar color on Windows.

https://reviewboard.mozilla.org/r/243686/#review251778
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?
Flags: needinfo?(jmathies)
Pushed by xquan@mozilla.com:
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
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf4962739d38
followup - Add the new properties into animation-type-longhand on a CLOSED TREE.
(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
Flags: needinfo?(jmathies)
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!
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
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?
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(cmills)
That's right that it is not yet exposed to the web.
Flags: needinfo?(xidorn+moz)
Bah, sorry, I missed that comment. Yeah, if this is not even exposed to the web yet, let's leave it for now.
Flags: needinfo?(cmills)
(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
You need to log in before you can comment on or make changes to this bug.