Skip to content

/ servo

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 stylesheet.ownerNode #23096

Closed
wants to merge 1 commit into from
+85 −30
Closed

implement stylesheet.ownerNode #23096

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

16 components/script/dom/cssrulelist.rs
View file
Open in desktop
@@ -14,7 +14,9 @@ use crate::dom::cssrule::CSSRule;
use crate::dom::cssstylesheet::CSSStyleSheet;
use crate::dom::htmlelement::HTMLElement;
use crate::dom::window::Window;
use crate::style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
use crate::stylesheet_loader::StylesheetLoader;

use dom_struct::dom_struct;
use servo_arc::Arc;
use style::shared_lock::Locked;
@@ -104,20 +106,20 @@ impl CSSRuleList {
let index = idx as usize;

let parent_stylesheet = self.parent_stylesheet.style_stylesheet();
let owner = self
.parent_stylesheet
.get_owner()
.downcast::<HTMLElement>()
.unwrap();
let loader = StylesheetLoader::for_element(owner);

let owner = self.parent_stylesheet.get_owner();
let loader = owner
.as_ref()
.map(|owner| StylesheetLoader::for_element(owner.downcast::<HTMLElement>().unwrap()));

let new_rule = css_rules.with_raw_offset_arc(|arc| {
arc.insert_rule(
&parent_stylesheet.shared_lock,
rule,
&parent_stylesheet.contents,
index,
nested,
Some(&loader),
loader.as_ref().map(|l| l as &StyleStylesheetLoader),
)
})?;

14 components/script/dom/cssstylesheet.rs
View file
Open in desktop
@@ -7,7 +7,7 @@ use crate::dom::bindings::codegen::Bindings::CSSStyleSheetBinding::CSSStyleSheet
use crate::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::WindowMethods;
use crate::dom::bindings::error::{Error, ErrorResult, Fallible};
use crate::dom::bindings::reflector::{reflect_dom_object, DomObject};
use crate::dom::bindings::root::{Dom, DomRoot, MutNullableDom};
use crate::dom::bindings::root::{DomRoot, MutNullableDom};
use crate::dom::bindings::str::DOMString;
use crate::dom::cssrulelist::{CSSRuleList, RulesSource};
use crate::dom::element::Element;
@@ -22,7 +22,7 @@ use style::stylesheets::Stylesheet as StyleStyleSheet;
#[dom_struct]
pub struct CSSStyleSheet {
stylesheet: StyleSheet,
owner: Dom<Element>,
owner: MutNullableDom<Element>,
rulelist: MutNullableDom<CSSRuleList>,
#[ignore_malloc_size_of = "Arc"]
style_stylesheet: Arc<StyleStyleSheet>,
@@ -39,7 +39,7 @@ impl CSSStyleSheet {
) -> CSSStyleSheet {
CSSStyleSheet {
stylesheet: StyleSheet::new_inherited(type_, href, title),
owner: Dom::from_ref(owner),
owner: MutNullableDom::new(Some(owner)),
rulelist: MutNullableDom::new(None),
style_stylesheet: stylesheet,
origin_clean: Cell::new(true),
@@ -75,8 +75,8 @@ impl CSSStyleSheet {
self.style_stylesheet.disabled()
}

pub fn get_owner(&self) -> &Element {
&*self.owner
pub fn get_owner(&self) -> Option<DomRoot<Element>> {
self.owner.get()
}

pub fn set_disabled(&self, disabled: bool) {
@@ -88,6 +88,10 @@ impl CSSStyleSheet {
}
}

pub fn set_owner(&self, value: Option<&Element>) {
self.owner.set(value);
}

pub fn shared_lock(&self) -> &SharedRwLock {
&self.style_stylesheet.shared_lock
}
9 components/script/dom/htmllinkelement.rs
View file
Open in desktop
@@ -85,6 +85,12 @@ impl HTMLLinkElement {
}
}

fn stylesheet_owner_cleanup(&self) {
if let Some(cssom_stylesheet) = self.cssom_stylesheet.get() {
cssom_stylesheet.set_owner(None);
}
}

#[allow(unrooted_must_root)]
pub fn new(
local_name: LocalName,
@@ -113,6 +119,7 @@ impl HTMLLinkElement {
doc.remove_stylesheet(self.upcast(), s)
}
*self.stylesheet.borrow_mut() = Some(s.clone());
self.stylesheet_owner_cleanup();
self.cssom_stylesheet.set(None);
This conversation was marked as resolved by sbansal3096

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 8, 2019

Collaborator

I think it's good to move the cleanup of cssom_stylesheet into a function so that we can reuse it in both set_stylesheet and unbind_from_tree. (Same for htmlstyleelement.rs)

Ex. (pseudo code)

fn cssom_stylesheet_cleanup(&self) {
    if let Some(cssom_stylesheet) = self.cssom_stylesheet.get() {
        cssom_stylesheet.set_owner(None);
    }
    self.cssom_stylesheet.set(None);
}

fn set_stylesheet(...) {
    ...
    *self.stylesheet.borrow_mut() = Some(s.clone());
    self.cssom_stylesheet_cleanup();
    ...
}

fn unbind_from_tree(...) {
    ...
    if let Some(s) = self.stylesheet.borrow_mut().take() {
        self.cssom_stylesheet_cleanup();
        ...
    }
}
doc.add_stylesheet(self.upcast(), s);
}
@@ -250,8 +257,8 @@ impl VirtualMethods for HTMLLinkElement {
if let Some(ref s) = self.super_type() {
s.unbind_from_tree(context);
}

if let Some(s) = self.stylesheet.borrow_mut().take() {
self.stylesheet_owner_cleanup();
document_from_node(self).remove_stylesheet(self.upcast(), &s);
}
}
8 components/script/dom/htmlstyleelement.rs
View file
Open in desktop
@@ -62,6 +62,12 @@ impl HTMLStyleElement {
}
}

fn stylesheet_owner_cleanup(&self) {
if let Some(cssom_stylesheet) = self.cssom_stylesheet.get() {
cssom_stylesheet.set_owner(None);
}
}

#[allow(unrooted_must_root)]
pub fn new(
local_name: LocalName,
@@ -143,6 +149,7 @@ impl HTMLStyleElement {
doc.remove_stylesheet(self.upcast(), s)
}
*self.stylesheet.borrow_mut() = Some(s.clone());
self.stylesheet_owner_cleanup();
self.cssom_stylesheet.set(None);
doc.add_stylesheet(self.upcast(), s);
}
@@ -216,6 +223,7 @@ impl VirtualMethods for HTMLStyleElement {

if context.tree_in_doc {
if let Some(s) = self.stylesheet.borrow_mut().take() {
self.stylesheet_owner_cleanup();

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 10, 2019

Member

This should also cleanup self.cssom_stylesheet, shouldn't it?
document_from_node(self).remove_stylesheet(self.upcast(), &s)
This conversation was marked as resolved by sbansal3096

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 7, 2019

Collaborator

I wonder we should do the set_owner(None) inside this if let because we should only make the owner to None when it's in the tree? 🤔 cc @emilio
}
}
7 components/script/dom/stylesheet.rs
View file
Open in desktop
@@ -5,8 +5,10 @@
use crate::dom::bindings::codegen::Bindings::StyleSheetBinding::StyleSheetMethods;
use crate::dom::bindings::inheritance::Castable;
use crate::dom::bindings::reflector::Reflector;
use crate::dom::bindings::root::DomRoot;
use crate::dom::bindings::str::DOMString;
use crate::dom::cssstylesheet::CSSStyleSheet;
use crate::dom::element::Element;
use dom_struct::dom_struct;

#[dom_struct]
@@ -44,6 +46,11 @@ impl StyleSheetMethods for StyleSheet {
self.href.clone()
}

// https://drafts.csswg.org/cssom/#dom-stylesheet-ownernode
fn GetOwnerNode(&self) -> Option<DomRoot<Element>> {
self.downcast::<CSSStyleSheet>().unwrap().get_owner()
}

// https://drafts.csswg.org/cssom/#dom-stylesheet-title
fn GetTitle(&self) -> Option<DOMString> {
self.title.clone()
2 components/script/dom/webidls/StyleSheet.webidl
View file
Open in desktop
@@ -7,8 +7,8 @@
interface StyleSheet {
readonly attribute DOMString type_;
readonly attribute DOMString? href;
readonly attribute Element? ownerNode;
This conversation was marked as resolved by CYBAI

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Mar 26, 2019

Collaborator

As we can see from the spec, the type of ownerNode is still Element or ProcessingInstruction. I think it's still good to keep the type here.

  readonly attribute (Element or ProcessingInstruction)? ownerNode;

@emilio how do you think?

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Mar 26, 2019

Member

I think given Servo doesn't support XML processing instructions, the difference is meaningless. All browsers use Node? in the IDL as you noted on IRC... So all of them are fine really.

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Mar 26, 2019

Collaborator

Yes, after checking Chrome's implementation and Firefox's implementation, both of them use Node? type for ownerNode. So, let's keep Element? here because Servo doesn't support XML processing instructions 🙇

// readonly attribute (Element or ProcessingInstruction)? ownerNode;
// readonly attribute StyleSheet? parentStyleSheet;
readonly attribute DOMString? title;

10 tests/wpt/metadata/MANIFEST.json
View file
Open in desktop
{}
]
],
"css/cssom/nullable-owner.html": [
[
"css/cssom/nullable-owner.html",
{}
]
],
"css/cssom/overflow-serialization.html": [
[
"css/cssom/overflow-serialization.html",
"7cbea37cba421bbf13887322cc48b72a8e04cdc8",
"testharness"
],
"css/cssom/nullable-owner.html": [
"f44e3cd420cf9a7baf7497fc03329a2d1a3289bb",
"testharness"
],
"css/cssom/overflow-serialization.html": [
"136b8aba117eb64403700d8c4348db085cede9c8",
"testharness"
12 tests/wpt/metadata/css/cssom/MediaList2.xhtml.ini
View file
Open in desktop
@@ -1,4 +1,12 @@
[MediaList2.xhtml]
[MediaList]
[MediaList2.xhtml]
[MediaList.mediaText]
expected: FAIL

[MediaList.length]

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 10, 2019

Member

Do we know why these are regressing?

This comment has been minimized.

Sign in to view
Copy link
@sbansal3096

sbansal3096 Apr 11, 2019

Author Contributor

I am not sure of the reason but these were failing from the start. Josh asked me to leave that for now and report it in a new issue. https://mozilla.logbot.info/servo/20190408#c16185606

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Apr 11, 2019

Member

They're not regressing; the test file uses ownerNode before running these tests, so they never had a chance to run before now.
expected: FAIL

[MediaList getter]
expected: FAIL

[MediaList.item]
expected: FAIL
9 tests/wpt/metadata/css/cssom/interfaces.html.ini
View file
Open in desktop
@@ -78,9 +78,6 @@
[StyleSheet interface: attribute type]
expected: FAIL

[StyleSheet interface: attribute ownerNode]
expected: FAIL

[StyleSheet interface: attribute parentStyleSheet]
expected: FAIL

@@ -678,9 +675,6 @@
[StyleSheet interface: sheet must inherit property "type" with the proper type]
expected: FAIL
[StyleSheet interface: sheet must inherit property "ownerNode" with the proper type]
expected: FAIL
[StyleSheet interface: sheet must inherit property "parentStyleSheet" with the proper type]
expected: FAIL
@@ -993,9 +987,6 @@
[StyleSheet interface: sheet must inherit property "type" with the proper type]
expected: FAIL
[StyleSheet interface: sheet must inherit property "ownerNode" with the proper type]
expected: FAIL
[StyleSheet interface: sheet must inherit property "parentStyleSheet" with the proper type]
expected: FAIL
9 tests/wpt/metadata/css/cssom/ttwf-cssom-doc-ext-load-count.html.ini
View file
Open in desktop
@@ -1,5 +1,4 @@
[ttwf-cssom-doc-ext-load-count.html]
type: testharness
[stylesheet.css should be unloaded and styleSheets.length === 0]
This conversation was marked as resolved by sbansal3096

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 8, 2019

Collaborator

I remember you said this test will keep failing? besides, is it possible to find the root cause of it? 🙇

This comment has been minimized.

Sign in to view
Copy link
@sbansal3096

sbansal3096 Apr 9, 2019

Author Contributor

It can be found, but it was even failing before this pull request. So I think it is best to reference this in new issue. Still if you want we can look into it.

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 9, 2019

Collaborator

but it was even failing before this pull request

yeah, understood so maybe we should keep it labeled as FAIL :)?

So I think it is best to reference this in new issue. Still if you want we can look into it.

Yes, I want to create a new issue for it but I didn't know the root cause for it :( I think it's still good to find the root cause to see if it's related to ownerNode itself :)

This comment has been minimized.

Sign in to view
Copy link
@sbansal3096

sbansal3096 Apr 9, 2019

Author Contributor

yeah, understood so maybe we should keep it labeled as FAIL :)?

Sorry if I misunderstood, does this mean making it's expectation as FAIL? If it is the case then I have already done that.

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 9, 2019

Collaborator

Sorry, maybe I didn't explain my thought clearly.

I mean

(the second test was originally labeled as FAIL)

servo/tests/wpt/web-platform-tests/css/cssom/ttwf-cssom-doc-ext-load-count.html

Line 39 in cd579f6

}, "stylesheet.css should be unloaded and styleSheets.length === 0");

and in current code change it's removed and only added the 3rd test as FAIL

servo/tests/wpt/web-platform-tests/css/cssom/ttwf-cssom-doc-ext-load-count.html

Line 57 in cd579f6

}, "stylesheet-1.css should be loaded and styleSheets.length === 1");

