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

The offsetWidth of a span element is 0. #12939

Closed
veer66 opened this Issue Aug 19, 2016 · 9 comments

Comments

Projects
None yet
5 participants
@veer66
Contributor

veer66 commented Aug 19, 2016

offsetWidth obtained by script is 0.

For example, this html page prints 0 to the console.

<!DOCTYPE html>
<html lang="en">
<body>
  <div id="main">
    <span id="span1">Test</span>
  </div>
  <script>
    console.log(document.getElementById("span1").offsetWidth);
  </script>
</body>
</html>

I used Servo commit 9415252 on Ubuntu 16.04.

@Permutatrix

This comment has been minimized.

Show comment
Hide comment
@Permutatrix

Permutatrix Aug 25, 2016

Contributor

The other offset properties are wrong, too:

<!DOCTYPE html>
<html>
  <body>
    <div id="main">
      <span id="span1">Test 1</span>
    </div>
    <p id="not-main" style="position: relative"></p>
    <script>
      var span1 = document.getElementById("span1");
      console.log(span1.offsetParent); // [object HTMLParagraphElement]
      console.log(span1.offsetLeft); // -8
      console.log(span1.offsetTop); // -27
      console.log(span1.offsetWidth); // 0
      console.log(span1.offsetHeight); // 0
    </script>
  </body>
</html>

This seems to be because the iteration initiated here doesn't actually find the node it's looking for if said node is set to display: inline. There's no assertion or anything there, so it just keeps going with its invalid state and produces the bizarre results above.

I'd like to take this. I've already made it my first foray into the Servo codebase by investigating it this far, and it doesn't seem all too involved.

Contributor

Permutatrix commented Aug 25, 2016

The other offset properties are wrong, too:

<!DOCTYPE html>
<html>
  <body>
    <div id="main">
      <span id="span1">Test 1</span>
    </div>
    <p id="not-main" style="position: relative"></p>
    <script>
      var span1 = document.getElementById("span1");
      console.log(span1.offsetParent); // [object HTMLParagraphElement]
      console.log(span1.offsetLeft); // -8
      console.log(span1.offsetTop); // -27
      console.log(span1.offsetWidth); // 0
      console.log(span1.offsetHeight); // 0
    </script>
  </body>
</html>

This seems to be because the iteration initiated here doesn't actually find the node it's looking for if said node is set to display: inline. There's no assertion or anything there, so it just keeps going with its invalid state and produces the bizarre results above.

I'd like to take this. I've already made it my first foray into the Servo codebase by investigating it this far, and it doesn't seem all too involved.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 25, 2016

Member

Fantastic; thanks!

Member

jdm commented Aug 25, 2016

Fantastic; thanks!

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 25, 2016

Member

@Permutatrix Did you make any progress here?

Member

jdm commented Sep 25, 2016

@Permutatrix Did you make any progress here?

@Permutatrix

This comment has been minimized.

Show comment
Hide comment
@Permutatrix

Permutatrix Sep 25, 2016

Contributor

@jdm Some. I found after taking it that fixing this bug would actually require implementing the offset properties for inline elements from scratch, so I looked into what that would entail, and Firefox and Chrome's implementations are inconsistent with each other. They behave very strangely when it comes to replaced and inline-block children—for instance, take this mess:

<!DOCTYPE html>
<html>
  <body>
    <span id="span1">
      Test 3
      <img src="100px-square.png" />
    </span>
    <script>
      var span1 = document.getElementById("span1");
      console.log(span1.offsetParent); // <body>
      console.log(span1.offsetLeft);   // Fx:   8. Ch:  8.
      console.log(span1.offsetTop);    // Fx:  93. Ch:  8.
      console.log(span1.offsetWidth);  // Fx: 153. Ch: 42.
      console.log(span1.offsetHeight); // Fx:  19. Ch: 17.
    </script>
  </body>
</html>

I figured I should talk to someone about this, but I guess I didn't. For a month. Because I am very "independent", which means terrible at teamwork.

I believe I could write an implementation of the offset properties for inline elements easily enough if I didn't have to worry about the details as Firefox and Chrome seem to have done, though I don't like the way these queries are implemented, using stateful objects that have the same method called over and over to process a sequence of items. It's terribly inside-out, and any implementation of something as complicated as these properties under the system is bound to be extremely brittle.

Contributor

Permutatrix commented Sep 25, 2016

@jdm Some. I found after taking it that fixing this bug would actually require implementing the offset properties for inline elements from scratch, so I looked into what that would entail, and Firefox and Chrome's implementations are inconsistent with each other. They behave very strangely when it comes to replaced and inline-block children—for instance, take this mess:

<!DOCTYPE html>
<html>
  <body>
    <span id="span1">
      Test 3
      <img src="100px-square.png" />
    </span>
    <script>
      var span1 = document.getElementById("span1");
      console.log(span1.offsetParent); // <body>
      console.log(span1.offsetLeft);   // Fx:   8. Ch:  8.
      console.log(span1.offsetTop);    // Fx:  93. Ch:  8.
      console.log(span1.offsetWidth);  // Fx: 153. Ch: 42.
      console.log(span1.offsetHeight); // Fx:  19. Ch: 17.
    </script>
  </body>
</html>

I figured I should talk to someone about this, but I guess I didn't. For a month. Because I am very "independent", which means terrible at teamwork.

I believe I could write an implementation of the offset properties for inline elements easily enough if I didn't have to worry about the details as Firefox and Chrome seem to have done, though I don't like the way these queries are implemented, using stateful objects that have the same method called over and over to process a sequence of items. It's terribly inside-out, and any implementation of something as complicated as these properties under the system is bound to be extremely brittle.

@Permutatrix

This comment has been minimized.

