Closed
Bug 1379577
Opened 7 years ago
Closed 7 years ago
stylo: computed style indexed property access to custom property names is intermittently wrong
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: heycam, Assigned: ferjm)
References
Details
Attachments
(3 files)
I was looking at the last two remaining failures in test_variables.html today, which are looking at item() calls and indexed getter calls on a computed style object, looking for a custom property name. This seems to only intermittently fail for me locally, which is a bit odd, since it doesn't look like there's anything timing dependent in the test.
Comment 1•7 years ago
|
||
ni? Fer, who looked at this not long ago... Actually I'm pretty sure I reviewed a patch for this, not sure if landed.
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 2•7 years ago
|
||
Yes, I have a WIP patch for this. I just need to fight with Rust lifetimes to make it work.
Assignee: nobody → ferjmoreno
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 3•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 4•7 years ago
|
||
I took this and did https://github.com/emilio/servo/tree/ordered-map so it builds. Still probably needs adjustments, since I don't think that the usage of HashMap for specified values guarantees that the key isn't contained on insertion, but should be a nice starting point.
Assignee | ||
Comment 5•7 years ago
|
||
Thank you Emilio! I completed the patch and opened https://github.com/servo/servo/pull/17973
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8893787 [details] Bug 1379577 - stylo: test custom properties computed values consistent order. https://reviewboard.mozilla.org/r/164900/#review170482 ::: layout/style/test/test_variables_order.html:32 (Diff revision 1) > + ["--SomeVariableName", "--a"].forEach((varName, index) => { > + is(cs.item(cs.length - (index + 1)), varName, "custom property name returned by item() on computed style"); > + is(cs[cs.length - (index + 1)], varName, "custom property name returned by indexed getter on style declaration"); > + }); So does this mean that the order is the order they are cascaded in? Given that the spec doesn't say what to do now (I think?) can you add a comment to say that we have this test just to prevent regressions, rather than testing spec-mandated behavior?
Attachment #8893787 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8894455 -
Flags: review?(cam)
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #7) > So does this mean that the order is the order they are cascaded in? Given > that the spec doesn't say what to do now (I think?) can you add a comment to > say that we have this test just to prevent regressions, rather than testing > spec-mandated behavior? Yes. I added the comment to the test. Thanks!
Reporter | ||
Comment 11•7 years ago
|
||
Comment on attachment 8894455 [details]
Servo PR
(See PR for comments.)
Attachment #8894455 -
Flags: review?(cam) → review+
Comment 12•7 years ago
|
||
Pushed by ferjmoreno@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c7dde633fa86 stylo: test custom properties computed values consistent order. r=heycam
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7dde633fa86
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•