Implement CSSStyleRule.selectorText. #17538

jyc · 2017-06-27T23:08:38Z

We parse when assigning using the namespaces of the stylesheet. It isn't
clear if the spec says to do that (Firefox doesn't support the setter at
all, Chrome does, Safari doesn't); the spec issue is here:
w3c/csswg-drafts#1511

Also fix ToCss implementation of AttrSelectorOperator to not pad with
spaces, to conform with CSSOM. This means we have to update some unit
tests that expect operators with spaces around them in attribute
selectors to roundtrip.

See the "attribute selector" section of "Serializing Selectors" here:
https://drafts.csswg.org/cssom/#serializing-selectors

CSSStyleRule.selectorText is specified here:
https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes is part of a series to fix Simplify an+b when serializing #17182

I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well.

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-06-27T23:08:44Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/cssstyledeclaration.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl
@KiChjang: components/script/dom/cssstyledeclaration.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl

jyc · 2017-06-30T00:10:06Z

Oh, the tests are those from #17203 . Will wait for that.

jyc · 2017-07-06T20:51:31Z

Neat! So I believe that once is this landed we should start seeing changes on results in

agh.. this is hard to organize. Wish GitHub had some way to make Pull Requests composed of Pull Requests.

jyc · 2017-07-11T22:17:44Z

@Manishearth if you could review this when you have time, that would be cool! (then could land the other parts). But I think we were still waiting on the other patch to remove spacing around operators in selectors; I've included those changes so that the checks pass here because it looked like that received a 'r-', but I can remove them easily as well.

Manishearth · 2017-07-11T22:23:33Z

Yeah, just include that patch in your PR and fix it up.

jyc · 2017-07-11T22:24:23Z

@Manishearth cool! It is done!

Manishearth · 2017-07-11T22:26:29Z

@bors-servo r+

bors-servo · 2017-07-11T22:26:29Z

📌 Commit 42ec33e has been approved by Manishearth

bors-servo · 2017-07-11T22:26:30Z

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

bors-servo · 2017-07-12T01:25:16Z

⌛ Testing commit 42ec33e with merge a9414a2...

Implement CSSStyleRule.selectorText. We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182  I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17538)

bors-servo · 2017-07-12T01:55:16Z

💔 Test failed - linux-rel-wpt

jyc · 2017-07-12T03:59:46Z

Aggh! I remember this error from when this was one PR:

{"status": "FAIL", "group": "default", "message": "sheet.cssRules[(sheet.cssRules.length - 1)] is undefined", "stack": "assert_selector_serializes_to@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:24:17\n@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:27:17\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20\ntest@http://web-platform.test:8000/resources/testharness.js:497:9\n@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:26:13\n", "subtest": "Simple type selector", "test": "/cssom/serialize-namespaced-type-selectors.html", "line": 103522, "action": "test_result", "expected": "PASS"}
{"status": "FAIL", "group": "default", "message": "sheet.cssRules[(sheet.cssRules.length - 1)] is undefined", "stack": "assert_selector_serializes_to@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:24:17\n@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:219:17\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20\ntest@http://web-platform.test:8000/resources/testharness.js:497:9\n@http://web-platform.test:8000/cssom/serialize-namespaced-type-selectors.html:218:13\n", "subtest": "Type selector with namespace equal to default namespace", "test": "/cssom/serialize-namespaced-type-selectors.html", "line": 103570, "action": "test_result", "expected": "PASS"}

I think that depends on #17501 . The test should actually not be passing right now, seeing as selectorText is unimplemented.

I will update the additional unexpected passes and maybe we can update the expectations to bec orrect for the unexpected fail when it should actually be passing, after merging #17501.

jyc · 2017-07-12T04:03:45Z

@bors-servo: try

bors-servo · 2017-07-12T04:42:38Z

💔 Test failed - mac-rel-css2

jyc · 2017-07-12T05:59:25Z

@bors-servo: try

bors-servo · 2017-07-12T05:59:29Z

⌛ Trying commit a830021 with merge 2926ff6...

Implement CSSStyleRule.selectorText. We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182  I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17538)

We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext

jyc · 2017-07-12T08:20:42Z

@bors-servo: try

bors-servo · 2017-07-12T08:20:47Z

⌛ Trying commit 92ec8f1 with merge a3bbbca...

