stylo: getComputedStyle returns empty for custom property

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: xidorn, Assigned: ferjm)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Given the following code:
<!DOCTYPE html>
<p style="--foo:bar">hello</p>
<script>
  var p = document.querySelector('p');
  alert(getComputedStyle(p).getPropertyValue('--foo'));
</script>

It should show "bar", but stylo always shows empty string.

Probably the stylo glue code doesn't pass the computed value of custom properties to gecko.

Test test_variable_serialization_computed.html is for this.
Xidorn, are you going to take these kinds of bugs?
Flags: needinfo?(xidorn+moz)
(Reporter)

Comment 2

5 months ago
Maybe later.
Flags: needinfo?(xidorn+moz)
Component: DOM: CSS Object Model → CSS Parsing and Computation
Priority: -- → P2
[16:24:27]  <xidorn>	there is only one remaining issue as far as I know that, getComputedStyle doesn't return value for custom properties
[16:24:49]  <xidorn>	which is probably because we don't generate nsStyleVariables from ServoComputedValues
Blocks: 1243581

Comment 4

2 months ago
More specifically, Servo_GetStyleVariables returns an empty style variable structure.
Assignee: nobody → josh

Updated

a month ago
Assignee: josh → nobody
(Assignee)

Updated

a month ago
Assignee: nobody → ferjmoreno
Blocks: 1375230
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
Comment on attachment 8880345 [details]
Bug 1336891 - stylo: Implement custom property value getter.

https://reviewboard.mozilla.org/r/151710/#review156684

r=me, with those. Thanks!

We should get a bug on file for CalcStyleDifference, which is the other user of StyleVariables(). Please ensure there's one on file for it.

::: layout/style/nsComputedDOMStyle.cpp:6920
(Diff revision 1)
> -
>    nsString variableValue;
>    const nsAString& name = Substring(aPropertyName,
>                                      CSS_CUSTOM_NAME_PREFIX_LENGTH);
> -  if (!variables->mVariables.Get(name, variableValue)) {
> +  bool present = mStyleContext->IsServo() ?
> +      Servo_GetCustomProperty(mStyleContext->ComputedValues(),

nit: I think prevailing Gecko style would be something like:

```
const bool present = mStyleContext->IsServo()
  ? Servo_GetCustomProperty(..)
  : StyleVariables()->mVariables.Get(name, variableValue);
```

(But you can run it through clang-format if you want to confirm).

::: servo/ports/geckolib/glue.rs:3079
(Diff revision 1)
>  }
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_GetCustomProperty(computed_values: ServoComputedValuesBorrowed,
> +                                          name: *const nsAString, value: *mut nsAString) -> bool {
> +    match ComputedValues::as_arc(&computed_values).custom_properties() {

Let's reduce nesting from here:

```
let custom_props = match ComputedValues::as_arc(&computed_values).custom_properties() {
    Some(p) => p,
    None => return false,
};

let name = ..;
let value = match custom_props.get(&name) {
    Some(v) => v,
    None => return false,
};

value.to_css(unsafe { value.as_mut().unwrap() }).unwrap();
true
```

Also, the way you're creating an atom is really inefficient, you should be able to do:

```
Atom::from((*name).as_slice())
```

Which avoids re-encoding it as utf-8, copying into a string, and then converting that string back to utf-16, and then copying it again.
Attachment #8880345 - Flags: review?(emilio+bugs) → review+

Comment 8

a month ago
mozreview-review
Comment on attachment 8880346 [details]
Bug 1336891 - stylo: update test expectations.

https://reviewboard.mozilla.org/r/151712/#review156688
Attachment #8880346 - Flags: review?(emilio+bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a month ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> We should get a bug on file for CalcStyleDifference, which is the other user
> of StyleVariables(). Please ensure there's one on file for it.

Bug 1375536
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a month ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05a2d6ac631c
stylo: Implement custom property value getter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/71a1dace62f9
stylo: update test expectations. r=emilio

Comment 15

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05a2d6ac631c
https://hg.mozilla.org/mozilla-central/rev/71a1dace62f9
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.