Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for removing stylesheets from their document. #14886

Closed
jdm opened this issue Jan 6, 2017 · 7 comments · Fixed by #14930
Closed

Implement support for removing stylesheets from their document. #14886

jdm opened this issue Jan 6, 2017 · 7 comments · Fixed by #14930
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue

Comments

@jdm
Copy link
Member

jdm commented Jan 6, 2017

10:32 <jdm> SimonSapin: do you know if we can support removing stylesheets from the document properly yet?
10:33 <SimonSapin> jdm: that should work fine now, assuming we call Stylist.update at the appropriate time

This means we should add this behaviour when:

  • <style> and <link> elements are removed from documents (unbind_from_tree)
  • the children of <style> elements are modified (we should remove the associated stylesheet and reparse it)
@jdm jdm added A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/dom Interacting with the DOM from web content labels Jan 6, 2017
@jdm
Copy link
Member Author

jdm commented Jan 6, 2017

@zaynetro If you're interested in making some other contributions, this might be interesting!

@zaynetro
Copy link
Contributor

zaynetro commented Jan 6, 2017

@jdm Thanks for mentioning me! I am definitely interested. Tomorrow I will take a look and ask questions.

@zaynetro
Copy link
Contributor

zaynetro commented Jan 7, 2017

I will try listing my findings and steps that I think should be taken. Note, that I have a very poor knowledge of the code base. Although I am hoping to improve it with your help.

  • <style> and <link> elements are meta elements (components/dom/style/htmlmetaelement.rs). When unbind_from_tree is being called we want to remove css styles from the document(?) and update document styles by calling Stylist.update().

    • When the element that is being removed has a stylesheet how do I remove the stylesheet from the document?
    • I found that Stylist.update is being called from LayoutThread.handle_reflow. How do I trigger the reflow from unbind_from_tree method?
    • <style> element can contain cssom_stylesheet that also needs to be removed from the document(?). At the moment I have no knowledge on how to do that.
  • "the children of <style> elements are modified" Any hints on where to look?

@jdm
Copy link
Member Author

jdm commented Jan 7, 2017

<style> and <link> elements are actually HTMLStyleElement and HTMLLinkElement respectively, defined in htmlstyleelement.rs and htmllinkelement.rs. The important hooks are unbind_from_tree and children_changed. It looks like if we call document.invalidate_stylesheets(), that will cause the code that invokes Stylist.update() to run when the next reflow happens.

@jdm
Copy link
Member Author

jdm commented Jan 7, 2017

From reading HTMLStyleElement::parse_own_css, it looks like we may already handle modifying the children of a <style> element correctly. It would be good to write a test that verifies this - start with a file that contains a style element with a body { background-color: red; } text node, and replace that text node (using JS) with a new one that says body { color: green } and verify that the background color is not red afterwards.

@zaynetro
Copy link
Contributor

zaynetro commented Jan 8, 2017

I can't believe I am so blind :) Thanks! Could you suggest the correct directory for the test? If I understand correctly then I can create one with:

./mach create-wpt tests/wpt/path/to/new/test.html

@jdm
Copy link
Member Author

jdm commented Jan 8, 2017

tests/wpt/mozilla/tests/Mozilla is fine for now. You'll want a reference test, which requires a --reference argument to create-wpt too.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Jan 11, 2017
bors-servo pushed a commit that referenced this issue Jan 20, 2017
Implement support for removing stylesheets from their document

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 22, 2017
Implement support for removing stylesheets from their document

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 24, 2017
Implement support for removing stylesheets from their document

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 24, 2017
Implement support for removing stylesheets from their document

<!-- Please describe your changes on the following line: -->
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14930)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants