From a5a52147836bafa7fa865300d03d19d2d4004dd4 Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Fri, 1 May 2015 11:04:52 +1000 Subject: [PATCH] Various fixes to getClientBoundingRect() * Fix queries involving stacking contexts * The code was double accumulating stacking context origins. * Handle queries of inline elements. * The node addresses being compared were incorrect (CharacterData vs. Span) * Handle ScriptQuery reflows correctly. * The layout task was skipping the compute absolute positions traversal, so failed before window.onload. --- components/gfx/display_list/mod.rs | 2 +- components/layout/block.rs | 2 +- components/layout/construct.rs | 16 +-- components/layout/context.rs | 5 +- components/layout/display_list_builder.rs | 10 +- components/layout/fragment.rs | 51 +++++---- components/layout/inline.rs | 30 +++-- components/layout/layout_task.rs | 108 +++++++++--------- components/layout/list_item.rs | 2 +- components/layout/traversal.rs | 7 +- components/script/layout_interface.rs | 2 +- .../tests/mozilla/getBoundingClientRect.html | 61 +++++++++- 12 files changed, 191 insertions(+), 105 deletions(-) diff --git a/components/gfx/display_list/mod.rs b/components/gfx/display_list/mod.rs index 75f6cde1e54..b18a6bc054a 100644 --- a/components/gfx/display_list/mod.rs +++ b/components/gfx/display_list/mod.rs @@ -64,7 +64,7 @@ pub static BLUR_INFLATION_FACTOR: i32 = 3; /// Because the script task's GC does not trace layout, node data cannot be safely stored in layout /// data structures. Also, layout code tends to be faster when the DOM is not being accessed, for /// locality reasons. Using `OpaqueNode` enforces this invariant. -#[derive(Clone, PartialEq, Copy)] +#[derive(Clone, PartialEq, Copy, Debug)] pub struct OpaqueNode(pub uintptr_t); impl OpaqueNode { diff --git a/components/layout/block.rs b/components/layout/block.rs index c21d3123bc7..34945dc5aea 100644 --- a/components/layout/block.rs +++ b/components/layout/block.rs @@ -1914,7 +1914,7 @@ impl Flow for BlockFlow { self.base .absolute_position_info .relative_containing_block_mode, - CoordinateSystem::Parent) + CoordinateSystem::Own) .translate(stacking_context_position)); } diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 2365fe69e3b..c72de1d122a 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -31,7 +31,7 @@ use fragment::TableColumnFragmentInfo; use fragment::UnscannedTextFragmentInfo; use fragment::{InlineBlockFragmentInfo, SpecificFragmentInfo}; use incremental::{RECONSTRUCT_FLOW, RestyleDamage}; -use inline::InlineFlow; +use inline::{InlineFlow, InlineFragmentNodeInfo}; use list_item::{ListItemFlow, ListStyleTypeContent}; use multicol::MulticolFlow; use opaque_node::OpaqueNodeMethods; @@ -169,14 +169,14 @@ struct InlineFragmentsAccumulator { /// Whether we've created a range to enclose all the fragments. This will be Some() if the /// outer node is an inline and None otherwise. - enclosing_style: Option>, + enclosing_node: Option, } impl InlineFragmentsAccumulator { fn new() -> InlineFragmentsAccumulator { InlineFragmentsAccumulator { fragments: LinkedList::new(), - enclosing_style: None, + enclosing_node: None, } } @@ -184,7 +184,9 @@ impl InlineFragmentsAccumulator { let fragments = LinkedList::new(); InlineFragmentsAccumulator { fragments: fragments, - enclosing_style: Some(node.style().clone()), + enclosing_node: Some(InlineFragmentNodeInfo { + address: OpaqueNodeMethods::from_thread_safe_layout_node(node), + style: node.style().clone() }), } } @@ -199,9 +201,9 @@ impl InlineFragmentsAccumulator { fn to_dlist(self) -> LinkedList { let InlineFragmentsAccumulator { mut fragments, - enclosing_style + enclosing_node, } = self; - if let Some(enclosing_style) = enclosing_style { + if let Some(enclosing_node) = enclosing_node { let frag_len = fragments.len(); for (idx, frag) in fragments.iter_mut().enumerate() { @@ -210,7 +212,7 @@ impl InlineFragmentsAccumulator { // frag is the last inline fragment in the inline node let is_last = idx == frag_len - 1; - frag.add_inline_context_style(enclosing_style.clone(), is_first, is_last); + frag.add_inline_context_style(enclosing_node.clone(), is_first, is_last); } } fragments diff --git a/components/layout/context.rs b/components/layout/context.rs index 38996d5eea1..9a7550fd2d6 100644 --- a/components/layout/context.rs +++ b/components/layout/context.rs @@ -15,7 +15,7 @@ use gfx::font_context::FontContext; use msg::constellation_msg::ConstellationChan; use net_traits::image::base::Image; use net_traits::image_cache_task::{ImageCacheChan, ImageCacheTask, ImageState}; -use script::layout_interface::{Animation, LayoutChan}; +use script::layout_interface::{Animation, LayoutChan, ReflowGoal}; use std::boxed; use std::cell::Cell; use std::ptr; @@ -98,6 +98,9 @@ pub struct SharedLayoutContext { /// A channel on which new animations that have been triggered by style recalculation can be /// sent. pub new_animations_sender: Sender, + + /// Why is this reflow occurring + pub goal: ReflowGoal, } pub struct SharedLayoutContextWrapper(pub *const SharedLayoutContext); diff --git a/components/layout/display_list_builder.rs b/components/layout/display_list_builder.rs index 890a3b12128..bf819c4378d 100644 --- a/components/layout/display_list_builder.rs +++ b/components/layout/display_list_builder.rs @@ -867,30 +867,30 @@ impl FragmentDisplayListBuilding for Fragment { // Add shadows, background, borders, and outlines, if applicable. if let Some(ref inline_context) = self.inline_context { - for style in inline_context.styles.iter().rev() { + for node in inline_context.nodes.iter().rev() { self.build_display_list_for_box_shadow_if_applicable( - &**style, + &*node.style, display_list, layout_context, level, &stacking_relative_border_box, &clip); self.build_display_list_for_background_if_applicable( - &**style, + &*node.style, display_list, layout_context, level, &stacking_relative_border_box, &clip); self.build_display_list_for_borders_if_applicable( - &**style, + &*node.style, border_painting_mode, display_list, &stacking_relative_border_box, level, &clip); self.build_display_list_for_outline_if_applicable( - &**style, + &*node.style, display_list, &stacking_relative_border_box, &clip); diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index c7e1cd1ed04..8b60b4cfc72 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -14,7 +14,7 @@ use flow; use flow::Flow; use flow_ref::FlowRef; use incremental::{self, RestyleDamage}; -use inline::{InlineFragmentContext, InlineMetrics}; +use inline::{InlineFragmentContext, InlineFragmentNodeInfo, InlineMetrics}; use layout_debug; use model::{self, IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto, specified}; use text; @@ -840,32 +840,37 @@ impl Fragment { self.restyle_damage | self.specific.restyle_damage() } + pub fn contains_node(&self, node_address: OpaqueNode) -> bool { + node_address == self.node || + self.inline_context.as_ref().map_or(false, |ctx| { + ctx.contains_node(node_address) + }) + } + /// Adds a style to the inline context for this fragment. If the inline context doesn't exist /// yet, it will be created. pub fn add_inline_context_style(&mut self, - style: Arc, + mut node_info: InlineFragmentNodeInfo, first_frag: bool, last_frag: bool) { if self.inline_context.is_none() { self.inline_context = Some(InlineFragmentContext::new()); } - let frag_style = if first_frag && last_frag { - style.clone() - } else { + if !first_frag || !last_frag { // Set the border width to zero and the border style to none on // border sides that are not the outermost for a node container. // Because with multiple inline fragments they don't have interior // borders separating each other. - let mut border_width = style.logical_border_width(); + let mut border_width = node_info.style.logical_border_width(); if !last_frag { - border_width.set_right(style.writing_mode, Zero::zero()); + border_width.set_right(node_info.style.writing_mode, Zero::zero()); } if !first_frag { - border_width.set_left(style.writing_mode, Zero::zero()); + border_width.set_left(node_info.style.writing_mode, Zero::zero()); } - Arc::new(make_border(&*style, border_width)) + node_info.style = Arc::new(make_border(&*node_info.style, border_width)) }; - self.inline_context.as_mut().unwrap().styles.push(frag_style); + self.inline_context.as_mut().unwrap().nodes.push(node_info); } /// Determines which quantities (border/padding/margin/specified) should be included in the @@ -1000,10 +1005,10 @@ impl Fragment { match self.inline_context { None => style_border_width, Some(ref inline_fragment_context) => { - inline_fragment_context.styles + inline_fragment_context.nodes .iter() .fold(style_border_width, - |acc, style| acc + style.logical_border_width()) + |acc, node| acc + node.style.logical_border_width()) } } } @@ -1092,10 +1097,10 @@ impl Fragment { match self.inline_context { None => style_padding, Some(ref inline_fragment_context) => { - inline_fragment_context.styles + inline_fragment_context.nodes .iter() - .fold(style_padding, |acc, style| { - acc + model::padding_from_style(&**style, + .fold(style_padding, |acc, node| { + acc + model::padding_from_style(&*node.style, Au(0)) }) } @@ -1136,9 +1141,9 @@ impl Fragment { }; if let Some(ref inline_fragment_context) = self.inline_context { - for style in inline_fragment_context.styles.iter() { - if style.get_box().position == position::T::relative { - rel_pos = rel_pos + from_style(&**style, containing_block_size); + for node in inline_fragment_context.nodes.iter() { + if node.style.get_box().position == position::T::relative { + rel_pos = rel_pos + from_style(&*node.style, containing_block_size); } } } @@ -1282,10 +1287,10 @@ impl Fragment { // Take borders and padding for parent inline fragments into account, if necessary. if self.is_primary_fragment() { if let Some(ref context) = self.inline_context { - for style in context.styles.iter() { - let border_width = style.logical_border_width().inline_start_end(); + for node in context.nodes.iter() { + let border_width = node.style.logical_border_width().inline_start_end(); let padding_inline_size = - model::padding_from_style(&**style, Au(0)).inline_start_end(); + model::padding_from_style(&*node.style, Au(0)).inline_start_end(); result.surrounding_size = result.surrounding_size + border_width + padding_inline_size; } @@ -2097,11 +2102,11 @@ impl<'a> Iterator for InlineStyleIterator<'a> { Some(ref inline_context) => inline_context, }; let inline_style_index = self.inline_style_index; - if inline_style_index == inline_context.styles.len() { + if inline_style_index == inline_context.nodes.len() { return None } self.inline_style_index += 1; - Some(&*inline_context.styles[inline_style_index]) + Some(&*inline_context.nodes[inline_style_index].style) } } diff --git a/components/layout/inline.rs b/components/layout/inline.rs index d2e48630ee8..8c0d5723695 100644 --- a/components/layout/inline.rs +++ b/components/layout/inline.rs @@ -17,6 +17,7 @@ use text; use collections::VecDeque; use geom::{Point2D, Rect}; +use gfx::display_list::OpaqueNode; use gfx::font::FontMetrics; use gfx::font_context::FontContext; use gfx::text::glyph::CharIndex; @@ -1088,10 +1089,10 @@ impl InlineFlow { for frag in self.fragments.fragments.iter() { match frag.inline_context { Some(ref inline_context) => { - for style in inline_context.styles.iter() { - let font_style = style.get_font_arc(); + for node in inline_context.nodes.iter() { + let font_style = node.style.get_font_arc(); let font_metrics = text::font_metrics_for_style(font_context, font_style); - let line_height = text::line_height_from_style(&**style, &font_metrics); + let line_height = text::line_height_from_style(&*node.style, &font_metrics); let inline_metrics = InlineMetrics::from_font_metrics(&font_metrics, line_height); block_size_above_baseline = max(block_size_above_baseline, @@ -1476,7 +1477,7 @@ impl Flow for InlineFlow { &fragment.stacking_relative_border_box(stacking_relative_position, relative_containing_block_size, relative_containing_block_mode, - CoordinateSystem::Parent) + CoordinateSystem::Own) .translate(stacking_context_position)) } } @@ -1494,24 +1495,35 @@ impl fmt::Debug for InlineFlow { } } +#[derive(Clone)] +pub struct InlineFragmentNodeInfo { + pub address: OpaqueNode, + pub style: Arc, +} + #[derive(Clone)] pub struct InlineFragmentContext { - pub styles: Vec>, + pub nodes: Vec, } impl InlineFragmentContext { pub fn new() -> InlineFragmentContext { InlineFragmentContext { - styles: vec!() + nodes: vec!(), } } + #[inline] + pub fn contains_node(&self, node_address: OpaqueNode) -> bool { + self.nodes.iter().position(|node| node.address == node_address).is_some() + } + fn ptr_eq(&self, other: &InlineFragmentContext) -> bool { - if self.styles.len() != other.styles.len() { + if self.nodes.len() != other.nodes.len() { return false } - for (this_style, other_style) in self.styles.iter().zip(other.styles.iter()) { - if !util::arc_ptr_eq(this_style, other_style) { + for (this_node, other_node) in self.nodes.iter().zip(other.nodes.iter()) { + if !util::arc_ptr_eq(&this_node.style, &other_node.style) { return false } } diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 6166b95f73b..2642f2dd151 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -350,7 +350,8 @@ impl LayoutTask { rw_data: &LayoutTaskData, screen_size_changed: bool, reflow_root: Option<&LayoutNode>, - url: &Url) + url: &Url, + goal: ReflowGoal) -> SharedLayoutContext { SharedLayoutContext { image_cache_task: rw_data.image_cache_task.clone(), @@ -366,6 +367,7 @@ impl LayoutTask { dirty: Rect::zero(), generation: rw_data.generation, new_animations_sender: rw_data.new_animations_sender.clone(), + goal: goal, } } @@ -467,7 +469,8 @@ impl LayoutTask { let mut layout_context = self.build_shared_layout_context(&*rw_data, false, None, - &self.url); + &self.url, + reflow_info.goal); self.perform_post_style_recalc_layout_passes(&reflow_info, &mut *rw_data, @@ -751,11 +754,11 @@ impl LayoutTask { rw_data.content_boxes_response = iterator.rects; } - fn build_display_list_for_reflow<'a>(&'a self, - data: &Reflow, - layout_root: &mut FlowRef, - shared_layout_context: &mut SharedLayoutContext, - rw_data: &mut LayoutTaskData) { + fn compute_abs_pos_and_build_display_list<'a>(&'a self, + data: &Reflow, + layout_root: &mut FlowRef, + shared_layout_context: &mut SharedLayoutContext, + rw_data: &mut LayoutTaskData) { let writing_mode = flow::base(&**layout_root).writing_mode; profile(time::ProfilerCategory::LayoutDispListBuild, self.profiler_metadata(), @@ -773,7 +776,8 @@ impl LayoutTask { match rw_data.parallel_traversal { None => { - sequential::build_display_list_for_subtree(layout_root, shared_layout_context); + sequential::build_display_list_for_subtree(layout_root, + shared_layout_context); } Some(ref mut traversal) => { parallel::build_display_list_for_subtree(layout_root, @@ -784,40 +788,42 @@ impl LayoutTask { } } - debug!("Done building display list."); + if data.goal == ReflowGoal::ForDisplay { + debug!("Done building display list."); - let root_background_color = get_root_flow_background_color(&mut **layout_root); - let root_size = { - let root_flow = flow::base(&**layout_root); - root_flow.position.size.to_physical(root_flow.writing_mode) - }; - let mut display_list = box DisplayList::new(); - flow::mut_base(&mut **layout_root).display_list_building_result - .add_to(&mut *display_list); - let paint_layer = Arc::new(PaintLayer::new(layout_root.layer_id(0), - root_background_color, - ScrollPolicy::Scrollable)); - let origin = Rect(Point2D(Au(0), Au(0)), root_size); + let root_background_color = get_root_flow_background_color(&mut **layout_root); + let root_size = { + let root_flow = flow::base(&**layout_root); + root_flow.position.size.to_physical(root_flow.writing_mode) + }; + let mut display_list = box DisplayList::new(); + flow::mut_base(&mut **layout_root).display_list_building_result + .add_to(&mut *display_list); + let paint_layer = Arc::new(PaintLayer::new(layout_root.layer_id(0), + root_background_color, + ScrollPolicy::Scrollable)); + let origin = Rect(Point2D(Au(0), Au(0)), root_size); - if opts::get().dump_display_list { - println!("#### start printing display list."); - display_list.print_items(String::from_str("#")); + if opts::get().dump_display_list { + println!("#### start printing display list."); + display_list.print_items(String::from_str("#")); + } + + let stacking_context = Arc::new(StackingContext::new(display_list, + &origin, + &origin, + 0, + &Matrix2D::identity(), + filter::T::new(Vec::new()), + mix_blend_mode::T::normal, + Some(paint_layer))); + + rw_data.stacking_context = Some(stacking_context.clone()); + + debug!("Layout done!"); + + self.paint_chan.send(PaintMsg::PaintInit(stacking_context)); } - - let stacking_context = Arc::new(StackingContext::new(display_list, - &origin, - &origin, - 0, - &Matrix2D::identity(), - filter::T::new(Vec::new()), - mix_blend_mode::T::normal, - Some(paint_layer))); - - rw_data.stacking_context = Some(stacking_context.clone()); - - debug!("Layout done!"); - - self.paint_chan.send(PaintMsg::PaintInit(stacking_context)); }); } @@ -878,7 +884,8 @@ impl LayoutTask { let mut shared_layout_context = self.build_shared_layout_context(&*rw_data, screen_size_changed, Some(&node), - &self.url); + &self.url, + data.reflow_info.goal); // Recalculate CSS styles and rebuild flows and fragments. profile(time::ProfilerCategory::LayoutStyleRecalc, @@ -946,7 +953,8 @@ impl LayoutTask { let mut layout_context = self.build_shared_layout_context(&*rw_data, false, None, - &self.url); + &self.url, + reflow_info.goal); let mut root_flow = (*rw_data.root_flow.as_ref().unwrap()).clone(); profile(time::ProfilerCategory::LayoutStyleRecalc, self.profiler_metadata(), @@ -1010,16 +1018,10 @@ impl LayoutTask { }); // Build the display list if necessary, and send it to the painter. - match data.goal { - ReflowGoal::ForDisplay => { - self.build_display_list_for_reflow(data, - &mut root_flow, - &mut *layout_context, - rw_data); - } - ReflowGoal::ForScriptQuery => {} - } - + self.compute_abs_pos_and_build_display_list(data, + &mut root_flow, + &mut *layout_context, + rw_data); self.first_reflow.set(false); if opts::get().trace_layout { @@ -1182,7 +1184,7 @@ impl FragmentBorderBoxIterator for UnioningFragmentBorderBoxIterator { } fn should_process(&mut self, fragment: &Fragment) -> bool { - self.node_address == fragment.node + fragment.contains_node(self.node_address) } } @@ -1206,7 +1208,7 @@ impl FragmentBorderBoxIterator for CollectingFragmentBorderBoxIterator { } fn should_process(&mut self, fragment: &Fragment) -> bool { - self.node_address == fragment.node + fragment.contains_node(self.node_address) } } diff --git a/components/layout/list_item.rs b/components/layout/list_item.rs index 8bf3fee6880..6dff3b48c95 100644 --- a/components/layout/list_item.rs +++ b/components/layout/list_item.rs @@ -172,7 +172,7 @@ impl Flow for ListItemFlow { .base .absolute_position_info .relative_containing_block_mode, - CoordinateSystem::Parent) + CoordinateSystem::Own) .translate(stacking_context_position)); } } diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index 0441f4fa76a..cfedc0ae3f1 100644 --- a/components/layout/traversal.rs +++ b/components/layout/traversal.rs @@ -14,6 +14,7 @@ use flow::{Flow, MutableFlowUtils}; use flow::{PreorderFlowTraversal, PostorderFlowTraversal}; use flow; use incremental::{self, BUBBLE_ISIZES, REFLOW, REFLOW_OUT_OF_FLOW, RestyleDamage}; +use script::layout_interface::ReflowGoal; use wrapper::{layout_node_to_unsafe_layout_node, LayoutNode}; use wrapper::{PostorderNodeMutTraversal, ThreadSafeLayoutNode, UnsafeLayoutNode}; use wrapper::{PreorderDomTraversal, PostorderDomTraversal}; @@ -378,5 +379,9 @@ impl<'a> PostorderFlowTraversal for BuildDisplayList<'a> { fn process(&self, flow: &mut Flow) { flow.build_display_list(self.layout_context); } -} + #[inline] + fn should_process(&self, _: &mut Flow) -> bool { + self.layout_context.shared.goal == ReflowGoal::ForDisplay + } +} diff --git a/components/script/layout_interface.rs b/components/script/layout_interface.rs index 4859c075058..f94fb29c18d 100644 --- a/components/script/layout_interface.rs +++ b/components/script/layout_interface.rs @@ -88,7 +88,7 @@ pub struct HitTestResponse(pub UntrustedNodeAddress); pub struct MouseOverResponse(pub Vec); /// Why we're doing reflow. -#[derive(PartialEq, Debug)] +#[derive(PartialEq, Copy, Debug)] pub enum ReflowGoal { /// We're reflowing in order to send a display list to the screen. ForDisplay, diff --git a/tests/wpt/mozilla/tests/mozilla/getBoundingClientRect.html b/tests/wpt/mozilla/tests/mozilla/getBoundingClientRect.html index 8d65b672072..6571fedabed 100644 --- a/tests/wpt/mozilla/tests/mozilla/getBoundingClientRect.html +++ b/tests/wpt/mozilla/tests/mozilla/getBoundingClientRect.html @@ -3,18 +3,68 @@ -
my div
+
my div
+
+
+
+ X +
+
+