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

Make offset parent queries less buggy. #14839

Merged
merged 22 commits into from Jan 18, 2017
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
014ad76
Added assertion to process_offset_parent_query
Permutatrix Aug 26, 2016
0a5218d
Fixed ParentOffsetBorderBoxIterator's keeping track of parents
Permutatrix Aug 30, 2016
6623583
offsetParent queries now basically work on inline nodes.
Permutatrix Sep 27, 2016
18793af
Don't crash when offset parent querying an element not in the document
Permutatrix Jan 4, 2017
a61a263
Added tests/wpt/web-platform-tests/cssom-view/offsetPropertiesInline.…
Permutatrix Jan 4, 2017
6a76525
Replaced trivial pattern matching with is_none() and is_some()
Permutatrix Jan 4, 2017
fa0cb7c
Assert self.node_offset_box is None if fragment.node == self.node_add…
Permutatrix Jan 4, 2017
699fc4b
Use InlineFragmentNodeFlags in ParentOffsetBorderBoxIterator
Permutatrix Jan 5, 2017
923ecba
Updated expectations for passing tests
Permutatrix Jan 5, 2017
db5da15
Updated expectations for failing tests that were passing erroneously
Permutatrix Jan 5, 2017
4825169
Split 123-character line
Permutatrix Jan 5, 2017
defa7d9
Actually, node_position probably won't be needed later.
Permutatrix Jan 5, 2017
b75dcc0
Made assertions more helpful
Permutatrix Jan 6, 2017
5be1879
"above the root node" -> "below the root node"
Permutatrix Jan 6, 2017
290ebab
Moved and adjusted offset_properties_inline test
Permutatrix Jan 6, 2017
5ef7a0a
Don't store the offset parent's dimensions
Permutatrix Jan 7, 2017
686d2f8
Don't use rposition() and unwrap() to find parent info
Permutatrix Jan 7, 2017
74e34d6
Handle hypothetical fragments
Permutatrix Jan 13, 2017
cb0d43a
Broke up some comment lines that were a bit too long
Permutatrix Jan 13, 2017
af633b2
Use parent padding box, not border box
Permutatrix Jan 13, 2017
2cd5384
Don't take measurements from non-primary fragments
Permutatrix Jan 13, 2017
400a267
/css-text-3_dev/html/word-break-normal-zh-000.htm fails on Mac
Permutatrix Jan 18, 2017
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Use InlineFragmentNodeFlags in ParentOffsetBorderBoxIterator

  • Loading branch information
Permutatrix committed Jan 13, 2017
commit 699fc4b216f8331063ca479e0c6fec38a831d3ff
@@ -14,6 +14,7 @@ use flow::{self, Flow};
use fragment::{Fragment, FragmentBorderBoxIterator, SpecificFragmentInfo};
use gfx::display_list::{DisplayItemMetadata, DisplayList, OpaqueNode, ScrollOffsetMap};
use gfx_traits::ScrollRootId;
use inline::LAST_FRAGMENT_OF_ELEMENT;
use ipc_channel::ipc::IpcSender;
use opaque_node::OpaqueNodeMethods;
use script_layout_interface::rpc::{ContentBoxResponse, ContentBoxesResponse};
@@ -563,32 +564,48 @@ impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator {
if fragment.style.get_box().position == computed_values::position::T::fixed {
self.parent_nodes.clear();
}
} else if fragment.contains_node(self.node_address) {
// 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.
} else if let Some((inline_context, node_position)) = fragment.inline_context.as_ref().and_then(|inline_context| {
inline_context.nodes.iter().position(|node| node.address == self.node_address).map(|node_position| {
(inline_context, node_position)
})
}) {
// TODO: Handle cases where the `offsetParent` is an inline
// element. This will likely be impossible until
// https://github.com/servo/servo/issues/13982 is fixed. It would
// have been much easier to just use find() instead of position(),
// but node_position will be needed later in order to handle those
// cases.

let node = &inline_context.nodes[node_position];

// Found a fragment in the flow tree whose inline context contains
// the DOM node we're looking for, i.e. the node is inline and
// contains this fragment.
match self.node_offset_box {
Some(NodeOffsetBoxInfo { ref mut rectangle, .. }) => {
*rectangle = rectangle.union(border_box);

This comment has been minimized.

@emilio

emilio Jan 4, 2017

Member

under which circumstances can this change the origin of the rectangle?

This comment has been minimized.

@Permutatrix

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.

@Permutatrix

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.

},
None => {
// https://github.com/servo/servo/issues/13982 will cause
// this assertion to fail sometimes, so it's commented out
// for now.
//assert!(node.flags.contains(FIRST_FRAGMENT_OF_ELEMENT));

self.node_offset_box = Some(NodeOffsetBoxInfo {
offset: border_box.origin,
rectangle: *border_box,
});
},
}

if node.flags.contains(LAST_FRAGMENT_OF_ELEMENT) {
self.has_processed_node = true;
}

// 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.

This comment has been minimized.

@stshine

stshine Jan 6, 2017

Contributor

An element with position: absolute or position: fixed is guaranteed to be a block so you don't need to check that.

This comment has been minimized.

@notriddle

notriddle Jan 6, 2017

Contributor

Actually, it's a bit more complicated than that. The offset* JavaScript functions need to operate on the real boxes, rather than the hypothetical boxes.

In the case of Servo's implementation, the hypothetical absolute-inline box is a Fragment, while the real absolute-inline box is a BlockFlow.

} else if self.node_offset_box.is_some() {
// We've been processing fragments within the node, but now
// we've found one outside it. That means we're done.
// Hopefully.
self.has_processed_node = true;
} else {
} else if self.node_offset_box.is_none() {
// TODO(gw): Is there a less fragile way of checking whether this
// fragment is the body element, rather than just checking that
// it's at level 1 (above the root node)?

This comment has been minimized.

@emilio

emilio Jan 5, 2017

Member

does this comment intend to say "just below the root node"? (I think the usual visualization is with the root at the top)

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.