Implement support for removing stylesheets from their document #14930

zaynetro · 2017-01-09T17:50:11Z

This pull request implements removing styles from the document when

<link> element with associated styles is removed
<style> element is removed

Additionally, it tests that when <style> element is changed. Styles are being reapplied correctly.

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

There are tests for these changes

This change is

highfive · 2017-01-09T17:50:15Z

Heads up! This PR modifies the following files:

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

Manishearth · 2017-01-09T17:56:09Z

I don't see any handling for style elements?

zaynetro · 2017-01-09T18:01:33Z

I was also surprised by it. Either the test is wrong or styles are recalculated in some other way, I hope you can help.

Manishearth · 2017-01-09T18:03:05Z

r? @SimonSapin

I don't know much about invalidation, I'm afraid.

SimonSapin · 2017-01-09T18:28:41Z

For <style> elements, I suspect that the test’s script updates/removes them before any styling is done, so there is nothing yet to invalidate. Try to use getComputedStyle before the update/removal to observe the previous style and force styling, and make sure the tests fail. Then adding or changing some Rust code should fix the tests.

Reviewed 3 of 3 files at r1, 5 of 5 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

tests/wpt/mozilla/tests/mozilla/remove_link_styles.css, line 3 at r2 (raw file):

body {
    background-color: orange;
    color: yellow;

Unless you need multiple different colors, please stick to red for incorrect and green for correct, so that someone debugging the test visually doesn’t need to look at the reference to see what color is expected.

tests/wpt/mozilla/tests/mozilla/remove_link_styles.html, line 5 at r2 (raw file):

<title>Removing link tag should remove associated styles</title>
<link rel="match" href="remove_link_styles_ref.html">
<link rel="stylesheet" type="text/css" href="remove_link_styles.css">

Since <link rel=stylesheet> happens before <style> and the two stylesheets have rule with identical specificity, the latter will "win" the cascade even before the stylesheet is removed. Either reverse their order, use a more specific selector, or use !important.

tests/wpt/mozilla/tests/mozilla/remove_link_styles.html, line 9 at r2 (raw file):

    <style>
        body {
            color: pink;

The color property is used, there is no text that would use it. Please add some text, preferably something descriptive that says what a successful result should look like.

tests/wpt/mozilla/tests/mozilla/remove_style_styles.html, line 8 at r3 (raw file):

    <style>
        body {
            background-color: purple;

Nit: use red for incorrect test results.

tests/wpt/mozilla/tests/mozilla/remove_style_styles_ref.html, line 4 at r3 (raw file):

<meta charset="utf-8">
<body>
</body>

Avoid completely empty/white test or reference files, since we can’t tell them apart from a potential bug where a screenshot is taken before anything is rendered. Please include at least some text, and while you’re at it make it descriptive.

tests/wpt/mozilla/tests/mozilla/reparse_style_elements.html, line 14 at r1 (raw file):

    <script>
        var s = document.querySelector('body > style');
        s.textContent = 'body { color: green; }';

Nit: this test works as-is, but since this is using the color property please add some text content so that color applies to something. Something descriptive like "This test should be green and the background should not be red."

Comments from Reviewable

zaynetro · 2017-01-11T07:08:13Z

@SimonSapin good call with using getComputedStyle to force styling. I managed to make the test to fail. Now I will start working on removing styles when style element is removed.

zaynetro · 2017-01-16T06:39:43Z

I was busy during the previous week. I will continue looking into this closer to the end of the current week.

zaynetro · 2017-01-20T18:40:33Z

I think I need some help with figuring out how to remove stylesheet added by <style> element.

Some of my findings (doesn't mean they are relevant to the problem):

When children of htmlstyleelement.rs have changed we parse_own_css and then send newly parsed stylesheet with:

win.layout_chan().send(Msg::AddStylesheet(sheet.clone())).unwrap();

I will get back to this later (A)

After sending stylesheet to window layout channel we invalidate stylesheets with doc.invalidate_stylesheets();. Implementation of Document::invalidate_stylesheets() tells me nothing about what is actually happening behind this function call. Where can I look to better understand what is actually going on there?

Returning back to (A). When we receive new stylesheet in layout thread we handle_add_stylesheet. So we load fonts(??) in add_font_face_rules. This is a bit confusing to me as I don't see the place where the stylesheet is being applied to the document.

The problem: I want to remove previously added stylesheet when <style> element is removed from the DOM. For this reason I'd like to have an understanding of the actions described before. Please, let me know if I am looking at the right direction.

    fn unbind_from_tree(&self, context: &UnbindContext) {
        if let Some(ref s) = self.super_type() {
            s.unbind_from_tree(context);
        }

        // Some magic code that will remove stylesheet from the document
    }

jdm · 2017-01-20T18:51:48Z

@zaynetro One piece you're missing is the implementation of Window::force_reflow, which is called when the script thread needs to tell the layout thread to reflow the document. This calls document.get_and_reset_stylesheets_changed_since_reflow(), which is a boolean passed to the layout thread along with all of the known stylesheets in the document - in layout_thread/lib.rs, the stylesheets_changed boolean is passed to Stylist::update in order to determine the stylesheets that are applied to the document.

jdm · 2017-01-20T18:58:31Z

As far as removing the stylesheet from unbind_from_tree, the important bit of Document::invalidate_stylesheets() is that it sets the stylesheets field to None. Next time layout happens, this us to find all of the stylesheets that currently exist in the tree (see Document::ensure_stylesheets()) and pass them to layout, which should have the effect you're looking for.

zaynetro · 2017-01-20T19:49:04Z

Thanks for the explanations!

I've just written a huge post that Document::invalidate_stylesheets doesn't work in this case, but before submitting it I decided to double check everything. The reason it was failing the test was a silly typo. In the test I have a JS function call window.getComputedStyle(document.body); but originally the function was window.getComputedStyles( . :(

Sorry, it took me a while to find out the root cause. Hopefully, everything should be fine now.

jdm · 2017-01-20T20:02:59Z

@bors-servo: try

bors-servo · 2017-01-20T20:03:03Z

⌛ Trying commit 2135d3d with merge d0ae98b...


        Auto merge of #14930 - zaynetro:remove-stylesheets, r=<try>

Implement support for removing stylesheets from their document  This pull request implements removing styles from the document when * `<link>` element with associated styles is removed * `<style>` element is removed Additionally, it tests that when `<style>` element is changed. Styles are being reapplied correctly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14886 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)

bors-servo · 2017-01-20T20:39:08Z

💔 Test failed - linux-rel-wpt

jdm · 2017-01-20T20:45:47Z

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: Same-origin with incorrect hash.

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: Same-origin with sha256 match, sha512 mismatch

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: <crossorigin='anonymous'> with incorrect hash, ACAO: *

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: <crossorigin='use-credentials'> with incorrect hash CORS-eligible

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: <crossorigin='anonymous'> with CORS-ineligible resource

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: Cross-origin, not CORS request, with correct hash

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ PASS [expected FAIL] Style: Cross-origin, not CORS request, with hash mismatch

Very nice.


        Test changing style tag elements


        Remove styles when link node is removed from document


        Removing style element's styles when elements is removed from dom


        Remove fail expectations from subresource integrity test

emilio · 2017-01-22T12:13:38Z

@bors-servo try

bors-servo · 2017-01-22T12:13:42Z

⌛ Trying commit d4ddafd with merge fa1178c...


        Auto merge of #14930 - zaynetro:remove-stylesheets, r=<try>

Implement support for removing stylesheets from their document  This pull request implements removing styles from the document when * `<link>` element with associated styles is removed * `<style>` element is removed Additionally, it tests that when `<style>` element is changed. Styles are being reapplied correctly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14886 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)

bors-servo · 2017-01-22T13:09:10Z

☀️ 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

emilio · 2017-01-22T14:05:40Z

@bors-servo r+

Thanks a lot for these changes @zaynetro! :)

bors-servo · 2017-01-22T14:05:40Z

📌 Commit d4ddafd has been approved by emilio

bors-servo · 2017-01-22T14:05:53Z

💡 This pull request was already approved, no need to approve it again.

bors-servo · 2017-01-22T14:05:54Z

📌 Commit d4ddafd has been approved by emilio

Ms2ger · 2017-01-24T09:10:39Z

@bors-servo try- retry

bors-servo · 2017-01-24T11:25:39Z

⌛ Testing commit d4ddafd with merge dbdcfa6...


        Auto merge of #14930 - zaynetro:remove-stylesheets, r=emilio

Implement support for removing stylesheets from their document  This pull request implements removing styles from the document when * `<link>` element with associated styles is removed * `<style>` element is removed Additionally, it tests that when `<style>` element is changed. Styles are being reapplied correctly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14886 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)

bors-servo · 2017-01-24T11:52:43Z

💔 Test failed - windows-msvc-dev

Ms2ger · 2017-01-24T15:15:44Z

@bors-servo retry servo/saltfs#585

bors-servo · 2017-01-24T17:40:09Z

⌛ Testing commit d4ddafd with merge f3c102d...


        Auto merge of #14930 - zaynetro:remove-stylesheets, r=emilio

Implement support for removing stylesheets from their document  This pull request implements removing styles from the document when * `<link>` element with associated styles is removed * `<style>` element is removed Additionally, it tests that when `<style>` element is changed. Styles are being reapplied correctly. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14886 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)

bors-servo · 2017-01-24T19:14:24Z

☀️ 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 Manishearth Jan 9, 2017

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

highfive assigned SimonSapin and unassigned Manishearth Jan 9, 2017

zaynetro force-pushed the zaynetro:remove-stylesheets branch from df4f243 to 10614d2 Jan 11, 2017

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Jan 13, 2017

jdm mentioned this pull request Jan 18, 2017

<link> elements need to clear any previous stylesheet before loading a new one #15101

Closed

zaynetro force-pushed the zaynetro:remove-stylesheets branch from 10614d2 to 2135d3d Jan 20, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 20, 2017

highfive added the S-tests-failed label Jan 20, 2017

Test changing style tag elements

9760ea2

zaynetro force-pushed the zaynetro:remove-stylesheets branch from 2135d3d to 6c6157b Jan 21, 2017

Roman Zaynetdinov added 3 commits Jan 9, 2017

Remove styles when link node is removed from document

b3ce08f

Removing style element's styles when elements is removed from dom

f0c72cb

Remove fail expectations from subresource integrity test

Loading status checks…

d4ddafd

zaynetro force-pushed the zaynetro:remove-stylesheets branch from 9ab6df3 to d4ddafd Jan 22, 2017

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

highfive assigned emilio and unassigned SimonSapin Jan 22, 2017

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

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

bors-servo merged commit d4ddafd into servo:master Jan 24, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

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

servo / servo

Implement support for removing stylesheets from their document #14930

Implement support for removing stylesheets from their document #14930

zaynetro commented Jan 9, 2017 •

edited by larsbergstrom

highfive commented Jan 9, 2017

Manishearth commented Jan 9, 2017

zaynetro commented Jan 9, 2017

Manishearth commented Jan 9, 2017

SimonSapin commented Jan 9, 2017

zaynetro commented Jan 11, 2017

zaynetro commented Jan 16, 2017

zaynetro commented Jan 20, 2017

jdm commented Jan 20, 2017 •

edited

jdm commented Jan 20, 2017 •

edited

zaynetro commented Jan 20, 2017 •

edited

jdm commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

jdm commented Jan 20, 2017

emilio commented Jan 22, 2017

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

emilio commented Jan 22, 2017 •

edited

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

servo / servo

Join GitHub today

Implement support for removing stylesheets from their document #14930

Implement support for removing stylesheets from their document #14930

Conversation

zaynetro commented Jan 9, 2017 • edited by larsbergstrom

highfive commented Jan 9, 2017

Manishearth commented Jan 9, 2017

zaynetro commented Jan 9, 2017

Manishearth commented Jan 9, 2017

SimonSapin commented Jan 9, 2017

zaynetro commented Jan 11, 2017

zaynetro commented Jan 16, 2017

zaynetro commented Jan 20, 2017

jdm commented Jan 20, 2017 • edited

jdm commented Jan 20, 2017 • edited

zaynetro commented Jan 20, 2017 • edited

jdm commented Jan 20, 2017

bors-servo commented Jan 20, 2017

bors-servo commented Jan 20, 2017

jdm commented Jan 20, 2017

emilio commented Jan 22, 2017

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

emilio commented Jan 22, 2017 • edited

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

bors-servo commented Jan 22, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

Ms2ger commented Jan 24, 2017

bors-servo commented Jan 24, 2017

bors-servo commented Jan 24, 2017

zaynetro commented Jan 9, 2017 •

edited by larsbergstrom

jdm commented Jan 20, 2017 •

edited

jdm commented Jan 20, 2017 •

edited

zaynetro commented Jan 20, 2017 •

edited

emilio commented Jan 22, 2017 •

edited