Handle properly alternate stylesheet #14911

charlesvdv · 2017-01-07T17:14:51Z

Alternate stylesheet are now fetched and then desactivated instead of ignoring them.

I don't know if tests are required to check if the stylesheet is correctly loaded and disabled ?

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14881 (github issue number if applicable).

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

This change is

highfive · 2017-01-07T17:14:56Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @cbrewster (or someone else) soon.

highfive · 2017-01-07T17:14:57Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmllinkelement.rs, components/script/stylesheet_loader.rs
@KiChjang: components/script/dom/htmllinkelement.rs, components/script/stylesheet_loader.rs

highfive · 2017-01-07T17:14:58Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2017-01-07T18:26:19Z

An automated reference test that verifies that the style present in an alternate stylesheet is not applied would be valuable. It could be created in tests/wpt/mozilla/tests/css.

jdm · 2017-01-07T18:28:21Z

components/script/stylesheet_loader.rs

@@ -147,6 +150,9 @@ impl FetchResponseListener for StylesheetContext {
                }
                StylesheetContextSource::Import(ref import) => {
                    let import = import.read();
+                    if elem.downcast::<HTMLLinkElement>().unwrap().is_alternate() {


I suspect this will break if we have <style>@import 'foo.css'</style>, since the element is a HTMLStyleElement. Try running ./mach test-wpt tests/wpt/mozilla/tests/mozilla/restyle-out-of-document.html to verify this.

The test pass.

But it's true that's bizarre. As style also support alternate I should also create a method is_alternate for the Import type ?

My mistake, that test doesn't include any stylesheets. Try ./mach test-wpt tests/wpt/mozilla/tests/mozilla/stylesheet-adopt-panic.html instead.

My reading of the spec doesn't include any way of specifying a style element as an alternate stylesheet. Where did you see that?

Indeed it fails. I will rebase with the change.

And indeed it say alternate flags Unset. So I guess it's not a flag after all.

charlesvdv · 2017-01-07T18:37:07Z

@jdm this test is already done by #10319

jdm · 2017-01-07T18:46:35Z

You're right!

jdm · 2017-01-08T16:39:00Z

@charlesvdv Could you modify the test from #10319 to verify that imported stylesheets from alternate stylesheets do not get applied?

charlesvdv · 2017-01-08T17:47:29Z

@jdm the test pass without code modification.

cbrewster · 2017-01-09T21:27:48Z

@bors-servo r+

bors-servo · 2017-01-09T21:27:49Z

📌 Commit fc149a8 has been approved by cbrewster

bors-servo · 2017-01-10T03:34:47Z

⌛ Testing commit fc149a8 with merge 1e871ea...


        Auto merge of #14911 - charlesvdv:alternate-stylesheet, r=cbrewster

Handle properly alternate stylesheet  Alternate stylesheet are now fetched and then desactivated instead of ignoring them. I don't know if tests are required to check if the stylesheet is correctly loaded and disabled ?  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14881 (github issue number if applicable).  - [ ] 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/14911)

bors-servo · 2017-01-10T04:10:30Z

💔 Test failed - mac-rel-wpt1

jdm · 2017-01-10T08:52:32Z

@bors-servo: retry

#14323

bors-servo · 2017-01-10T09:17:04Z

⌛ Testing commit fc149a8 with merge d5795f7...

bors-servo · 2017-01-10T09:54:44Z

💔 Test failed - linux-rel-wpt

jdm · 2017-01-10T10:41:07Z


  ▶ OK [expected TIMEOUT] /subresource-integrity/subresource-integrity.sub.html

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected NOTRUN] Style: Same-origin with incorrect sha256 and sha512 hash, rel='alternate stylesheet' enabled

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  │ FAIL [expected NOTRUN] Style: Same-origin with correct sha256 and sha512 hash, rel='alternate stylesheet' enabled
  │   → assert_equals: expected "rgb(255, 0, 0)" but got "rgba(0, 0, 0, 0)"
  │ 
  │ SRIStyleTest.prototype.execute/</<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:127:21
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ SRIStyleTest.prototype.execute/<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:125:17

Good, we're fixing the test behaviour that made us file #14881!

charlesvdv · 2017-01-10T10:49:39Z

nice !👍 Should I check which test pass and enable them ?

emilio · 2017-01-10T14:01:05Z

@Charlesdv: yes :)


        Handle properly alternate stylesheet


        Modify test to check if @import stylesheet get applied in a alternate…