Show comment
Hide comment
@Permutatrix

Permutatrix Sep 27, 2016

Contributor

Alright, I made something that seems to know what it's doing if you don't grill it too hard. (It notably doesn't yet work on absolutely positioned inline elements.) But then it occurred to me that this FragmentBorderBoxIterator system as it is now has no way of addressing a few cases involving inline elements, because an empty inline element is completely invisible to such an iterator.

Right now I'm thinking of solving this problem by getting rid of ParentOffsetBorderBoxIterator completely (yay!) and moving the logic into process_offset_parent_query so we can interrogate the layout_root Flow directly. I'll have to learn more about how Flows work. This issue is a lot more interesting than it looked.

Contributor

Permutatrix commented Sep 27, 2016

Alright, I made something that seems to know what it's doing if you don't grill it too hard. (It notably doesn't yet work on absolutely positioned inline elements.) But then it occurred to me that this FragmentBorderBoxIterator system as it is now has no way of addressing a few cases involving inline elements, because an empty inline element is completely invisible to such an iterator.

Right now I'm thinking of solving this problem by getting rid of ParentOffsetBorderBoxIterator completely (yay!) and moving the logic into process_offset_parent_query so we can interrogate the layout_root Flow directly. I'll have to learn more about how Flows work. This issue is a lot more interesting than it looked.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jan 3, 2017

Member

Any updates on this? What work is left to be done to make Permutatrix@85da4a9 landable?

Member

Manishearth commented Jan 3, 2017

Any updates on this? What work is left to be done to make Permutatrix@85da4a9 landable?

@Permutatrix

This comment has been minimized.

Show comment
Hide comment
@Permutatrix

Permutatrix Jan 4, 2017

Contributor

@Manishearth I guess I was just too much of a chucklehead to realize that Permutatrix@85da4a9 mostly fixes this bug even with #13982 out there. (Never mind that that bug should be long fixed by now anyway.) I'll have to remove the unwrap(), because even in the absence of any other bugs, it'll fail if an element is queried that isn't in the document. Other than that, it's probably mostly fine. I'll run the tests and see if I need to change anything else.

Contributor

Permutatrix commented Jan 4, 2017

@Manishearth I guess I was just too much of a chucklehead to realize that Permutatrix@85da4a9 mostly fixes this bug even with #13982 out there. (Never mind that that bug should be long fixed by now anyway.) I'll have to remove the unwrap(), because even in the absence of any other bugs, it'll fail if an element is queried that isn't in the document. Other than that, it's probably mostly fine. I'll run the tests and see if I need to change anything else.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Jan 4, 2017

Member

Cool! Let me know if you need help! 😄

Member

Manishearth commented Jan 4, 2017

Cool! Let me know if you need help! 😄

@Permutatrix

This comment has been minimized.

Show comment
Hide comment
@Permutatrix

Permutatrix Jan 4, 2017

Contributor

@Manishearth Thanks. You guys are too nice to me, eheh... I just need to learn to be more responsible...

Contributor

Permutatrix commented Jan 4, 2017

@Manishearth Thanks. You guys are too nice to me, eheh... I just need to learn to be more responsible...

@Permutatrix Permutatrix referenced this issue Jan 4, 2017

Merged

Make offset parent queries less buggy. #14839

4 of 4 tasks complete

bors-servo added a commit that referenced this issue Jan 4, 2017

Auto merge of #14839 - Permutatrix:iss-12939, r=<try>
Make offset parent queries less buggy.

<!-- Please describe your changes on the following line: -->
Offset parent queries, which are used in the getters for HTMLElement's `offsetParent`, `offsetTop`, `offsetLeft`, `offsetWidth`, and `offsetHeight`, are pretty busted. The most egregious bug is that, as reported in #12939, inline elements are treated as if they're not present in the document. This PR fixes that and all of the other bugs I could trace directly to the offset parent query code, but `offsetTop` and `offsetLeft` are still terribly unreliable for reasons I haven't looked into (#13708). Inline elements with no content are still treated as not present due to #13982, so #13944 isn't fixed with this PR, either.

---
<!-- 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 #12939, #12595

<!-- 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/14839)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Jan 5, 2017

Auto merge of #14839 - Permutatrix:iss-12939, r=<try>
Make offset parent queries less buggy.

<!-- Please describe your changes on the following line: -->
Offset parent queries, which are used in the getters for HTMLElement's `offsetParent`, `offsetTop`, `offsetLeft`, `offsetWidth`, and `offsetHeight`, are pretty busted. The most egregious bug is that, as reported in #12939, inline elements are treated as if they're not present in the document. This PR fixes that and all of the other bugs I could trace directly to the offset parent query code, but `offsetTop` and `offsetLeft` are still terribly unreliable for reasons I haven't looked into (#13708). Inline elements with no content are still treated as not present due to #13982, so #13944 isn't fixed with this PR, either.

---
<!-- 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 #12939, #12595

<!-- 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/14839)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Jan 18, 2017

Auto merge of #14839 - Permutatrix:iss-12939, r=emilio
Make offset parent queries less buggy.

<!-- Please describe your changes on the following line: -->
Offset parent queries, which are used in the getters for HTMLElement's `offsetParent`, `offsetTop`, `offsetLeft`, `offsetWidth`, and `offsetHeight`, are pretty busted. The most egregious bug is that, as reported in #12939, inline elements are treated as if they're not present in the document. This PR fixes that and all of the other bugs I could trace directly to the offset parent query code, but `offsetTop` and `offsetLeft` are still unreliable in certain circumstances for reasons I haven't looked into (#13708). Inline elements with no content are still treated as not present due to #13982, so #13944 isn't fixed with this PR, either.

---
<!-- 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 #12939 and fix #12595

<!-- 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/14839)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment