From 44a11a7c6c371b5bc9042aca3d37ec6ccac15706 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Wed, 6 Aug 2025 15:46:43 +0200 Subject: [PATCH] script/layout: Ensure a StackingContextTree before IntersectionObserver geometry queries (#38473) IntersectionObserver needs to be able to query node geometry without forcing a layout. A previous layout could have run without needing a `StackingContextTree`. In that case the layout-less query should finish building the `StackingContextTree` before doing the query. Add a new type of layout API which requests that layout finishes building the StackingContextTree. This change also slightly simplifies and corrects the naming of `Element` APIs around client box queries. Testing: This should fix intermittent failures in WPT tests. Fixes: #38380. Fixes: #38390. Closes: #38400. Signed-off-by: Martin Robinson --- components/layout/layout_impl.rs | 64 ++++++++++++------- components/script/dom/document.rs | 4 +- components/script/dom/element.rs | 8 +-- components/script/dom/htmlanchorelement.rs | 2 +- components/script/dom/htmlimageelement.rs | 14 ++-- components/script/dom/htmlinputelement.rs | 2 +- components/script/dom/htmlselectelement.rs | 2 +- components/script/dom/intersectionobserver.rs | 7 +- components/script/dom/node.rs | 12 +--- components/script/dom/window.rs | 15 +++-- components/shared/layout/lib.rs | 4 ++ .../fixed-position-iframe-scroll.html.ini | 3 + 12 files changed, 78 insertions(+), 59 deletions(-) create mode 100644 tests/wpt/meta/intersection-observer/fixed-position-iframe-scroll.html.ini diff --git a/components/layout/layout_impl.rs b/components/layout/layout_impl.rs index bca7f7bee59..ba0a76ec8be 100644 --- a/components/layout/layout_impl.rs +++ b/components/layout/layout_impl.rs @@ -261,11 +261,17 @@ impl Layout for LayoutThread { /// query and possibly, query without consideration of transform. #[servo_tracing::instrument(skip_all)] fn query_content_box(&self, node: TrustedNodeAddress) -> Option> { + // If we have not built a fragment tree yet, there is no way we have layout information for + // this query, which can be run without forcing a layout (for IntersectionObserver). + if self.fragment_tree.borrow().is_none() { + return None; + } + let node = unsafe { ServoLayoutNode::new(&node) }; let stacking_context_tree = self.stacking_context_tree.borrow(); let stacking_context_tree = stacking_context_tree .as_ref() - .expect("Should always have a StackingContextTree for geometry queries"); + .expect("Should always have a StackingContextTree for content box queries"); process_content_box_request(stacking_context_tree, node) } @@ -275,11 +281,17 @@ impl Layout for LayoutThread { /// See . #[servo_tracing::instrument(skip_all)] fn query_content_boxes(&self, node: TrustedNodeAddress) -> Vec> { + // If we have not built a fragment tree yet, there is no way we have layout information for + // this query, which can be run without forcing a layout (for IntersectionObserver). + if self.fragment_tree.borrow().is_none() { + return vec![]; + } + let node = unsafe { ServoLayoutNode::new(&node) }; let stacking_context_tree = self.stacking_context_tree.borrow(); let stacking_context_tree = stacking_context_tree .as_ref() - .expect("Should always have a StackingContextTree for geometry queries"); + .expect("Should always have a StackingContextTree for content box queries"); process_content_boxes_request(stacking_context_tree, node) } @@ -454,6 +466,15 @@ impl Layout for LayoutThread { ) } + fn ensure_stacking_context_tree(&self, viewport_details: ViewportDetails) { + if self.stacking_context_tree.borrow().is_some() && + !self.need_new_stacking_context_tree.get() + { + return; + } + self.build_stacking_context_tree(viewport_details); + } + fn register_paint_worklet_modules( &mut self, _name: Atom, @@ -696,7 +717,7 @@ impl LayoutThread { if self.calculate_overflow(damage) { reflow_phases_run.insert(ReflowPhasesRun::CalculatedOverflow); } - if self.build_stacking_context_tree(&reflow_request, damage) { + if self.build_stacking_context_tree_for_reflow(&reflow_request, damage) { reflow_phases_run.insert(ReflowPhasesRun::BuiltStackingContextTree); } if self.build_display_list(&reflow_request, damage, &image_resolver) { @@ -976,8 +997,7 @@ impl LayoutThread { true } - #[servo_tracing::instrument(name = "Stacking Context Tree Construction", skip_all)] - fn build_stacking_context_tree( + fn build_stacking_context_tree_for_reflow( &self, reflow_request: &ReflowRequest, damage: RestyleDamage, @@ -987,14 +1007,19 @@ impl LayoutThread { { return false; } - let Some(fragment_tree) = &*self.fragment_tree.borrow() else { - return false; - }; if !damage.contains(RestyleDamage::REBUILD_STACKING_CONTEXT) && !self.need_new_stacking_context_tree.get() { return false; } + self.build_stacking_context_tree(reflow_request.viewport_details) + } + + #[servo_tracing::instrument(name = "Stacking Context Tree Construction", skip_all)] + fn build_stacking_context_tree(&self, viewport_details: ViewportDetails) -> bool { + let Some(fragment_tree) = &*self.fragment_tree.borrow() else { + return false; + }; let mut stacking_context_tree = self.stacking_context_tree.borrow_mut(); let old_scroll_offsets = stacking_context_tree @@ -1006,7 +1031,7 @@ impl LayoutThread { // applicable spatial and clip nodes. let mut new_stacking_context_tree = StackingContextTree::new( fragment_tree, - reflow_request.viewport_details, + viewport_details, self.id.into(), !self.have_ever_generated_display_list.get(), &self.debug, @@ -1022,6 +1047,13 @@ impl LayoutThread { .set_all_scroll_offsets(&old_scroll_offsets); } + if self.debug.dump_scroll_tree { + new_stacking_context_tree + .compositor_info + .scroll_tree + .debug_print(); + } + *stacking_context_tree = Some(new_stacking_context_tree); // Force display list generation as layout has changed. @@ -1030,20 +1062,6 @@ impl LayoutThread { // The stacking context tree is up-to-date again. self.need_new_stacking_context_tree.set(false); - if self.debug.dump_scroll_tree { - // Print the [ScrollTree], this is done after display list build so we have - // the information about webrender id. Whether a scroll tree is initialized - // or not depends on the reflow goal. - if let Some(tree) = self.stacking_context_tree.borrow().as_ref() { - tree.compositor_info.scroll_tree.debug_print(); - } else { - println!( - "Scroll Tree -- reflow {:?}: scroll tree is not initialized yet.", - reflow_request.reflow_goal - ); - } - } - true } diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 8ab8f6bb7bc..88f8b1c82c1 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1056,7 +1056,7 @@ impl Document { // inside other scrollable containers. Ideally this should use an implementation of // `scrollIntoView` when that is available: // See https://github.com/servo/servo/issues/24059. - let rect = element.upcast::().bounding_content_box_or_zero(); + let rect = element.upcast::().content_box().unwrap_or_default(); // In order to align with element edges, we snap to unscaled pixel boundaries, since // the paint thread currently does the same for drawing elements. This is important @@ -1356,7 +1356,7 @@ impl Document { // Notify the embedder to display an input method. if let Some(kind) = elem.input_method_type() { - let rect = elem.upcast::().bounding_content_box_or_zero(); + let rect = elem.upcast::().content_box().unwrap_or_default(); let rect = Rect::new( Point2D::new(rect.origin.x.to_px(), rect.origin.y.to_px()), Size2D::new(rect.size.width.to_px(), rect.size.height.to_px()), diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index d040ec86d57..4f044dbc14b 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -1005,7 +1005,7 @@ impl Element { block: ScrollLogicalPosition, inline: ScrollLogicalPosition, ) -> ScrollPosition { - let target_bounding_box = self.upcast::().bounding_content_box_or_zero(); + let target_bounding_box = self.upcast::().content_box().unwrap_or_default(); let device_pixel_ratio = self .upcast::() @@ -1071,7 +1071,7 @@ impl Element { } else { // Handle element-specific scrolling // Scrolling box bounds and current scroll position - let scrolling_box = scrolling_node.bounding_content_box_or_zero(); + let scrolling_box = scrolling_node.content_box().unwrap_or_default(); let scrolling_left = scrolling_box.origin.x.to_nearest_pixel(device_pixel_ratio) as f64; let scrolling_top = scrolling_box.origin.y.to_nearest_pixel(device_pixel_ratio) as f64; let scrolling_width = scrolling_box @@ -1125,7 +1125,7 @@ impl Element { if matches!(position, Position::Relative | Position::Absolute) { // If this element establishes a positioning context, // Get its bounding box to calculate the offset - let positioning_box = node.bounding_content_box_or_zero(); + let positioning_box = node.content_box().unwrap_or_default(); let positioning_left = positioning_box .origin .x @@ -3458,7 +3458,7 @@ impl ElementMethods for Element { // https://drafts.csswg.org/cssom-view/#dom-element-getboundingclientrect fn GetBoundingClientRect(&self, can_gc: CanGc) -> DomRoot { let win = self.owner_window(); - let rect = self.upcast::().bounding_content_box_or_zero(); + let rect = self.upcast::().content_box().unwrap_or_default(); DOMRect::new( win.upcast(), rect.origin.x.to_f64_px(), diff --git a/components/script/dom/htmlanchorelement.rs b/components/script/dom/htmlanchorelement.rs index 514126b214b..8c1b61f96f6 100644 --- a/components/script/dom/htmlanchorelement.rs +++ b/components/script/dom/htmlanchorelement.rs @@ -329,7 +329,7 @@ impl Activatable for HTMLAnchorElement { if let Some(element) = target.downcast::() { if target.is::() && element.has_attribute(&local_name!("ismap")) { let target_node = element.upcast::(); - let rect = target_node.bounding_content_box_or_zero(); + let rect = target_node.content_box().unwrap_or_default(); ismap_suffix = Some(format!( "?{},{}", mouse_event.ClientX().to_f32().unwrap() - rect.origin.x.to_f32_px(), diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index ac17f6094bd..cf2a764b509 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -1657,10 +1657,9 @@ impl HTMLImageElementMethods for HTMLImageElement { // https://html.spec.whatwg.org/multipage/#dom-img-width fn Width(&self) -> u32 { let node = self.upcast::(); - match node.bounding_content_box() { - Some(rect) => rect.size.width.to_px() as u32, - None => self.NaturalWidth(), - } + node.content_box() + .map(|rect| rect.size.width.to_px() as u32) + .unwrap_or_else(|| self.NaturalWidth()) } // https://html.spec.whatwg.org/multipage/#dom-img-width @@ -1671,10 +1670,9 @@ impl HTMLImageElementMethods for HTMLImageElement { // https://html.spec.whatwg.org/multipage/#dom-img-height fn Height(&self) -> u32 { let node = self.upcast::(); - match node.bounding_content_box() { - Some(rect) => rect.size.height.to_px() as u32, - None => self.NaturalHeight(), - } + node.content_box() + .map(|rect| rect.size.height.to_px() as u32) + .unwrap_or_else(|| self.NaturalHeight()) } // https://html.spec.whatwg.org/multipage/#dom-img-height diff --git a/components/script/dom/htmlinputelement.rs b/components/script/dom/htmlinputelement.rs index a9966a425a6..c0467c31e32 100644 --- a/components/script/dom/htmlinputelement.rs +++ b/components/script/dom/htmlinputelement.rs @@ -2736,7 +2736,7 @@ impl HTMLInputElement { let (ipc_sender, ipc_receiver) = ipc::channel::>().expect("Failed to create IPC channel!"); let document = self.owner_document(); - let rect = self.upcast::().bounding_content_box_or_zero(); + let rect = self.upcast::().content_box().unwrap_or_default(); let rect = Rect::new( Point2D::new(rect.origin.x.to_px(), rect.origin.y.to_px()), Size2D::new(rect.size.width.to_px(), rect.size.height.to_px()), diff --git a/components/script/dom/htmlselectelement.rs b/components/script/dom/htmlselectelement.rs index e4ca7a247be..aa30daeef79 100644 --- a/components/script/dom/htmlselectelement.rs +++ b/components/script/dom/htmlselectelement.rs @@ -377,7 +377,7 @@ impl HTMLSelectElement { }) .collect(); - let rect = self.upcast::().bounding_content_box_or_zero(); + let rect = self.upcast::().content_box().unwrap_or_default(); let rect = Rect::new( Point2D::new(rect.origin.x.to_px(), rect.origin.y.to_px()), Size2D::new(rect.size.width.to_px(), rect.size.height.to_px()), diff --git a/components/script/dom/intersectionobserver.rs b/components/script/dom/intersectionobserver.rs index 6a6f9ce45eb..172e83d58d7 100644 --- a/components/script/dom/intersectionobserver.rs +++ b/components/script/dom/intersectionobserver.rs @@ -408,6 +408,7 @@ impl IntersectionObserver { /// /// pub(crate) fn root_intersection_rectangle(&self, document: &Document) -> Option> { + let window = document.window(); let intersection_rectangle = match &self.root { // Handle if root is an element. Some(ElementOrDocument::Element(element)) => { @@ -419,7 +420,7 @@ impl IntersectionObserver { // > Otherwise, it’s the result of getting the bounding box for the intersection root. // TODO: replace this once getBoundingBox() is implemented correctly. - DomRoot::upcast::(element.clone()).bounding_content_box_no_reflow() + window.content_box_query_without_reflow(&DomRoot::upcast::(element.clone())) }, // Handle if root is a Document, which includes implicit root and explicit Document root. _ => { @@ -503,7 +504,9 @@ impl IntersectionObserver { // This is what we are currently using for getBoundingBox(). However, it is not correct, // mainly because it is not considering transform and scroll offset. // TODO: replace this once getBoundingBox() is implemented correctly. - let maybe_target_rect = target.upcast::().bounding_content_box_no_reflow(); + let maybe_target_rect = document + .window() + .content_box_query_without_reflow(target.upcast::()); // Following the implementation of Gecko, we will skip further processing if these // information not available. This would also handle display none element. diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index aae644cc010..4fdc4471540 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -942,20 +942,10 @@ impl Node { TrustedNodeAddress(self as *const Node as *const libc::c_void) } - /// Returns the rendered bounding content box if the element is rendered, - /// and none otherwise. - pub(crate) fn bounding_content_box(&self) -> Option> { + pub(crate) fn content_box(&self) -> Option> { self.owner_window().content_box_query(self) } - pub(crate) fn bounding_content_box_or_zero(&self) -> Rect { - self.bounding_content_box().unwrap_or_else(Rect::zero) - } - - pub(crate) fn bounding_content_box_no_reflow(&self) -> Option> { - self.owner_window().content_box_query_unchecked(self) - } - pub(crate) fn content_boxes(&self) -> Vec> { self.owner_window().content_boxes_query(self) } diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index b9af64afcf5..f487ba2b16a 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -2434,16 +2434,19 @@ impl Window { ) } - // Query content box without considering any reflow - pub(crate) fn content_box_query_unchecked(&self, node: &Node) -> Option> { - self.layout - .borrow() - .query_content_box(node.to_trusted_node_address()) + /// Do the same kind of query as `Self::content_box_query`, but do not force a reflow. + /// This is used for things like `IntersectionObserver` which should observe the value + /// from the most recent reflow, but do not need it to reflect the current state of + /// the DOM / style. + pub(crate) fn content_box_query_without_reflow(&self, node: &Node) -> Option> { + let layout = self.layout.borrow(); + layout.ensure_stacking_context_tree(self.viewport_details.get()); + layout.query_content_box(node.to_trusted_node_address()) } pub(crate) fn content_box_query(&self, node: &Node) -> Option> { self.layout_reflow(QueryMsg::ContentBox); - self.content_box_query_unchecked(node) + self.content_box_query_without_reflow(node) } pub(crate) fn content_boxes_query(&self, node: &Node) -> Vec> { diff --git a/components/shared/layout/lib.rs b/components/shared/layout/lib.rs index 53ce56c6ca8..0a228eaa8ee 100644 --- a/components/shared/layout/lib.rs +++ b/components/shared/layout/lib.rs @@ -241,6 +241,10 @@ pub trait Layout { /// Requests a reflow. fn reflow(&mut self, reflow_request: ReflowRequest) -> Option; + /// Do not request a reflow, but ensure that any previous reflow completes building a stacking + /// context tree so that it is ready to query the final size of any elements in script. + fn ensure_stacking_context_tree(&self, viewport_details: ViewportDetails); + /// Tells layout that script has added some paint worklet modules. fn register_paint_worklet_modules( &mut self, diff --git a/tests/wpt/meta/intersection-observer/fixed-position-iframe-scroll.html.ini b/tests/wpt/meta/intersection-observer/fixed-position-iframe-scroll.html.ini new file mode 100644 index 00000000000..5a2ef6cfe8b --- /dev/null +++ b/tests/wpt/meta/intersection-observer/fixed-position-iframe-scroll.html.ini @@ -0,0 +1,3 @@ +[fixed-position-iframe-scroll.html] + [scrollTo(0, 1000)] + expected: FAIL