Closed Bug 1336891 Opened 7 years ago Closed 7 years ago

stylo: getComputedStyle returns empty for custom property

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: ferjm)

References

Details

Attachments

(2 files)

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)
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
More specifically, Servo_GetStyleVariables returns an empty style variable structure.
Assignee: nobody → josh
Assignee: josh → nobody
Assignee: nobody → ferjmoreno
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 on attachment 8880346 [details]
Bug 1336891 - stylo: update test expectations.

https://reviewboard.mozilla.org/r/151712/#review156688
Attachment #8880346 - Flags: review?(emilio+bugs) → review+
(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
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
https://hg.mozilla.org/mozilla-central/rev/05a2d6ac631c
https://hg.mozilla.org/mozilla-central/rev/71a1dace62f9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: