-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
HTMLElement::Offset{Parent,Left,Right} should call is_the_html_body_element. #13659
Conversation
Heads up! This PR modifies the following files:
|
Anyone have suggestions where a regression test for this should live? |
@bors-servo try |
HTMLElement::Offset{Parent,Left,Right} should call is_the_html_body_element. Fixes #10520. <!-- 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/13659) <!-- Reviewable:end -->
⛄ The build was interrupted to prioritize another pull request. |
HTMLElement::Offset{Parent,Left,Right} should call is_the_html_body_element. Fixes #10520. <!-- 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/13659) <!-- Reviewable:end -->
☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev |
|
Possibly depends on w3c/csswg-drafts#584 |
w3c/csswg-drafts#584 has been addressed (somewhat) This is now dependent on #13708 in order for the regression test to work |
8e1415e
to
ee9a5b7
Compare
@@ -272,9 +272,13 @@ impl HTMLElementMethods for HTMLElement { | |||
|
|||
// https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent | |||
fn GetOffsetParent(&self) -> Option<Root<Element>> { | |||
if self.is::<HTMLBodyElement>() || self.is::<HTMLHtmlElement>() { | |||
if self.is::<HTMLHtmlElement>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, an html
element can be the child of whatever. The test really needs to be about the root element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sounds right. I'm going to assume I just call https://doc.servo.org/script/dom/element/struct.Element.html#method.root_element and check if it's the same as self
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be better to compare self to its document's root element, rather than go up the whole hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even, that self's parent is a Document
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I don't really know what the next steps are here for this. Commit these changes with a failing test? |
@bors-servo: try |
HTMLElement::Offset{Parent,Left,Right} should call is_the_html_body_element. Fixes #10520. <!-- 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/13659) <!-- Reviewable:end -->
💔 Test failed - linux-dev |
|
In theory the test shouldn't be failing anymore if the code compiles. |
Disclaimer: I'm not going to be able to get to this anytime soon if someone else wants to take over |
What test..? |
I guess the regression test needs to be written again. |
Let's close this since nobody's actively working on it. |
Fixes #10520.
This change is