Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMake offset parent queries less buggy. #14839
Merged
Conversation
highfive
commented
Jan 4, 2017
|
Heads up! This PR modifies the following files:
|
|
I didn't update the test expectations because I couldn't figure out how to separate the gunk from the things that were actually affected by my changes. I do know that there are some tests that now fail with this PR because they were passing erroneously. |
|
And yeah, this little patch took me four months to get around to finishing. I'm the best, I'm totally in control of my life, hurr hurr. |
|
@bors-servo try |
bors-servo
added a commit
that referenced
this pull request
Jan 4, 2017
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 -->
components/layout/query.rs
Outdated
| @@ -527,21 +530,58 @@ impl FragmentBorderBoxIterator for UnioningFragmentScrollAreaIterator { | |||
| // https://drafts.csswg.org/cssom-view/#extensions-to-the-htmlelement-interface | |||
| impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator { | |||
| fn process(&mut self, fragment: &Fragment, level: i32, border_box: &Rect<Au>) { | |||
| match self.node_offset_box { | |||
This comment has been minimized.
This comment has been minimized.
components/layout/query.rs
Outdated
| // TODO: `position: fixed` on inline elements doesn't seem to work | ||
| // as of Sep 25, 2016, so I don't know how one will check if an | ||
| // inline element has it when it does. | ||
| } else if let Some(_) = self.node_offset_box { |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 4, 2017
Member
similarly, I think it's better to use self.node_offset_box.is_some() here.
components/layout/query.rs
Outdated
| // Found a fragment in the flow tree that matches a child | ||
| // of the DOM node being looked for. | ||
| // Since we haven't found an exact match, the node must be | ||
| // inline. Hopefully. |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 4, 2017
Member
Gah, this feels quite a lot like a hack... No better ideas right now though :(
This comment has been minimized.
This comment has been minimized.
Permutatrix
Jan 4, 2017
Author
Contributor
@emilio Yes, this entire thing is truly horrible and relies on a lot of unenforced invariants. You could break it from some entirely separate part of layout, and the tests probably wouldn't tell you.
A much better way of handling this particular section has occurred to me, using InlineFragmentNodeFlags. In fact, the current strategy fails when applied to an element containing an {ib} split (never mind that KHTML/WebKit/Blink-based browsers all fail in such a situation as well!). I'm afraid this code hasn't aged quite as well as I thought, but no matter. I'll fix it.
components/layout/query.rs
Outdated
| // inline. Hopefully. | ||
| match self.node_offset_box { | ||
| Some(NodeOffsetBoxInfo { ref mut rectangle, .. }) => { | ||
| *rectangle = rectangle.union(border_box); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Permutatrix
Jan 4, 2017
•
Author
Contributor
@emilio Text that starts in the middle of a line and has a line break will have boxes further to the left than the first one, so they'll change the origin. I imagine right-to-left text will do so as well.
The offset properties are very confusing. The position (offsetLeft, offsetTop) doesn't actually correspond to the upper-left corner of a rectangle with dimensions offsetWidth x offsetHeight.
Come to think of it, right-to-left text might mess up the result of offsetLeft from this code. Then again, off the top of my head it seems like the "messed-up" results I'm imagining, unintuitive though they are, may actually be the correct ones according the spec. I'll have to look into that.
This comment has been minimized.
This comment has been minimized.
Permutatrix
Jan 5, 2017
•
Author
Contributor
Okay, Firefox (and Konqueror, remarkably enough) measure offsetLeft based on the first line box, which is technically correct, but WebKit/Blink-based browsers just use the first node like this patch currently does. It's the same story for getClientRects(), actually—Gecko correctly returns each whole line's border box, whereas WebKit/Blink and Servo split it up into nodes. This should be a separate issue, I suppose.
|
|
|
|
I just noticed that my code can't detect when the <!DOCTYPE html>
<style>
.parent {
position: relative;
}
</style>
<div class="parent">
<span class="parent">
<span id="this">ABC</span>
</span>
</div>
<span class="parent">
<div>
<span id="that">ABC</span>
</div>
</span>
<script>
// should be: SPAN. Gecko: SPAN. KHTML/WebKit/Blink: SPAN. Patched Servo: DIV.
console.log(document.getElementById('this').offsetParent.tagName);
// should be: SPAN. Gecko: SPAN. KHTML/WebKit/Blink: BODY. Patched Servo: BODY.
console.log(document.getElementById('that').offsetParent.tagName);
</script>This may be pretty easy to fix, or it may be very hard. I'll look into it. |
|
Okay, I believe that it will be impossible to fix the bug I described in #14839 (comment) until #13982 is fixed. I'll file a separate issue after this is merged. |
|
Alright, I addressed the issues @emilio brought up and updated the test expectations. (Hopefully these new changes won't change which tests are passing.) |
|
@bors-servo try |
bors-servo
added a commit
that referenced
this pull request
Jan 5, 2017
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 -->
|
|
Those are all intermittent, right? |
|
Third one is, first two are not, as far as we know. The filtered-intermittents file will filter out intermittents and tell you what's left. It's possible that those two are intermittents since we've seen their type before, but since we're changing layout I'd rather be sure. Retrying @bors-servo try |
|
@Manishearth Really? It looks like the first two are covered by #13919. Unless I'm missing something about how this works...? |
|
Oh, that's due to a bug in the intermittents tracker, sorry (It doesn't notice when issues are renamed) |
|
Interesting to see that the fails is quite valid since we do not implement the wide character breaks properly (and neither Blink nor Gecko). |
|
Yeah, but they're intermittent failures, so they don't always happen and we can't mark them as "expected fail" in the manifest. |
|
Problem with inline elements is that they are |
|
I removed some baroque functional business I had naïvely put in "for later" because I didn't think hard enough about how #14839 (comment) should ultimately be resolved. |
|
|
|
|
This PR removed the ini file for this test, but it's not passing on the builder. |
|
Non-reference: Only mac failed, and it appears to be a font-based issue, so let's mark this as an expected failure on mac only. |
|
@bors-servo r=emilio |
|
|
|
|
@jdm Done. |
|
@bors-servo: r=emilio |
|
|
bors-servo
added a commit
that referenced
this pull request
Jan 18, 2017
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Permutatrix commentedJan 4, 2017
•
edited
Offset parent queries, which are used in the getters for HTMLElement's
offsetParent,offsetTop,offsetLeft,offsetWidth, andoffsetHeight, 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, butoffsetTopandoffsetLeftare 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../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is