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)
Core
CSS Parsing and Computation
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.
Comment 1•7 years ago
|
||
Xidorn, are you going to take these kinds of bugs?
Flags: needinfo?(xidorn+moz)
Updated•7 years ago
|
Component: DOM: CSS Object Model → CSS Parsing and Computation
Priority: -- → P2
Comment 3•7 years ago
|
||
[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
Comment 4•7 years ago
|
||
More specifically, Servo_GetStyleVariables returns an empty style variable structure.
Assignee: nobody → josh
Updated•7 years ago
|
Assignee: josh → nobody
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ferjmoreno
Blocks: 1375230
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05a2d6ac631c https://hg.mozilla.org/mozilla-central/rev/71a1dace62f9
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•