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)

defect

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.
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)
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)
Blocks: 1377115
Attached patch WIPSplinter Review
Priority: -- → P2
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.
Thank you Emilio! I completed the patch and opened https://github.com/servo/servo/pull/17973
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+
Attached file Servo PR
Attachment #8894455 - Flags: review?(cam)
(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!
Comment on attachment 8894455 [details]
Servo PR

(See PR for comments.)
Attachment #8894455 - Flags: review?(cam) → review+
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c7dde633fa86
stylo: test custom properties computed values consistent order. r=heycam
https://hg.mozilla.org/mozilla-central/rev/c7dde633fa86
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: