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

offsetParent queries now basically work on inline nodes.

  • Loading branch information
Permutatrix committed Jan 13, 2017
commit 66235837e72873f15376131f651d510688d424ab
@@ -440,24 +440,29 @@ impl UnioningFragmentScrollAreaIterator {
}
}

struct NodeOffsetBoxInfo {
offset: Point2D<Au>,
rectangle: Rect<Au>,
}

struct ParentBorderBoxInfo {
node_address: OpaqueNode,
border_box: Rect<Au>,
}

struct ParentOffsetBorderBoxIterator {
node_address: OpaqueNode,
has_found_node: bool,
node_border_box: Rect<Au>,
has_processed_node: bool,
node_offset_box: Option<NodeOffsetBoxInfo>,
parent_nodes: Vec<Option<ParentBorderBoxInfo>>,
}

impl ParentOffsetBorderBoxIterator {
fn new(node_address: OpaqueNode) -> ParentOffsetBorderBoxIterator {
ParentOffsetBorderBoxIterator {
node_address: node_address,
has_found_node: false,
node_border_box: Rect::zero(),
has_processed_node: false,
node_offset_box: None,
parent_nodes: Vec::new(),
}
}
@@ -534,20 +539,53 @@ 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>) {
// Remove all nodes at this level or higher, as they can't be parents of this node.
self.parent_nodes.truncate(level as usize);
assert!(self.parent_nodes.len() == level as usize);
match self.node_offset_box {

This comment has been minimized.

@emilio

emilio Jan 4, 2017

Member

nit: Use if self.node_offset_box.is_none().

Some(_) => { }, // We've already found the node, so we already have its parent.
None => {
// Remove all nodes at this level or higher, as they can't be parents of this node.
self.parent_nodes.truncate(level as usize);
assert!(self.parent_nodes.len() == level as usize);
},
}

if fragment.node == self.node_address {
// Found the fragment in the flow tree that matches the
// DOM node being looked for.
self.has_found_node = true;
self.node_border_box = *border_box;
self.has_processed_node = true;
self.node_offset_box = Some(NodeOffsetBoxInfo {
offset: border_box.origin,
rectangle: *border_box,
});

// offsetParent returns null if the node is fixed.
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.

This comment has been minimized.

@emilio

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.

@Permutatrix

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.

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 => {
self.node_offset_box = Some(NodeOffsetBoxInfo {
offset: border_box.origin,
rectangle: *border_box,
});
},
}

// 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 let Some(_) = self.node_offset_box {

This comment has been minimized.

@emilio

emilio Jan 4, 2017

Member

similarly, I think it's better to use self.node_offset_box.is_some() here.

// 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 {
// TODO(gw): Is there a less fragile way of checking whether this
// fragment is the body element, rather than just checking that
@@ -586,7 +624,7 @@ impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator {
}

fn should_process(&mut self, _: &Fragment) -> bool {
!self.has_found_node
!self.has_processed_node
}
}

@@ -807,14 +845,15 @@ pub fn process_offset_parent_query<N: LayoutNode>(requested_node: N, layout_root
-> OffsetParentResponse {
let mut iterator = ParentOffsetBorderBoxIterator::new(requested_node.opaque());
sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
assert!(iterator.has_found_node);
// If we didn't find the node, something went wrong. Unwrap it.
let node_offset_box = iterator.node_offset_box.unwrap();

let parent_info_index = iterator.parent_nodes.iter().rposition(|info| info.is_some());
match parent_info_index {
Some(parent_info_index) => {
let parent = iterator.parent_nodes[parent_info_index].as_ref().unwrap();
let origin = iterator.node_border_box.origin - parent.border_box.origin;
let size = iterator.node_border_box.size;
let origin = node_offset_box.offset - parent.border_box.origin;

This comment has been minimized.

@stshine

stshine Jan 6, 2017

Contributor

According to https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsettop
offsetTop the result of subtracting the y-coordinate of the top padding edge of the first CSS layout box associated with the offsetParent of the element from the y-coordinate of the top border edge of the first CSS layout box associated with the element,
So you may need to subtract the border of the offsetParent first?

This comment has been minimized.

@Permutatrix

Permutatrix Jan 13, 2017

Author Contributor

@stshine Sorry for taking a week to respond, but that's a really great catch. Thank you for looking at this more closely than I did.

let size = node_offset_box.rectangle.size;
OffsetParentResponse {
node_address: Some(parent.node_address.to_untrusted_node_address()),
rect: Rect::new(origin, size),
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.