From e7ef9cfb303598e582f0f589da855bdd33c1dd08 Mon Sep 17 00:00:00 2001 From: Connor Brewster Date: Fri, 20 Apr 2018 22:44:59 -0500 Subject: [PATCH] Make session history aware of URLs --- components/atoms/static_atoms.txt | 1 + components/constellation/constellation.rs | 108 ++++++++++++------ components/constellation/session_history.rs | 31 ++++- components/script/dom/history.rs | 44 ++++++- components/script/script_thread.rs | 14 ++- components/script_traits/lib.rs | 6 +- components/script_traits/script_msg.rs | 4 +- tests/wpt/metadata/MANIFEST.json | 10 ++ ...ration-fragment-scrolling-samedoc.html.ini | 3 +- ...ll-restoration-navigation-samedoc.html.ini | 3 +- .../history_pushstate_url.html | 24 ++++ 11 files changed, 192 insertions(+), 56 deletions(-) create mode 100644 tests/wpt/web-platform-tests/html/browsers/history/the-history-interface/history_pushstate_url.html diff --git a/components/atoms/static_atoms.txt b/components/atoms/static_atoms.txt index 3554a2e7a3b0..ba159bafdfa1 100644 --- a/components/atoms/static_atoms.txt +++ b/components/atoms/static_atoms.txt @@ -26,6 +26,7 @@ file fullscreenchange fullscreenerror gattserverdisconnected +hashchange hidden image input diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 261ed6e265bd..16b3674db506 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -140,6 +140,7 @@ use session_history::{JointSessionHistory, NeedsToReload, SessionHistoryChange, use std::borrow::ToOwned; use std::collections::{HashMap, VecDeque}; use std::marker::PhantomData; +use std::mem::replace; use std::process; use std::rc::{Rc, Weak}; use std::sync::Arc; @@ -1074,13 +1075,13 @@ impl Constellation self.handle_traverse_history_msg(source_top_ctx_id, direction); } // Handle a push history state request. - FromScriptMsg::PushHistoryState(history_state_id) => { + FromScriptMsg::PushHistoryState(history_state_id, url) => { debug!("constellation got push history state message from script"); - self.handle_push_history_state_msg(source_pipeline_id, history_state_id); + self.handle_push_history_state_msg(source_pipeline_id, history_state_id, url); } - FromScriptMsg::ReplaceHistoryState(history_state_id) => { + FromScriptMsg::ReplaceHistoryState(history_state_id, url) => { debug!("constellation got replace history state message from script"); - self.handle_replace_history_state_msg(source_pipeline_id, history_state_id); + self.handle_replace_history_state_msg(source_pipeline_id, history_state_id, url); } // Handle a joint session history length request. FromScriptMsg::JointSessionHistoryLength(sender) => { @@ -1938,11 +1939,10 @@ impl Constellation direction: TraversalDirection) { let mut browsing_context_changes = HashMap::::new(); - let mut pipeline_changes = HashMap::>::new(); + let mut pipeline_changes = HashMap::, ServoUrl)>::new(); + let mut url_to_load = HashMap::::new(); { - let session_history = self.joint_session_histories - .entry(top_level_browsing_context_id).or_insert(JointSessionHistory::new()); - + let session_history = self.get_joint_session_history(top_level_browsing_context_id); match direction { TraversalDirection::Forward(forward) => { let future_length = session_history.future.len(); @@ -1955,12 +1955,18 @@ impl Constellation match diff { SessionHistoryDiff::BrowsingContextDiff { browsing_context_id, ref new_reloader, .. } => { browsing_context_changes.insert(browsing_context_id, new_reloader.clone()); - }, - SessionHistoryDiff::PipelineDiff { ref pipeline_reloader, new_history_state_id, .. } => { - // TODO(cbrewster): Handle the case where the pipeline needs to be reloaded. - // We should use the history state URL to change the URL that is reloaded. - if let NeedsToReload::No(pipeline_id) = *pipeline_reloader { - pipeline_changes.insert(pipeline_id, Some(new_history_state_id)); + } + SessionHistoryDiff::PipelineDiff { + ref pipeline_reloader, + new_history_state_id, + ref new_url, + .. + } => match *pipeline_reloader { + NeedsToReload::No(pipeline_id) => { + pipeline_changes.insert(pipeline_id, (Some(new_history_state_id), new_url.clone())); + } + NeedsToReload::Yes(pipeline_id, ..) => { + url_to_load.insert(pipeline_id, new_url.clone()); } }, } @@ -1979,11 +1985,17 @@ impl Constellation SessionHistoryDiff::BrowsingContextDiff { browsing_context_id, ref old_reloader, .. } => { browsing_context_changes.insert(browsing_context_id, old_reloader.clone()); }, - SessionHistoryDiff::PipelineDiff { ref pipeline_reloader, old_history_state_id, .. } => { - // TODO(cbrewster): Handle the case where the pipeline needs to be reloaded. - // We should use the history state URL to change the URL that is reloaded. - if let NeedsToReload::No(pipeline_id) = *pipeline_reloader { - pipeline_changes.insert(pipeline_id, old_history_state_id); + SessionHistoryDiff::PipelineDiff { + ref pipeline_reloader, + old_history_state_id, + ref old_url, + .. + } => match *pipeline_reloader { + NeedsToReload::No(pipeline_id) => { + pipeline_changes.insert(pipeline_id, (old_history_state_id, old_url.clone())); + } + NeedsToReload::Yes(pipeline_id, ..) => { + url_to_load.insert(pipeline_id, old_url.clone()); } }, } @@ -1993,12 +2005,17 @@ impl Constellation } } - for (browsing_context_id, pipeline_id) in browsing_context_changes.drain() { - self.update_browsing_context(browsing_context_id, pipeline_id); + for (browsing_context_id, mut pipeline_reloader) in browsing_context_changes.drain() { + if let NeedsToReload::Yes(pipeline_id, ref mut load_data) = pipeline_reloader { + if let Some(url) = url_to_load.get(&pipeline_id) { + load_data.url = url.clone(); + } + } + self.update_browsing_context(browsing_context_id, pipeline_reloader); } - for (pipeline_id, history_state_id) in pipeline_changes.drain() { - self.update_pipeline(pipeline_id, history_state_id); + for (pipeline_id, (history_state_id, url)) in pipeline_changes.drain() { + self.update_pipeline(pipeline_id, history_state_id, url); } self.notify_history_changed(top_level_browsing_context_id); @@ -2076,12 +2093,13 @@ impl Constellation } } - fn update_pipeline(&mut self, pipeline_id: PipelineId, history_state_id: Option) { + fn update_pipeline(&mut self, pipeline_id: PipelineId, history_state_id: Option, url: ServoUrl) { let result = match self.pipelines.get_mut(&pipeline_id) { None => return warn!("Pipeline {} history state updated after closure", pipeline_id), Some(pipeline) => { - let msg = ConstellationControlMsg::UpdateHistoryStateId(pipeline_id, history_state_id); + let msg = ConstellationControlMsg::UpdateHistoryState(pipeline_id, history_state_id, url.clone()); pipeline.history_state_id = history_state_id; + pipeline.url = url; pipeline.event_loop.send(msg) }, }; @@ -2100,13 +2118,19 @@ impl Constellation let _ = sender.send(length as u32); } - fn handle_push_history_state_msg(&mut self, pipeline_id: PipelineId, history_state_id: HistoryStateId) { - let (top_level_browsing_context_id, old_state_id) = match self.pipelines.get_mut(&pipeline_id) { + fn handle_push_history_state_msg( + &mut self, + pipeline_id: PipelineId, + history_state_id: HistoryStateId, + url: ServoUrl) + { + let (top_level_browsing_context_id, old_state_id, old_url) = match self.pipelines.get_mut(&pipeline_id) { Some(pipeline) => { let old_history_state_id = pipeline.history_state_id; + let old_url = replace(&mut pipeline.url, url.clone()); pipeline.history_state_id = Some(history_state_id); pipeline.history_states.insert(history_state_id); - (pipeline.top_level_browsing_context_id, old_history_state_id) + (pipeline.top_level_browsing_context_id, old_history_state_id, old_url) } None => return warn!("Push history state {} for closed pipeline {}", history_state_id, pipeline_id), }; @@ -2115,18 +2139,30 @@ impl Constellation let diff = SessionHistoryDiff::PipelineDiff { pipeline_reloader: NeedsToReload::No(pipeline_id), new_history_state_id: history_state_id, + new_url: url, old_history_state_id: old_state_id, + old_url: old_url, }; session_history.push_diff(diff); } - fn handle_replace_history_state_msg(&mut self, pipeline_id: PipelineId, history_state_id: HistoryStateId) { - match self.pipelines.get_mut(&pipeline_id) { + fn handle_replace_history_state_msg( + &mut self, + pipeline_id: PipelineId, + history_state_id: HistoryStateId, + url: ServoUrl) + { + let top_level_browsing_context_id = match self.pipelines.get_mut(&pipeline_id) { Some(pipeline) => { pipeline.history_state_id = Some(history_state_id); + pipeline.url = url.clone(); + pipeline.top_level_browsing_context_id } None => return warn!("Replace history state {} for closed pipeline {}", history_state_id, pipeline_id), - } + }; + + let session_history = self.get_joint_session_history(top_level_browsing_context_id); + session_history.replace_history_state(pipeline_id, history_state_id, url); } fn handle_key_msg(&mut self, ch: Option, key: Key, state: KeyState, mods: KeyModifiers) { @@ -2493,7 +2529,7 @@ impl Constellation let (pipelines_to_close, states_to_close) = if let Some(replace_reloader) = change.replace { let session_history = self.joint_session_histories .entry(change.top_level_browsing_context_id).or_insert(JointSessionHistory::new()); - session_history.replace(replace_reloader.clone(), + session_history.replace_reloader(replace_reloader.clone(), NeedsToReload::No(change.new_pipeline_id)); match replace_reloader { @@ -2595,7 +2631,11 @@ impl Constellation let mut dead_pipelines = vec![]; for evicted_id in pipelines_to_evict { let load_data = match self.pipelines.get(&evicted_id) { - Some(pipeline) => pipeline.load_data.clone(), + Some(pipeline) => { + let mut load_data = pipeline.load_data.clone(); + load_data.url = pipeline.url.clone(); + load_data + }, None => continue, }; @@ -2606,7 +2646,7 @@ impl Constellation let session_history = self.get_joint_session_history(top_level_browsing_context_id); for (alive_id, dead) in dead_pipelines { - session_history.replace(NeedsToReload::No(alive_id), dead); + session_history.replace_reloader(NeedsToReload::No(alive_id), dead); } } diff --git a/components/constellation/session_history.rs b/components/constellation/session_history.rs index 44112ffe6c69..b4de8d0f6d48 100644 --- a/components/constellation/session_history.rs +++ b/components/constellation/session_history.rs @@ -4,6 +4,7 @@ use msg::constellation_msg::{BrowsingContextId, HistoryStateId, PipelineId, TopLevelBrowsingContextId}; use script_traits::LoadData; +use servo_url::ServoUrl; use std::{fmt, mem}; use std::cmp::PartialEq; @@ -37,9 +38,29 @@ impl JointSessionHistory { mem::replace(&mut self.future, vec![]) } - pub fn replace(&mut self, old_reloader: NeedsToReload, new_reloader: NeedsToReload) { + pub fn replace_reloader(&mut self, old_reloader: NeedsToReload, new_reloader: NeedsToReload) { for diff in self.past.iter_mut().chain(self.future.iter_mut()) { - diff.replace(&old_reloader, &new_reloader); + diff.replace_reloader(&old_reloader, &new_reloader); + } + } + + pub fn replace_history_state(&mut self, pipeline_id: PipelineId, history_state_id: HistoryStateId, url: ServoUrl) { + if let Some(SessionHistoryDiff::PipelineDiff { ref mut new_history_state_id, ref mut new_url, .. }) = + self.past.iter_mut().find(|diff| match diff { + SessionHistoryDiff::PipelineDiff { pipeline_reloader: NeedsToReload::No(id), .. } => pipeline_id == *id, + _ => false, + }) { + *new_history_state_id = history_state_id; + *new_url = url.clone(); + } + + if let Some(SessionHistoryDiff::PipelineDiff { ref mut old_history_state_id, ref mut old_url, .. }) = + self.future.iter_mut().find(|diff| match diff { + SessionHistoryDiff::PipelineDiff { pipeline_reloader: NeedsToReload::No(id), .. } => pipeline_id == *id, + _ => false, + }) { + *old_history_state_id = Some(history_state_id); + *old_url = url; } } @@ -146,8 +167,12 @@ pub enum SessionHistoryDiff { pipeline_reloader: NeedsToReload, /// The old history state id. old_history_state_id: Option, + /// The old url + old_url: ServoUrl, /// The new history state id. new_history_state_id: HistoryStateId, + /// The new url + new_url: ServoUrl, }, } @@ -179,7 +204,7 @@ impl SessionHistoryDiff { } /// Replaces all occurances of the replaced pipeline with a new pipeline - pub fn replace(&mut self, replaced_reloader: &NeedsToReload, reloader: &NeedsToReload) { + pub fn replace_reloader(&mut self, replaced_reloader: &NeedsToReload, reloader: &NeedsToReload) { match *self { SessionHistoryDiff::BrowsingContextDiff { ref mut old_reloader, ref mut new_reloader, .. } => { if *old_reloader == *replaced_reloader { diff --git a/components/script/dom/history.rs b/components/script/dom/history.rs index 46288fffb079..51f9a42c06c3 100644 --- a/components/script/dom/history.rs +++ b/components/script/dom/history.rs @@ -12,8 +12,10 @@ use dom::bindings::reflector::{DomObject, Reflector, reflect_dom_object}; use dom::bindings::root::{Dom, DomRoot}; use dom::bindings::str::{DOMString, USVString}; use dom::bindings::structuredclone::StructuredCloneData; +use dom::event::Event; use dom::eventtarget::EventTarget; use dom::globalscope::GlobalScope; +use dom::hashchangeevent::HashChangeEvent; use dom::popstateevent::PopStateEvent; use dom::window::Window; use dom_struct::dom_struct; @@ -71,8 +73,22 @@ impl History { Ok(()) } + // https://html.spec.whatwg.org/multipage/#history-traversal + // Steps 5-16 #[allow(unsafe_code)] - pub fn activate_state(&self, state_id: Option) { + pub fn activate_state(&self, state_id: Option, url: ServoUrl) { + // Steps 5 + let document = self.window.Document(); + let old_url = document.url().clone(); + document.set_url(url.clone()); + + // Step 6 + let hash_changed = old_url.fragment() != url.fragment(); + + // TODO: Step 8 - scroll restoration + + // Step 11 + let state_changed = state_id != self.state_id.get(); self.state_id.set(state_id); let serialized_data = match state_id { Some(state_id) => { @@ -98,8 +114,26 @@ impl History { } } - unsafe { - PopStateEvent::dispatch_jsval(self.window.upcast::(), &*self.window, self.state.handle()); + // TODO: Queue events on DOM Manipulation task source if non-blocking flag is set. + // Step 16.1 + if state_changed { + PopStateEvent::dispatch_jsval( + self.window.upcast::(), + &*self.window, + unsafe { self.state.handle() } + ); + } + + // Step 16.3 + if hash_changed { + let event = HashChangeEvent::new( + &self.window, + atom!("hashchange"), + true, + false, + old_url.into_string(), + url.into_string()); + event.upcast::().fire(self.window.upcast::()); } } @@ -175,7 +209,7 @@ impl History { PushOrReplace::Push => { let state_id = HistoryStateId::new(); self.state_id.set(Some(state_id)); - let msg = ScriptMsg::PushHistoryState(state_id); + let msg = ScriptMsg::PushHistoryState(state_id, new_url.clone()); let _ = self.window.upcast::().script_to_constellation_chan().send(msg); state_id }, @@ -188,7 +222,7 @@ impl History { state_id }, }; - let msg = ScriptMsg::ReplaceHistoryState(state_id); + let msg = ScriptMsg::ReplaceHistoryState(state_id, new_url.clone()); let _ = self.window.upcast::().script_to_constellation_chan().send(msg); state_id }, diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 4e89ba4ceff4..6ee0ec46d33a 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1169,7 +1169,7 @@ impl ScriptThread { Navigate(id, ..) => Some(id), PostMessage(id, ..) => Some(id), UpdatePipelineId(_, _, id, _) => Some(id), - UpdateHistoryStateId(id, ..) => Some(id), + UpdateHistoryState(id, ..) => Some(id), RemoveHistoryStates(id, ..) => Some(id), FocusIFrame(id, ..) => Some(id), WebDriverScriptCommand(id, ..) => Some(id), @@ -1297,8 +1297,8 @@ impl ScriptThread { browsing_context_id, new_pipeline_id, reason), - ConstellationControlMsg::UpdateHistoryStateId(pipeline_id, history_state_id) => - self.handle_update_history_state_id_msg(pipeline_id, history_state_id), + ConstellationControlMsg::UpdateHistoryState(pipeline_id, history_state_id, url) => + self.handle_update_history_state_msg(pipeline_id, history_state_id, url), ConstellationControlMsg::RemoveHistoryStates(pipeline_id, history_states) => self.handle_remove_history_states(pipeline_id, history_states), ConstellationControlMsg::FocusIFrame(parent_pipeline_id, frame_id) => @@ -1679,10 +1679,14 @@ impl ScriptThread { } } - fn handle_update_history_state_id_msg(&self, pipeline_id: PipelineId, history_state_id: Option) { + fn handle_update_history_state_msg( + &self, pipeline_id: PipelineId, + history_state_id: Option, + url: ServoUrl, + ) { match { self.documents.borrow().find_window(pipeline_id) } { None => return warn!("update history state after pipeline {} closed.", pipeline_id), - Some(window) => window.History().r().activate_state(history_state_id), + Some(window) => window.History().r().activate_state(history_state_id, url), } } diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index 272cfe3cedbe..98cf57116a78 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -289,8 +289,8 @@ pub enum ConstellationControlMsg { /// Updates the current pipeline ID of a given iframe. /// First PipelineId is for the parent, second is the new PipelineId for the frame. UpdatePipelineId(PipelineId, BrowsingContextId, PipelineId, UpdatePipelineIdReason), - /// Updates the history state of a given pipeline. - UpdateHistoryStateId(PipelineId, Option), + /// Updates the history state and url of a given pipeline. + UpdateHistoryState(PipelineId, Option, ServoUrl), /// Removes inaccesible history states. RemoveHistoryStates(PipelineId, Vec), /// Set an iframe to be focused. Used when an element in an iframe gains focus. @@ -347,7 +347,7 @@ impl fmt::Debug for ConstellationControlMsg { Navigate(..) => "Navigate", PostMessage(..) => "PostMessage", UpdatePipelineId(..) => "UpdatePipelineId", - UpdateHistoryStateId(..) => "UpdateHistoryStateId", + UpdateHistoryState(..) => "UpdateHistoryState", RemoveHistoryStates(..) => "RemoveHistoryStates", FocusIFrame(..) => "FocusIFrame", WebDriverScriptCommand(..) => "WebDriverScriptCommand", diff --git a/components/script_traits/script_msg.rs b/components/script_traits/script_msg.rs index 43812ad097a2..ec29bb74586c 100644 --- a/components/script_traits/script_msg.rs +++ b/components/script_traits/script_msg.rs @@ -105,9 +105,9 @@ pub enum ScriptMsg { /// HTMLIFrameElement Forward or Back traversal. TraverseHistory(TraversalDirection), /// Inform the constellation of a pushed history state. - PushHistoryState(HistoryStateId), + PushHistoryState(HistoryStateId, ServoUrl), /// Inform the constellation of a replaced history state. - ReplaceHistoryState(HistoryStateId), + ReplaceHistoryState(HistoryStateId, ServoUrl), /// Gets the length of the joint session history from the constellation. JointSessionHistoryLength(IpcSender), /// Favicon detected diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json index 27f49354c7e5..3e1aea384284 100644 --- a/tests/wpt/metadata/MANIFEST.json +++ b/tests/wpt/metadata/MANIFEST.json @@ -327481,6 +327481,12 @@ {} ] ], + "html/browsers/history/the-history-interface/history_pushstate_url.html": [ + [ + "/html/browsers/history/the-history-interface/history_pushstate_url.html", + {} + ] + ], "html/browsers/history/the-history-interface/history_replacestate.html": [ [ "/html/browsers/history/the-history-interface/history_replacestate.html", @@ -554878,6 +554884,10 @@ "8add09b76d8662f4343188a1ac36fe71fd4a225d", "testharness" ], + "html/browsers/history/the-history-interface/history_pushstate_url.html": [ + "d5e1649c88102f27da5fe1ac16715cfee7f70f84", + "testharness" + ], "html/browsers/history/the-history-interface/history_replacestate.html": [ "91cf4970bfa565ee8b846582bdbe608d38902fe7", "testharness" diff --git a/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html.ini b/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html.ini index 05ee8e956258..4e0e6ce3f8a6 100644 --- a/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html.ini +++ b/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-samedoc.html.ini @@ -1,7 +1,6 @@ [scroll-restoration-fragment-scrolling-samedoc.html] type: testharness - expected: TIMEOUT bug: https://github.com/servo/servo/issues/14970 [Manual scroll restoration should take precedent over scrolling to fragment in cross doc navigation] - expected: TIMEOUT + expected: FAIL diff --git a/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-navigation-samedoc.html.ini b/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-navigation-samedoc.html.ini index f12bd68a7872..239cddf72cc8 100644 --- a/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-navigation-samedoc.html.ini +++ b/tests/wpt/metadata/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-navigation-samedoc.html.ini @@ -1,7 +1,6 @@ [scroll-restoration-navigation-samedoc.html] type: testharness - expected: TIMEOUT bug: https://github.com/servo/servo/issues/14970 [history.{push,replace}State retain scroll restoration mode and navigation in the same document respects it] - expected: TIMEOUT + expected: FAIL diff --git a/tests/wpt/web-platform-tests/html/browsers/history/the-history-interface/history_pushstate_url.html b/tests/wpt/web-platform-tests/html/browsers/history/the-history-interface/history_pushstate_url.html new file mode 100644 index 000000000000..cfbca35bfdfb --- /dev/null +++ b/tests/wpt/web-platform-tests/html/browsers/history/the-history-interface/history_pushstate_url.html @@ -0,0 +1,24 @@ + + + + +History pushState sets the url + + + + +
+ + +