From 48fa77df6710a89e156c8cebb7dec10c8cda4ae6 Mon Sep 17 00:00:00 2001 From: Daniel Adams <70986246+msub2@users.noreply.github.com> Date: Tue, 12 Mar 2024 08:32:30 -0400 Subject: [PATCH] Gamepad: Align closer to spec and implement missing slots (#31385) * Implement missing gamepad slots, align to spec more - Fixes TODO's from initial gamepad implementation - Adds some missing spec steps * Only handle gamepad events when pref is enabled * Return empty list in getGamepads if document not active * ./mach fmt * Update getGamepads to return an array instead of GamepadList * Add spec link for [[exposed]] slot * Remove failing test expectations for not-fully-active * A few fixes - Change should_notify to has_gesture - Add spec links and TODO to navigator - Remove unneeded clone from GamepadList::list - Move gamepadconnected event firing into has_gesture block * Use queue_with_canceller for tasks and add expects * Explicitly check for gamepad user gesture * Move user gesture check into separate function * Change contains_user_gesture to be a gamepad function * mach fmt * Change axis/button threshold constants to be private to module --- components/script/dom/gamepad.rs | 36 ++++++++- components/script/dom/gamepadlist.rs | 8 ++ components/script/dom/globalscope.rs | 73 ++++++++++++------- components/script/dom/navigator.rs | 42 +++++++++-- .../script/dom/webidls/Navigator.webidl | 2 +- ports/servoshell/app.rs | 2 +- .../gamepad/not-fully-active.html.ini | 3 - .../meta/gamepad/not-fully-active.html.ini | 3 - 8 files changed, 124 insertions(+), 45 deletions(-) delete mode 100644 tests/wpt/meta-legacy-layout/gamepad/not-fully-active.html.ini delete mode 100644 tests/wpt/meta/gamepad/not-fully-active.html.ini diff --git a/components/script/dom/gamepad.rs b/components/script/dom/gamepad.rs index 2cf327cd06e..68e7a1de2e5 100644 --- a/components/script/dom/gamepad.rs +++ b/components/script/dom/gamepad.rs @@ -6,6 +6,7 @@ use std::cell::Cell; use dom_struct::dom_struct; use js::typedarray::{Float64, Float64Array}; +use script_traits::GamepadUpdateType; use super::bindings::buffer_source::HeapBufferSource; use crate::dom::bindings::codegen::Bindings::GamepadBinding::{GamepadHand, GamepadMethods}; @@ -23,6 +24,9 @@ use crate::dom::gamepadpose::GamepadPose; use crate::dom::globalscope::GlobalScope; use crate::script_runtime::JSContext; +// This value is for determining when to consider a gamepad as having a user gesture +// from an axis tilt. This matches the threshold in Chromium. +const AXIS_TILT_THRESHOLD: f64 = 0.5; // This value is for determining when to consider a non-digital button "pressed". // Like Gecko and Chromium it derives from the XInput trigger threshold. const BUTTON_PRESS_THRESHOLD: f64 = 30.0 / 255.0; @@ -44,6 +48,7 @@ pub struct Gamepad { hand: GamepadHand, axis_bounds: (f64, f64), button_bounds: (f64, f64), + exposed: Cell, } impl Gamepad { @@ -74,6 +79,7 @@ impl Gamepad { hand: hand, axis_bounds: axis_bounds, button_bounds: button_bounds, + exposed: Cell::new(false), } } @@ -105,7 +111,7 @@ impl Gamepad { gamepad_id, id, 0, - false, + true, 0., String::from("standard"), &button_list, @@ -175,7 +181,7 @@ impl Gamepad { self.gamepad_id } - pub fn update_connected(&self, connected: bool) { + pub fn update_connected(&self, connected: bool, has_gesture: bool) { if self.connected.get() == connected { return; } @@ -187,7 +193,9 @@ impl Gamepad { GamepadEventType::Disconnected }; - self.notify_event(event_type); + if has_gesture { + self.notify_event(event_type); + } } pub fn update_index(&self, index: i32) { @@ -263,4 +271,26 @@ impl Gamepad { warn!("Button bounds difference is either 0 or non-finite!"); } } + + /// + pub fn exposed(&self) -> bool { + self.exposed.get() + } + + /// + pub fn set_exposed(&self, exposed: bool) { + self.exposed.set(exposed); + } +} + +/// +pub fn contains_user_gesture(update_type: GamepadUpdateType) -> bool { + match update_type { + GamepadUpdateType::Axis(_, value) => { + return value.abs() > AXIS_TILT_THRESHOLD; + }, + GamepadUpdateType::Button(_, value) => { + return value > BUTTON_PRESS_THRESHOLD; + }, + }; } diff --git a/components/script/dom/gamepadlist.rs b/components/script/dom/gamepadlist.rs index 4f373e24d96..5efa99f7aa8 100644 --- a/components/script/dom/gamepadlist.rs +++ b/components/script/dom/gamepadlist.rs @@ -48,6 +48,14 @@ impl GamepadList { pub fn remove_gamepad(&self, index: usize) { self.list.borrow_mut().remove(index); } + + pub fn list(&self) -> Vec>> { + self.list + .borrow() + .iter() + .map(|gamepad| Some(DomRoot::from_ref(&**gamepad))) + .collect() + } } impl GamepadListMethods for GamepadList { diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index cb8bb72a7fd..c9fd013318c 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -67,7 +67,6 @@ use crate::dom::bindings::codegen::Bindings::GamepadListBinding::GamepadList_Bin use crate::dom::bindings::codegen::Bindings::ImageBitmapBinding::{ ImageBitmapOptions, ImageBitmapSource, }; -use crate::dom::bindings::codegen::Bindings::NavigatorBinding::Navigator_Binding::NavigatorMethods; use crate::dom::bindings::codegen::Bindings::PerformanceBinding::Performance_Binding::PerformanceMethods; use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState; use crate::dom::bindings::codegen::Bindings::VoidFunctionBinding::VoidFunction; @@ -95,7 +94,8 @@ use crate::dom::event::{Event, EventBubbles, EventCancelable, EventStatus}; use crate::dom::eventsource::EventSource; use crate::dom::eventtarget::EventTarget; use crate::dom::file::File; -use crate::dom::gamepad::Gamepad; +use crate::dom::gamepad::{contains_user_gesture, Gamepad}; +use crate::dom::gamepadevent::GamepadEventType; use crate::dom::gpudevice::GPUDevice; use crate::dom::htmlscriptelement::{ScriptId, SourceCode}; use crate::dom::identityhub::Identities; @@ -3136,42 +3136,43 @@ impl GlobalScope { // TODO: 2. If document is not null and is not allowed to use the "gamepad" permission, // then abort these steps. let this = Trusted::new(&*self); - self.gamepad_task_source().queue( + self.gamepad_task_source().queue_with_canceller( task!(gamepad_connected: move || { let global = this.root(); let gamepad = Gamepad::new(&global, index as u32, name, axis_bounds, button_bounds); if let Some(window) = global.downcast::() { - let gamepad_list = window.Navigator().GetGamepads(); + let has_gesture = window.Navigator().has_gamepad_gesture(); + if has_gesture { + gamepad.set_exposed(true); + if window.Document().is_fully_active() { + gamepad.update_connected(true, has_gesture); + } + } + let gamepad_list = window.Navigator().gamepads(); let gamepad_arr: [DomRoot; 1] = [gamepad.clone()]; gamepad_list.add_if_not_exists(&gamepad_arr); - - // TODO: 3.4 If navigator.[[hasGamepadGesture]] is true: - // TODO: 3.4.1 Set gamepad.[[exposed]] to true. - - if window.Document().is_fully_active() { - gamepad.update_connected(true); - } } }), - &self, + &self.task_canceller(TaskSourceName::Gamepad) ) - .unwrap(); + .expect("Failed to queue gamepad connected task."); } /// pub fn handle_gamepad_disconnect(&self, index: usize) { let this = Trusted::new(&*self); self.gamepad_task_source() - .queue( + .queue_with_canceller( task!(gamepad_disconnected: move || { let global = this.root(); if let Some(window) = global.downcast::() { - let gamepad_list = window.Navigator().GetGamepads(); + let gamepad_list = window.Navigator().gamepads(); if let Some(gamepad) = gamepad_list.Item(index as u32) { - // TODO: If gamepad.[[exposed]] - gamepad.update_connected(false); - gamepad_list.remove_gamepad(index); + if window.Document().is_fully_active() { + gamepad.update_connected(false, gamepad.exposed()); + gamepad_list.remove_gamepad(index); + } } for i in (0..gamepad_list.Length()).rev() { if gamepad_list.Item(i as u32).is_none() { @@ -3182,9 +3183,9 @@ impl GlobalScope { } } }), - &self, + &self.task_canceller(TaskSourceName::Gamepad), ) - .unwrap(); + .expect("Failed to queue gamepad disconnected task."); } /// @@ -3193,12 +3194,12 @@ impl GlobalScope { // self.gamepad_task_source() - .queue( + .queue_with_canceller( task!(update_gamepad_state: move || { let global = this.root(); if let Some(window) = global.downcast::() { - let gamepad_list = window.Navigator().GetGamepads(); - if let Some(gamepad) = gamepad_list.IndexedGetter(index as u32) { + let gamepad_list = window.Navigator().gamepads(); + if let Some(gamepad) = gamepad_list.Item(index as u32) { let current_time = global.performance().Now(); gamepad.update_timestamp(*current_time); @@ -3211,14 +3212,32 @@ impl GlobalScope { } }; - // TODO: 6. If navigator.[[hasGamepadGesture]] is false - // and gamepad contains a gamepad user gesture: + if !window.Navigator().has_gamepad_gesture() && contains_user_gesture(update_type) { + window.Navigator().set_has_gamepad_gesture(true); + for i in 0..gamepad_list.Length() { + if let Some(gamepad) = gamepad_list.Item(i as u32) { + gamepad.set_exposed(true); + gamepad.update_timestamp(*current_time); + let new_gamepad = Trusted::new(&*gamepad); + if window.Document().is_fully_active() { + window.task_manager().gamepad_task_source().queue_with_canceller( + task!(update_gamepad_connect: move || { + let gamepad = new_gamepad.root(); + gamepad.notify_event(GamepadEventType::Connected); + }), + &window.upcast::().task_canceller(TaskSourceName::Gamepad), + ) + .expect("Failed to queue update gamepad connect task."); + } + } + } + } } } }), - &self, + &self.task_canceller(TaskSourceName::Gamepad), ) - .unwrap(); + .expect("Failed to queue update gamepad state task."); } pub(crate) fn current_group_label(&self) -> Option { diff --git a/components/script/dom/navigator.rs b/components/script/dom/navigator.rs index 612d2e68ff4..d16b4f263ae 100644 --- a/components/script/dom/navigator.rs +++ b/components/script/dom/navigator.rs @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use std::cell::Cell; use std::convert::TryInto; use dom_struct::dom_struct; @@ -9,11 +10,13 @@ use js::jsval::JSVal; use lazy_static::lazy_static; use crate::dom::bindings::codegen::Bindings::NavigatorBinding::NavigatorMethods; +use crate::dom::bindings::codegen::Bindings::WindowBinding::Window_Binding::WindowMethods; use crate::dom::bindings::reflector::{reflect_dom_object, DomObject, Reflector}; use crate::dom::bindings::root::{DomRoot, MutNullableDom}; use crate::dom::bindings::str::DOMString; use crate::dom::bindings::utils::to_frozen_array; use crate::dom::bluetooth::Bluetooth; +use crate::dom::gamepad::Gamepad; use crate::dom::gamepadlist::GamepadList; use crate::dom::gpu::GPU; use crate::dom::mediadevices::MediaDevices; @@ -43,10 +46,13 @@ pub struct Navigator { service_worker: MutNullableDom, xr: MutNullableDom, mediadevices: MutNullableDom, + /// gamepads: MutNullableDom, permissions: MutNullableDom, mediasession: MutNullableDom, gpu: MutNullableDom, + /// + has_gamepad_gesture: Cell, } impl Navigator { @@ -63,6 +69,7 @@ impl Navigator { permissions: Default::default(), mediasession: Default::default(), gpu: Default::default(), + has_gamepad_gesture: Cell::new(false), } } @@ -73,6 +80,21 @@ impl Navigator { pub fn xr(&self) -> Option> { self.xr.get() } + + pub fn gamepads(&self) -> DomRoot { + let gamepads = self + .gamepads + .or_init(|| GamepadList::new(&self.global(), &[])); + gamepads + } + + pub fn has_gamepad_gesture(&self) -> bool { + self.has_gamepad_gesture.get() + } + + pub fn set_has_gamepad_gesture(&self, has_gamepad_gesture: bool) { + self.has_gamepad_gesture.set(has_gamepad_gesture); + } } impl NavigatorMethods for Navigator { @@ -169,14 +191,20 @@ impl NavigatorMethods for Navigator { true } - // https://www.w3.org/TR/gamepad/#navigator-interface-extension - fn GetGamepads(&self) -> DomRoot { - let root = self - .gamepads - .or_init(|| GamepadList::new(&self.global(), &[])); + /// + fn GetGamepads(&self) -> Vec>> { + let global = self.global(); + let window = global.as_window(); + let doc = window.Document(); - // TODO: Add gamepads - root + // TODO: Handle permissions policy once implemented + if !doc.is_fully_active() || !self.has_gamepad_gesture.get() { + return Vec::new(); + } + + let root = self.gamepads.or_init(|| GamepadList::new(&global, &[])); + + root.list() } // https://w3c.github.io/permissions/#navigator-and-workernavigator-extension fn Permissions(&self) -> DomRoot { diff --git a/components/script/dom/webidls/Navigator.webidl b/components/script/dom/webidls/Navigator.webidl index aea9a16e725..b10173b408e 100644 --- a/components/script/dom/webidls/Navigator.webidl +++ b/components/script/dom/webidls/Navigator.webidl @@ -69,7 +69,7 @@ partial interface Navigator { // https://w3c.github.io/gamepad/#navigator-interface-extension partial interface Navigator { - [Pref="dom.gamepad.enabled"] GamepadList getGamepads(); + [Pref="dom.gamepad.enabled"] sequence getGamepads(); }; // https://html.spec.whatwg.org/multipage/#navigatorconcurrenthardware diff --git a/ports/servoshell/app.rs b/ports/servoshell/app.rs index 1a7c1587657..33ee1023f9e 100644 --- a/ports/servoshell/app.rs +++ b/ports/servoshell/app.rs @@ -436,7 +436,7 @@ impl App { // Catch some keyboard events, and push the rest onto the WebViewManager event queue. webviews.handle_window_events(embedder_events); - if webviews.webview_id().is_some() { + if pref!(dom.gamepad.enabled) && webviews.webview_id().is_some() { webviews.handle_gamepad_events(); } diff --git a/tests/wpt/meta-legacy-layout/gamepad/not-fully-active.html.ini b/tests/wpt/meta-legacy-layout/gamepad/not-fully-active.html.ini deleted file mode 100644 index 1b5a91a41e7..00000000000 --- a/tests/wpt/meta-legacy-layout/gamepad/not-fully-active.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[not-fully-active.html] - [calling getGamepads() in a non-fully-active document] - expected: FAIL diff --git a/tests/wpt/meta/gamepad/not-fully-active.html.ini b/tests/wpt/meta/gamepad/not-fully-active.html.ini deleted file mode 100644 index 1b5a91a41e7..00000000000 --- a/tests/wpt/meta/gamepad/not-fully-active.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[not-fully-active.html] - [calling getGamepads() in a non-fully-active document] - expected: FAIL