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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

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

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, okay, it is the external style sheet which includes a rule applied to #t5 with --a.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1377115 - Enable test_variables.html.

The external sheet isn't meant to test the order of items. It is just
for testing handling of relative url in CSS variable, so it doesn't make
sense to have it affect the test for checking return value from item().

Review commit: https://reviewboard.mozilla.org/r/158706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158706/

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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

[Cameron McCormack (:heycam)]

Comment on attachment 8887778 [details]
Bug 1377115 - Enable test_variables.html.

https://reviewboard.mozilla.org/r/158706/#review164512

Comment 7

[Mozilla Autoland]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44c251302acc
Enable test_variables.html. r=heycam

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/44c251302acc