… stylesheet


        Update metadata tests

jdm · 2017-01-11T17:54:47Z

@bors-servo: r=cbrewster

bors-servo · 2017-01-11T17:54:48Z

📌 Commit cb9940f has been approved by cbrewster

bors-servo · 2017-01-11T18:34:05Z

⌛ Testing commit cb9940f with merge cfc74e2...


        Auto merge of #14911 - charlesvdv:alternate-stylesheet, r=cbrewster

Handle properly alternate stylesheet  Alternate stylesheet are now fetched and then desactivated instead of ignoring them. I don't know if tests are required to check if the stylesheet is correctly loaded and disabled ?  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14881 (github issue number if applicable).  - [ ] 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/14911)

bors-servo · 2017-01-11T19:31:11Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned cbrewster Jan 7, 2017

highfive added the S-awaiting-review label Jan 7, 2017

jdm requested changes Jan 7, 2017

View changes

jdm added the S-needs-tests label Jan 7, 2017

jdm removed the S-needs-tests label Jan 7, 2017

charlesvdv force-pushed the charlesvdv:alternate-stylesheet branch from b74e41a to d20695f Jan 7, 2017

jdm added S-needs-tests and removed S-awaiting-review labels Jan 8, 2017

highfive added the S-awaiting-review label Jan 8, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 9, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 10, 2017

highfive removed the S-tests-failed label Jan 10, 2017

highfive added the S-awaiting-merge label Jan 10, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 10, 2017

charlesvdv added 2 commits Jan 7, 2017

Handle properly alternate stylesheet

19e31d5

Modify test to check if @import stylesheet get applied in a alternate…

2bcefa6

… stylesheet

charlesvdv force-pushed the charlesvdv:alternate-stylesheet branch from fc149a8 to f25efc3 Jan 11, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jan 11, 2017

charlesvdv force-pushed the charlesvdv:alternate-stylesheet branch from f25efc3 to cb9940f Jan 11, 2017

Update metadata tests

Loading status checks…

cb9940f

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 11, 2017

bors-servo merged commit cb9940f into servo:master Jan 11, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 11, 2017

bors-servo mentioned this pull request Jan 11, 2017

Handle crossorigin in Link #14940

Merged

3 of 3 tasks complete

servo / servo

Handle properly alternate stylesheet #14911

Handle properly alternate stylesheet #14911

charlesvdv commented Jan 7, 2017 •

edited

highfive commented Jan 7, 2017

highfive commented Jan 7, 2017

highfive commented Jan 7, 2017

jdm commented Jan 7, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

charlesvdv commented Jan 7, 2017

jdm commented Jan 7, 2017

jdm commented Jan 8, 2017

charlesvdv commented Jan 8, 2017

cbrewster commented Jan 9, 2017

bors-servo commented Jan 9, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

jdm commented Jan 10, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

jdm commented Jan 10, 2017 •

edited

charlesvdv commented Jan 10, 2017

emilio commented Jan 10, 2017

jdm commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

servo / servo

Join GitHub today

Handle properly alternate stylesheet #14911

Handle properly alternate stylesheet #14911

Conversation

charlesvdv commented Jan 7, 2017 • edited

highfive commented Jan 7, 2017

highfive commented Jan 7, 2017

highfive commented Jan 7, 2017

jdm commented Jan 7, 2017

This comment has been minimized.

jdm Jan 7, 2017

This comment has been minimized.

charlesvdv Jan 7, 2017

This comment has been minimized.

jdm Jan 7, 2017

This comment has been minimized.

charlesvdv Jan 7, 2017

charlesvdv commented Jan 7, 2017

jdm commented Jan 7, 2017

jdm commented Jan 8, 2017

charlesvdv commented Jan 8, 2017

cbrewster commented Jan 9, 2017

bors-servo commented Jan 9, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

jdm commented Jan 10, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

jdm commented Jan 10, 2017 • edited

charlesvdv commented Jan 10, 2017

emilio commented Jan 10, 2017

jdm commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

bors-servo commented Jan 11, 2017

charlesvdv commented Jan 7, 2017 •

edited

jdm commented Jan 10, 2017 •

edited