1340696 - stylo: need to support system colors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

  data:text/html,<textarea style="-moz-appearance: none">

It sure would be nice if the border were visible, say.  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

See also bug 1341697.

Comment 2

[Simon Sapin (:SimonSapin)]

This involves adding system color support in rust-cssparser, as a new enum variant:

    pub enum Color {
        RGBA(RGBA),
        CurrentColor,
        System(SystemColor),  // new
    }

… with SystemColor a C-like enum.

In Servo, make them map to white or black as suggested in https://github.com/servo/rust-cssparser/issues/2.

In Stylo:

* Add bindings for the LookAndFeel::ColorID enum
* Have a Rust mapping of SystemColor to ColorID
* Add bindings for (a function wrapper for) the LookAndFeel::GetColor method, which takes ColorID and returns nscolor
* Use that nscolor for setting computed values.

Comment 3

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1340696 - stylo: Support system colors;

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

Comment 4

[Manish Goregaokar [:manishearth]]

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

Form widgets still won't work well because most form widget UA declarations include a :-moz-system-metric pseudo, which we don't parse, and we end up ignoring the entire selector list.


There also are system fonts in these declarations, which are ... weird.

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847825 [details]
Bug 1340696 - stylo: Support system colors;

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8847825 [details]
Bug 1340696 - stylo: Support system colors;

https://reviewboard.mozilla.org/r/120742/#review122768

::: layout/style/ServoBindings.cpp:490
(Diff revision 2)
>  {
>    nsRuleNode::FillAllMaskLists(*aLayers, aMaxLen);
>  }
>  
> +nscolor Gecko_GetLookAndFeelSystemColor(int32_t aId,
> +                                        RawGeckoPresContextBorrowed aPresContext) {

Nit: { on new line.

::: layout/style/ServoBindings.cpp:493
(Diff revision 2)
>  
> +nscolor Gecko_GetLookAndFeelSystemColor(int32_t aId,
> +                                        RawGeckoPresContextBorrowed aPresContext) {
> +  bool useStandinsForNativeColors = aPresContext && !aPresContext->IsChrome();
> +  nscolor result;
> +  LookAndFeel::ColorID colorId = (LookAndFeel::ColorID) aId;

Nit: static_cast rather than C-style cast.

::: servo/components/style/properties/longhand/color.mako.rs:59
(Diff revision 2)
> +    % if product == "gecko":
> +        <%
> +            # These are actually parsed. See nsCSSProps::kColorKTable
> +            system_colors = """activeborder activecaption appworkspace background buttonface

File a Servo issue to support these values as described by Simon?

::: servo/components/style/properties/longhand/color.mako.rs:103
(Diff revision 2)
> +            fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +                let s = match *self {
> +                    % for color in system_colors + extra_colors:
> +                        LookAndFeel_ColorID::eColorID_${to_rust_ident(color)} => "${color}",
> +                    % endfor
> +                    LookAndFeel_ColorID::eColorID_LAST_COLOR => "",

Do we need to serialize this as the empty string, or can we unreachable!() it?

Comment 7

[Manish Goregaokar [:manishearth]]

Servo PR at https://github.com/servo/servo/pull/15974 . Feel free to autoland this once that lands if I'm not around (I should be).

Issue filed for pure servo impl at https://github.com/servo/servo/issues/15973

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847825 [details]
Bug 1340696 - stylo: Support system colors;

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48f891a36a8f
stylo: Support system colors; r=heycam

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48f891a36a8f