From e5a1af5b7ae276a3a47c4b6c5a85b210ab7753a1 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 12 Jan 2016 20:57:23 -0800 Subject: [PATCH 1/3] compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. Closes #8081. --- components/compositing/compositor.rs | 76 ++++++++++++------- components/compositing/compositor_layer.rs | 1 + components/compositing/constellation.rs | 11 +++ components/script/dom/window.rs | 2 + components/script/script_thread.rs | 18 +++++ components/script_traits/lib.rs | 2 + tests/wpt/mozilla/meta/MANIFEST.json | 24 ++++++ .../mozilla/tests/css/iframe/navigation.html | 28 +++++++ .../tests/css/iframe/navigation_ref.html | 17 +++++ 9 files changed, 153 insertions(+), 26 deletions(-) create mode 100644 tests/wpt/mozilla/tests/css/iframe/navigation.html create mode 100644 tests/wpt/mozilla/tests/css/iframe/navigation_ref.html diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 84d101ac9d1..3ada78486cd 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -790,6 +790,13 @@ impl IOCompositor { match self.find_layer_with_pipeline_and_layer_id(pipeline_id, properties.id) { Some(existing_layer) => { + // If this layer contains a subpage, then create the root layer for that subpage + // now. + if properties.subpage_pipeline_id.is_some() { + self.create_root_layer_for_subpage_if_necessary(properties, + existing_layer.clone()) + } + existing_layer.update_layer(properties); true } @@ -868,32 +875,9 @@ impl IOCompositor { } // If this layer contains a subpage, then create the root layer for that subpage now. - if let Some(ref subpage_pipeline_id) = layer_properties.subpage_pipeline_id { - let subpage_layer_properties = LayerProperties { - id: LayerId::null(), - parent_id: None, - rect: Rect::new(Point2D::zero(), layer_properties.rect.size), - background_color: layer_properties.background_color, - scroll_policy: ScrollPolicy::Scrollable, - transform: Matrix4::identity(), - perspective: Matrix4::identity(), - subpage_pipeline_id: layer_properties.subpage_pipeline_id, - establishes_3d_context: true, - scrolls_overflow_area: true, - }; - - let wants_scroll_events = if subpage_layer_properties.scrolls_overflow_area { - WantsScrollEventsFlag::WantsScrollEvents - } else { - WantsScrollEventsFlag::DoesntWantScrollEvents - }; - let subpage_layer = CompositorData::new_layer(*subpage_pipeline_id, - subpage_layer_properties, - wants_scroll_events, - new_layer.tile_size); - *subpage_layer.masks_to_bounds.borrow_mut() = true; - new_layer.add_child(subpage_layer); - self.pending_subpages.insert(*subpage_pipeline_id); + if layer_properties.subpage_pipeline_id.is_some() { + self.create_root_layer_for_subpage_if_necessary(layer_properties, + new_layer.clone()) } parent_layer.add_child(new_layer.clone()); @@ -902,6 +886,46 @@ impl IOCompositor { self.dump_layer_tree(); } + fn create_root_layer_for_subpage_if_necessary(&mut self, + layer_properties: LayerProperties, + parent_layer: Rc>) { + if parent_layer.children + .borrow() + .iter() + .any(|child| child.extra_data.borrow().subpage_info.is_some()) { + return + } + + let subpage_pipeline_id = + layer_properties.subpage_pipeline_id + .expect("create_root_layer_for_subpage() called for non-subpage?!"); + let subpage_layer_properties = LayerProperties { + id: LayerId::null(), + parent_id: None, + rect: Rect::new(Point2D::zero(), layer_properties.rect.size), + background_color: layer_properties.background_color, + scroll_policy: ScrollPolicy::Scrollable, + transform: Matrix4::identity(), + perspective: Matrix4::identity(), + subpage_pipeline_id: Some(subpage_pipeline_id), + establishes_3d_context: true, + scrolls_overflow_area: true, + }; + + let wants_scroll_events = if subpage_layer_properties.scrolls_overflow_area { + WantsScrollEventsFlag::WantsScrollEvents + } else { + WantsScrollEventsFlag::DoesntWantScrollEvents + }; + let subpage_layer = CompositorData::new_layer(subpage_pipeline_id, + subpage_layer_properties, + wants_scroll_events, + parent_layer.tile_size); + *subpage_layer.masks_to_bounds.borrow_mut() = true; + parent_layer.add_child(subpage_layer); + self.pending_subpages.insert(subpage_pipeline_id); + } + fn send_window_size(&self) { let dppx = self.page_zoom * self.device_pixels_per_screen_px(); let initial_viewport = self.window_size.as_f32() / dppx; diff --git a/components/compositing/compositor_layer.rs b/components/compositing/compositor_layer.rs index 9e2b47a5c08..8246f973b66 100644 --- a/components/compositing/compositor_layer.rs +++ b/components/compositing/compositor_layer.rs @@ -211,6 +211,7 @@ pub enum ScrollEventResult { impl CompositorLayer for Layer { fn update_layer_except_bounds(&self, layer_properties: LayerProperties) { self.extra_data.borrow_mut().scroll_policy = layer_properties.scroll_policy; + self.extra_data.borrow_mut().subpage_info = layer_properties.subpage_pipeline_id; *self.transform.borrow_mut() = layer_properties.transform; *self.perspective.borrow_mut() = layer_properties.perspective; diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 62170e3f5d4..829b9ead234 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -1325,6 +1325,17 @@ impl Constellation fn handle_activate_document_msg(&mut self, pipeline_id: PipelineId) { debug!("Document ready to activate {:?}", pipeline_id); + if let Some(ref child_pipeline) = self.pipelines.get(&pipeline_id) { + if let Some(ref parent_info) = child_pipeline.parent_info { + if let Some(parent_pipeline) = self.pipelines.get(&parent_info.0) { + let _ = parent_pipeline.script_chan + .send(ConstellationControlMsg::FramedContentChanged( + parent_info.0, + parent_info.1)); + } + } + } + // If this pipeline is already part of the current frame tree, // we don't need to do anything. if self.pipeline_is_in_current_frame(pipeline_id) { diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 46cda6c4aa4..4c26c15e6f5 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -107,6 +107,7 @@ pub enum ReflowReason { ImageLoaded, RequestAnimationFrame, WebFontLoaded, + FramedContentChanged, } pub type ScrollPoint = Point2D; @@ -1425,6 +1426,7 @@ fn debug_reflow_events(goal: &ReflowGoal, query_type: &ReflowQueryType, reason: ReflowReason::ImageLoaded => "\tImageLoaded", ReflowReason::RequestAnimationFrame => "\tRequestAnimationFrame", ReflowReason::WebFontLoaded => "\tWebFontLoaded", + ReflowReason::FramedContentChanged => "\tFramedContentChanged", }); println!("{}", debug_msg); diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 663d87f46ee..ef55e608be8 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1131,6 +1131,8 @@ impl ScriptThread { ConstellationControlMsg::DispatchFrameLoadEvent { target: pipeline_id, parent: containing_id } => self.handle_frame_load_event(containing_id, pipeline_id), + ConstellationControlMsg::FramedContentChanged(containing_pipeline_id, subpage_id) => + self.handle_framed_content_changed(containing_pipeline_id, subpage_id), ConstellationControlMsg::ReportCSSError(pipeline_id, filename, line, column, msg) => self.handle_css_error_reporting(pipeline_id, filename, line, column, msg), } @@ -1487,6 +1489,22 @@ impl ScriptThread { } } + fn handle_framed_content_changed(&self, + parent_pipeline_id: PipelineId, + subpage_id: SubpageId) { + let borrowed_page = self.root_page(); + let page = borrowed_page.find(parent_pipeline_id).unwrap(); + let doc = page.document(); + let frame_element = doc.find_iframe(subpage_id); + if let Some(ref frame_element) = frame_element { + frame_element.upcast::().dirty(NodeDamage::OtherNodeDamage); + let window = page.window(); + window.reflow(ReflowGoal::ForDisplay, + ReflowQueryType::NoQuery, + ReflowReason::FramedContentChanged); + } + } + /// Handles a mozbrowser event, for example see: /// https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowserloadstart fn handle_mozbrowser_event_msg(&self, diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index a4d07966028..9229c626ab6 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -143,6 +143,8 @@ pub enum ConstellationControlMsg { /// The pipeline that contains a frame loading the target pipeline. parent: PipelineId }, + /// Notifies a parent frame that one of its child frames is now active. + FramedContentChanged(PipelineId, SubpageId), /// Report an error from a CSS parser for the given pipeline ReportCSSError(PipelineId, String, u32, u32, String), } diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 44620f9b226..93bf5f886ae 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -1688,6 +1688,18 @@ "url": "/_mozilla/css/iframe/multiple_external.html" } ], + "css/iframe/navigation.html": [ + { + "path": "css/iframe/navigation.html", + "references": [ + [ + "/_mozilla/css/iframe/navigation_ref.html", + "==" + ] + ], + "url": "/_mozilla/css/iframe/navigation.html" + } + ], "css/iframe/overflow.html": [ { "path": "css/iframe/overflow.html", @@ -7756,6 +7768,18 @@ "url": "/_mozilla/css/iframe/multiple_external.html" } ], + "css/iframe/navigation.html": [ + { + "path": "css/iframe/navigation.html", + "references": [ + [ + "/_mozilla/css/iframe/navigation_ref.html", + "==" + ] + ], + "url": "/_mozilla/css/iframe/navigation.html" + } + ], "css/iframe/overflow.html": [ { "path": "css/iframe/overflow.html", diff --git a/tests/wpt/mozilla/tests/css/iframe/navigation.html b/tests/wpt/mozilla/tests/css/iframe/navigation.html new file mode 100644 index 00000000000..bbbceaea51d --- /dev/null +++ b/tests/wpt/mozilla/tests/css/iframe/navigation.html @@ -0,0 +1,28 @@ + + + + + + + + + + diff --git a/tests/wpt/mozilla/tests/css/iframe/navigation_ref.html b/tests/wpt/mozilla/tests/css/iframe/navigation_ref.html new file mode 100644 index 00000000000..0207d0908e1 --- /dev/null +++ b/tests/wpt/mozilla/tests/css/iframe/navigation_ref.html @@ -0,0 +1,17 @@ + + + + + + + + From 7eca462c5a5fa613e54a09df6c5c5215b4b9762e Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 25 Jan 2016 15:01:53 -0500 Subject: [PATCH 2/3] Make iframe's load event trigger a reflow of the enclosing window. Add a catch-all reflow for all same-origin pages sharing an event loop.a --- components/script/dom/htmliframeelement.rs | 8 +++++++- components/script/dom/window.rs | 4 ++++ components/script/script_thread.rs | 8 ++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs index 65cfd32fa17..fd8788480a6 100644 --- a/components/script/dom/htmliframeelement.rs +++ b/components/script/dom/htmliframeelement.rs @@ -23,9 +23,10 @@ use dom::htmlelement::HTMLElement; use dom::node::{Node, UnbindContext, window_from_node}; use dom::urlhelper::UrlHelper; use dom::virtualmethods::VirtualMethods; -use dom::window::Window; +use dom::window::{ReflowReason, Window}; use js::jsapi::{JSAutoCompartment, JSAutoRequest, RootedValue, JSContext, MutableHandleValue}; use js::jsval::{UndefinedValue, NullValue}; +use layout_interface::ReflowQueryType; use msg::constellation_msg::{ConstellationChan}; use msg::constellation_msg::{NavigationDirection, PipelineId, SubpageId}; use page::IterablePage; @@ -34,6 +35,7 @@ use script_traits::{IFrameLoadInfo, MozBrowserEvent, ScriptMsg as ConstellationM use std::ascii::AsciiExt; use std::cell::Cell; use string_cache::Atom; +use style::context::ReflowGoal; use url::Url; use util::prefs; use util::str::{DOMString, LengthOrPercentageOrAuto}; @@ -211,6 +213,10 @@ impl HTMLIFrameElement { let window = window_from_node(self); self.upcast::().fire_simple_event("load", GlobalRef::Window(window.r())); // TODO Step 5 - unset child document `mut iframe load` flag + + window.reflow(ReflowGoal::ForDisplay, + ReflowQueryType::NoQuery, + ReflowReason::IFrameLoadEvent); } } diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 4c26c15e6f5..2ca2090e547 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -108,6 +108,8 @@ pub enum ReflowReason { RequestAnimationFrame, WebFontLoaded, FramedContentChanged, + IFrameLoadEvent, + MissingExplicitReflow, } pub type ScrollPoint = Point2D; @@ -1427,6 +1429,8 @@ fn debug_reflow_events(goal: &ReflowGoal, query_type: &ReflowQueryType, reason: ReflowReason::RequestAnimationFrame => "\tRequestAnimationFrame", ReflowReason::WebFontLoaded => "\tWebFontLoaded", ReflowReason::FramedContentChanged => "\tFramedContentChanged", + ReflowReason::IFrameLoadEvent => "\tIFrameLoadEvent", + ReflowReason::MissingExplicitReflow => "\tMissingExplicitReflow", }); println!("{}", debug_msg); diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index ef55e608be8..4cae85da9ba 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1028,6 +1028,14 @@ impl ScriptThread { window.reflow(ReflowGoal::ForDisplay, ReflowQueryType::NoQuery, ReflowReason::ImageLoaded); + } else { + // Reflow currently happens when explicitly invoked by code that + // knows the document could have been modified. This should really + // be driven by the compositor on an as-needed basis instead, to + // minimize unnecessary work. + window.reflow(ReflowGoal::ForDisplay, + ReflowQueryType::NoQuery, + ReflowReason::MissingExplicitReflow); } } } From c79231f9c619f33f7fea66c08bec7a7c0695dc21 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 27 Jan 2016 18:17:03 -0500 Subject: [PATCH 3/3] Ignore navigation requests from pages that are not active. --- components/compositing/constellation.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 829b9ead234..4bf942302f7 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -946,6 +946,14 @@ impl Constellation } } + if !self.pipeline_is_in_current_frame(source_id) { + // Disregard this load if the navigating pipeline is not actually + // active. This could be caused by a delayed navigation (eg. from + // a timer) or a race between multiple navigations (such as an + // onclick handler on an anchor element). + return None; + } + self.handle_load_start_msg(&source_id); // Being here means either there are no pending frames, or none of the pending // changes would be overridden by changing the subframe associated with source_id.