Closed
Bug 1377115
Opened 7 years ago
Closed 7 years ago
stylo: re-enable layout/style/test/test_variables.html
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
Attachments
(1 file)
Bug 1375555 introduced an intermittent failure for layout/style/test/test_variables.html and I had to disable it in bug 1377068. This issue is to track the work to re-enable it. My theory is that we still don't preserve a consistent order for custom properties, because the patches for bug 1375555 only enforce this order for the computed values map. We still need to enforce an order to the specific values map and keep it while building the final map of applicable custom properties.
Assignee | ||
Updated•7 years ago
|
Comment 1•7 years ago
|
||
It seems to only fail at test5, where we sometimes return '--a' for the last item in declaration block from getComputedStyle. However, I don't think this is really about the order. In this test, the element #t5 shouldn't have any rule with '--a' applied at all, which means there must be something we are messing up more fundamentally.
I tried to open inspector and see what rules are applied, I get:
> console.error:
> Message: Error: couldn't find start of the rule
> Stack:
> getRuleText@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1579:13
> getAuthoredCssText/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1239:20
> process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
> walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
> Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
> schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
> Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
> _handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:387:7
> _run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:319:13
> TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
> asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
> handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
> onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1799:15
> receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7
> getRuleText@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1579:13
> getAuthoredCssText/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1239:20
> process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
> walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
> Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
> schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
> Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
> _handleResultValue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:387:7
> _run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:319:13
> TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
> asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
> handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
> onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1799:15
> receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7
> console.error:
> Protocol error (unknownError): couldn't find start of the rule
> console.error:
> Protocol error (unknownError): couldn't find start of the rule
> console.error:
> Protocol error (unknownError): couldn't find start of the rule
which might be another indication of something going wrong.
Comment 2•7 years ago
|
||
Hmmm, okay, it is the external style sheet which includes a rule applied to #t5 with --a.
Comment 3•7 years ago
|
||
It doesn't seem to me that the spec says anything about order of custom properties in computed value. I guess not having a stable order may not be a too serious issue at the moment. We can just change the external style sheet to only apply to #t4 instead, so that we don't fail at test5.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
If we want to fix bug 1379577, we should probably add another test specifically for testing the order, rather than relying on random existing test to check so.
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8887778 [details] Bug 1377115 - Enable test_variables.html. https://reviewboard.mozilla.org/r/158706/#review164512
Attachment #8887778 -
Flags: review?(cam) → review+
Comment 7•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s a6d5cfb86e6f -d 6d506550be89: rebasing 408100:a6d5cfb86e6f "Bug 1377115 - Enable test_variables.html. r=heycam" (tip) merging layout/style/test/mochitest.ini warning: conflicts while merging layout/style/test/mochitest.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/44c251302acc Enable test_variables.html. r=heycam
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44c251302acc
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
•