stylo: re-enable layout/style/test/test_variables.html

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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 months ago
Assignee: nobody → ferjmoreno
Blocks: 1243581
Depends on: 1375555
Blocks: 1337599
(Assignee)

Updated

6 months ago
Depends on: 1379577
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.
Hmmm, okay, it is the external style sheet which includes a rule applied to #t5 with --a.
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)
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

6 months 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

6 months 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)

Comment 8

6 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44c251302acc
Enable test_variables.html. r=heycam

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44c251302acc
Status: NEW → RESOLVED
Last Resolved: 6 months 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.