1371393 - Stylo: inDOMUtils::GetRuleColumn reports differently than Gecko

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

DevTools expects inDOMUtils::GetRuleColumn to report the first column for the rule, but Stylo appears to report the last column.

This leads to failures like:

* devtools/client/commandline/test/browser_cmd_csscoverage_oneshot.js[1]

Got {"reports":[{"selectorText":".page3-test2","start":{"line":9,"column":19}}]}, expected {"reports":[{"selectorText":".page3-test2","start":{"line":9,"column":5}}]}

(and more reports like this for the same test)

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=105301412&repo=try&lineNumber=2780

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The appears to also affect tests like:

* devtools/client/animationinspector/test/browser_animation_pseudo_elements.js[2]

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

[2]: https://treeherder.mozilla.org/logviewer.html#?job_id=105301412&repo=try&lineNumber=2419

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Recommending P1 for this, appears to pop up on more and more tests as I continue sifting.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1371393 - Stylo: set location for NestedRuleParser during prelude parsing.

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

Comment 4

[Simon Sapin (:SimonSapin)]

Comment on attachment 8877196 [details]
Bug 1371393 - Stylo: set location for NestedRuleParser during prelude parsing.

https://reviewboard.mozilla.org/r/148576/#review152996

I’d prefer not to modify the selectors crate for this, and do it instead in servo/components/style/stylesheets/rule_parser.rs in `<NestedRuleParser as QualifiedRuleParser>::parse_prelude`

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8877196 [details]
Bug 1371393 - Stylo: set location for NestedRuleParser during prelude parsing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148576/diff/1-2/

Comment 6

[Simon Sapin (:SimonSapin)]

Comment on attachment 8877196 [details]
Bug 1371393 - Stylo: set location for NestedRuleParser during prelude parsing.

https://reviewboard.mozilla.org/r/148576/#review153502

Looks good, thanks!

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Merged https://github.com/servo/servo/commit/7334298f3018f4be1b20bdaf58efd4f67beeda4b

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/mozilla-central/rev/f791b63f00bf