So, I think both of them are FAIL now.

This comment has been minimized.

Sign in to view
Copy link
@sbansal3096

sbansal3096 Apr 9, 2019

Author Contributor

Actually the 2nd test is passing now, so I removed the FAIL label. And due to this test passing, it was revealed that there was some error previously in the 3rd test.
#23096 (comment)

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Apr 9, 2019

Collaborator

Ahh, I see. Sorry, I misunderstood your comment yesterday. Then your current change is correct! Thanks!
expected: FAIL

[ttwf-cssom-doc-ext-load-count.html]
type: testharness
[stylesheet-1.css should be loaded and styleSheets.length === 1]
expected: FAIL
19 tests/wpt/web-platform-tests/css/cssom/nullable-owner.html
View file
Open in desktop
@@ -0,0 +1,19 @@
<!doctype html>
<head>
<meta charset="utf-8">
<title>ownerNode null test</title>
<link rel="help" href="https://drafts.csswg.org/cssom/#dom-stylesheet-ownernode">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<style></style>
</head>
<body>
<script>
test(function() {
let sheet = document.querySelector("style").sheet;
assert_true(sheet.ownerNode instanceof HTMLStyleElement, "ownerNode should be an instance of HTMLStyleElement");
sheet.ownerNode.remove();
assert_equals(sheet.ownerNode, null, "ownerNode should be null after removing the styleSheet");

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Apr 10, 2019

Member

We should also assert that the style element's sheet is null, which now would fail. Basically, any call to stylesheet_owner_cleanup needs to also cleanup the owner's stylesheet. Mind filing a followup issue to extend this test and fix that?

This comment has been minimized.

Sign in to view
Copy link
@sbansal3096

sbansal3096 Apr 11, 2019

Author Contributor

Ok, I can do that. So is there any change that I need to make in this pull request?
}, "ownerNode should be cleared out before removing the styleSheet");
</script>
</body>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.