Implement CSSStyleRule.selectorText. We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182  I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17538)

bors-servo · 2017-07-12T09:10:52Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

jyc · 2017-07-12T17:41:22Z

@bors-servo r=Manishearth

bors-servo · 2017-07-12T17:41:23Z

@jyc: 🔑 Insufficient privileges: Not in reviewers

canova · 2017-07-12T17:46:37Z

@bors-servo r=Manishearth

bors-servo · 2017-07-12T17:46:37Z

📌 Commit 92ec8f1 has been approved by Manishearth

bors-servo · 2017-07-12T18:08:37Z

⌛ Testing commit 92ec8f1 with merge 4b6e793...

Implement CSSStyleRule.selectorText. We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes is part of a series to fix #17182  I am running this to identify what tests will fail; as the other PRs in the series are merged, I believe the tests that pass will change as well. - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17538)

bors-servo · 2017-07-12T19:14:40Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: Manishearth
Pushing 4b6e793 to master...

We parse when assigning using the namespaces of the stylesheet. It isn't clear if the spec says to do that (Firefox doesn't support the setter at all, Chrome does, Safari doesn't); the spec issue is here: w3c/csswg-drafts#1511 Also fix ToCss implementation of AttrSelectorOperator to not pad with spaces, to conform with CSSOM. This means we have to update some unit tests that expect operators with spaces around them in attribute selectors to roundtrip. See the "attribute selector" section of "Serializing Selectors" here: https://drafts.csswg.org/cssom/#serializing-selectors CSSStyleRule.selectorText is specified here: https://drafts.csswg.org/cssom/#dom-cssstylerule-selectortext Upstreamed from servo/servo#17538 [ci skip]

highfive assigned Manishearth Jun 27, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 27, 2017

jyc force-pushed the selectorText branch from 51c2f8f to 14c13b1 Compare June 29, 2017 17:32

ferjm requested review from cbrewster and removed request for cbrewster June 30, 2017 06:56

jyc force-pushed the selectorText branch 3 times, most recently from 7822dd4 to 42ec33e Compare July 5, 2017 23:19

jyc changed the title ~~[WIP] Implement CSSStyleRule.selectorText.~~ Implement CSSStyleRule.selectorText. Jul 11, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 11, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 12, 2017

jyc force-pushed the selectorText branch from 42ec33e to 052de9f Compare July 12, 2017 04:03

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 12, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2017

jyc force-pushed the selectorText branch from 052de9f to a830021 Compare July 12, 2017 05:58

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 12, 2017

jyc force-pushed the selectorText branch from a830021 to 92ec8f1 Compare July 12, 2017 08:08

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 12, 2017

bors-servo merged commit 92ec8f1 into servo:master Jul 12, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 12, 2017

SimonSapin mentioned this pull request Aug 14, 2017

Do not add spaces when serializing operators in attribute selectors #17181

Closed

This was referenced Oct 6, 2017

Implement CSSStyleRule.selectorText. web-platform-tests/wpt#7613

Closed

Implement CSSStyleRule.selectorText. web-platform-tests/wpt#7698

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CSSStyleRule.selectorText. #17538

Implement CSSStyleRule.selectorText. #17538

jyc commented Jun 27, 2017 •

edited by larsbergstrom

highfive commented Jun 27, 2017

jyc commented Jun 30, 2017

jyc commented Jul 6, 2017

jyc commented Jul 11, 2017

Manishearth commented Jul 11, 2017

jyc commented Jul 11, 2017

Manishearth commented Jul 11, 2017

bors-servo commented Jul 11, 2017

bors-servo commented Jul 11, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

canova commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

Implement CSSStyleRule.selectorText. #17538

Implement CSSStyleRule.selectorText. #17538

Conversation

jyc commented Jun 27, 2017 • edited by larsbergstrom

highfive commented Jun 27, 2017

jyc commented Jun 30, 2017

jyc commented Jul 6, 2017

jyc commented Jul 11, 2017

Manishearth commented Jul 11, 2017

jyc commented Jul 11, 2017

Manishearth commented Jul 11, 2017

bors-servo commented Jul 11, 2017

bors-servo commented Jul 11, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jul 12, 2017

bors-servo commented Jul 12, 2017

canova commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

bors-servo commented Jul 12, 2017

jyc commented Jun 27, 2017 •

edited by larsbergstrom