From 966888615fa8f4bf6ff4edb8496b0545f33912e3 Mon Sep 17 00:00:00 2001 From: Gae24 <96017547+Gae24@users.noreply.github.com> Date: Sun, 16 Feb 2025 19:53:35 +0100 Subject: [PATCH] `DataTransferItem`: improve spec compliance (#35418) * DataTransfer: remove PlainString and Binary structs Signed-off-by: Gae24 <96017547+Gae24@users.noreply.github.com> * add DataTransferItemList remove() test case Signed-off-by: Gae24 <96017547+Gae24@users.noreply.github.com> * bring datatransferitem closer to the spec Signed-off-by: Gae24 <96017547+Gae24@users.noreply.github.com> * queue a task to invoke the callback Signed-off-by: Gae24 <96017547+Gae24@users.noreply.github.com> --------- Signed-off-by: Gae24 <96017547+Gae24@users.noreply.github.com> --- components/script/dom/datatransferitem.rs | 102 +++++++++++--- components/script/dom/datatransferitemlist.rs | 59 ++++----- components/script/dom/document.rs | 16 +-- components/script/drag_data_store.rs | 124 ++++++++---------- components/script/textinput.rs | 4 +- tests/wpt/meta/MANIFEST.json | 2 +- .../datatransferitemlist-remove.html | 16 +++ 7 files changed, 198 insertions(+), 125 deletions(-) diff --git a/components/script/dom/datatransferitem.rs b/components/script/dom/datatransferitem.rs index 630facfe228..d4888d2f91c 100644 --- a/components/script/dom/datatransferitem.rs +++ b/components/script/dom/datatransferitem.rs @@ -2,74 +2,146 @@ * 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, Ref, RefCell}; use std::rc::Rc; use dom_struct::dom_struct; use crate::dom::bindings::callback::ExceptionHandling; +use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::DataTransferItemBinding::{ DataTransferItemMethods, FunctionStringCallback, }; +use crate::dom::bindings::refcounted::Trusted; use crate::dom::bindings::reflector::{reflect_dom_object, DomGlobal, Reflector}; use crate::dom::bindings::root::DomRoot; use crate::dom::bindings::str::DOMString; use crate::dom::file::File; use crate::dom::globalscope::GlobalScope; -use crate::drag_data_store::Kind; +use crate::drag_data_store::{DragDataStore, Kind, Mode}; use crate::script_runtime::CanGc; #[dom_struct] pub(crate) struct DataTransferItem { reflector_: Reflector, - #[ignore_malloc_size_of = "TODO"] + #[ignore_malloc_size_of = "Rc"] #[no_trace] - item: Kind, + data_store: Rc>>, + id: u16, + pending_callbacks: DomRefCell>, + next_callback: Cell, +} + +#[derive(JSTraceable, MallocSizeOf)] +struct PendingStringCallback { + id: usize, + #[ignore_malloc_size_of = "Rc"] + callback: Rc, } impl DataTransferItem { - fn new_inherited(item: Kind) -> DataTransferItem { + fn new_inherited(data_store: Rc>>, id: u16) -> DataTransferItem { DataTransferItem { reflector_: Reflector::new(), - item, + data_store, + id, + pending_callbacks: Default::default(), + next_callback: Cell::new(0), } } pub(crate) fn new( global: &GlobalScope, + data_store: Rc>>, + id: u16, can_gc: CanGc, - item: Kind, ) -> DomRoot { reflect_dom_object( - Box::new(DataTransferItem::new_inherited(item)), + Box::new(DataTransferItem::new_inherited(data_store, id)), global, can_gc, ) } + + fn item_kind(&self) -> Option> { + Ref::filter_map(self.data_store.borrow(), |data_store| { + data_store + .as_ref() + .and_then(|data_store| data_store.get_by_id(&self.id)) + }) + .ok() + } + + fn can_read(&self) -> bool { + self.data_store + .borrow() + .as_ref() + .is_some_and(|data_store| data_store.mode() != Mode::Protected) + } } impl DataTransferItemMethods for DataTransferItem { /// fn Kind(&self) -> DOMString { - match self.item { - Kind::Text(_) => DOMString::from("string"), - Kind::File(_) => DOMString::from("file"), - } + self.item_kind() + .map_or(DOMString::new(), |item| match *item { + Kind::Text { .. } => DOMString::from("string"), + Kind::File { .. } => DOMString::from("file"), + }) } /// fn Type(&self) -> DOMString { - self.item.type_() + self.item_kind() + .map_or(DOMString::new(), |item| item.type_()) } /// fn GetAsString(&self, callback: Option>) { - if let (Some(callback), Some(data)) = (callback, self.item.as_string()) { - let _ = callback.Call__(data, ExceptionHandling::Report); + // Step 1 If the callback is null, return. + let Some(callback) = callback else { + return; + }; + + // Step 2 If the DataTransferItem object is not in the read/write mode or the read-only mode, return. + if !self.can_read() { + return; + } + + // Step 3 If the drag data item kind is not text, then return. + if let Some(string) = self.item_kind().and_then(|item| item.as_string()) { + let id = self.next_callback.get(); + let pending_callback = PendingStringCallback { id, callback }; + self.pending_callbacks.borrow_mut().push(pending_callback); + + self.next_callback.set(id + 1); + let this = Trusted::new(self); + + // Step 4 Otherwise, queue a task to invoke callback, + // passing the actual data of the item represented by the DataTransferItem object as the argument. + self.global() + .task_manager() + .dom_manipulation_task_source() + .queue(task!(invoke_callback: move || { + if let Some(index) = this.root().pending_callbacks.borrow().iter().position(|val| val.id == id) { + let callback = this.root().pending_callbacks.borrow_mut().swap_remove(index).callback; + let _ = callback.Call__(DOMString::from(string), ExceptionHandling::Report); + } + })); } } /// fn GetAsFile(&self, can_gc: CanGc) -> Option> { - self.item.as_file(&self.global(), can_gc) + // Step 1 If the DataTransferItem object is not in the read/write mode or the read-only mode, then return null. + if !self.can_read() { + return None; + } + + // Step 2 If the drag data item kind is not File, then return null. + // Step 3 Return a new File object representing the actual data + // of the item represented by the DataTransferItem object. + self.item_kind() + .and_then(|item| item.as_file(&self.global(), can_gc)) } } diff --git a/components/script/dom/datatransferitemlist.rs b/components/script/dom/datatransferitemlist.rs index 0a7714419e3..3f2f029ab67 100644 --- a/components/script/dom/datatransferitemlist.rs +++ b/components/script/dom/datatransferitemlist.rs @@ -17,7 +17,7 @@ use crate::dom::bindings::str::DOMString; use crate::dom::datatransferitem::DataTransferItem; use crate::dom::file::File; use crate::dom::window::Window; -use crate::drag_data_store::{Binary, DragDataStore, Kind, Mode, PlainString}; +use crate::drag_data_store::{DragDataStore, Kind, Mode}; use crate::script_runtime::{CanGc, JSContext}; #[dom_struct] @@ -90,9 +90,9 @@ impl DataTransferItemListMethods for DataTransferItemList }; // Step 2 - data_store - .get_item(index as usize) - .map(|item| DataTransferItem::new(&self.global(), can_gc, item)) + data_store.get_by_index(index as usize).map(|(id, _)| { + DataTransferItem::new(&self.global(), Rc::clone(&self.data_store), *id, can_gc) + }) } /// @@ -113,18 +113,18 @@ impl DataTransferItemListMethods for DataTransferItemList // whose type string is equal to the value of the method's second argument, converted to ASCII lowercase, // and whose data is the string given by the method's first argument. type_.make_ascii_lowercase(); - data_store.add(Kind::Text(PlainString::new(data, type_)))?; + data_store.add(Kind::Text { data, type_ }).map(|id| { + self.frozen_types.clear(); - self.frozen_types.clear(); - - // Step 3 Determine the value of the indexed property corresponding to the newly added item, - // and return that value (a newly created DataTransferItem object). - let index = data_store.list_len() - 1; - let item = data_store - .get_item(index) - .map(|item| DataTransferItem::new(&self.global(), can_gc, item)); - - Ok(item) + // Step 3 Determine the value of the indexed property corresponding to the newly added item, + // and return that value (a newly created DataTransferItem object). + Some(DataTransferItem::new( + &self.global(), + Rc::clone(&self.data_store), + id, + can_gc, + )) + }) } /// @@ -141,24 +141,21 @@ impl DataTransferItemListMethods for DataTransferItemList // and whose data is the same as the File's data. let mut type_ = data.file_type(); type_.make_ascii_lowercase(); - let binary = Binary::new( - data.file_bytes().unwrap_or_default(), - data.name().clone(), - type_, - ); + let bytes = data.file_bytes().unwrap_or_default(); + let name = data.name().clone(); - data_store.add(Kind::File(binary))?; + data_store.add(Kind::File { bytes, name, type_ }).map(|id| { + self.frozen_types.clear(); - self.frozen_types.clear(); - - // Step 3 Determine the value of the indexed property corresponding to the newly added item, - // and return that value (a newly created DataTransferItem object). - let index = data_store.list_len() - 1; - let item = data_store - .get_item(index) - .map(|item| DataTransferItem::new(&self.global(), can_gc, item)); - - Ok(item) + // Step 3 Determine the value of the indexed property corresponding to the newly added item, + // and return that value (a newly created DataTransferItem object). + Some(DataTransferItem::new( + &self.global(), + Rc::clone(&self.data_store), + id, + can_gc, + )) + }) } /// diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index b48c29dd635..05da711d35e 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -194,7 +194,7 @@ use crate::dom::wheelevent::WheelEvent as DomWheelEvent; use crate::dom::window::Window; use crate::dom::windowproxy::WindowProxy; use crate::dom::xpathevaluator::XPathEvaluator; -use crate::drag_data_store::{DragDataStore, Kind, Mode, PlainString}; +use crate::drag_data_store::{DragDataStore, Kind, Mode}; use crate::fetch::FetchCanceller; use crate::iframe_collection::IFrameCollection; use crate::messaging::{CommonScriptMsg, MainThreadScriptMsg}; @@ -1688,11 +1688,9 @@ impl Document { // Step 7.1.2.1 For each clipboard-part on the OS clipboard: // Step 7.1.2.1.1 If clipboard-part contains plain text, then - let plain_string = PlainString::new( - DOMString::from_string(text_contents.to_string()), - DOMString::from("text/plain"), - ); - let _ = drag_data_store.add(Kind::Text(plain_string)); + let data = DOMString::from(text_contents.to_string()); + let type_ = DOMString::from("text/plain"); + let _ = drag_data_store.add(Kind::Text { data, type_ }); // Step 7.1.2.1.2 TODO If clipboard-part represents file references, then for each file reference // Step 7.1.2.1.3 TODO If clipboard-part contains HTML- or XHTML-formatted text then @@ -1744,16 +1742,16 @@ impl Document { // Step 1.2 for item in drag_data_store.iter_item_list() { match item { - Kind::Text(string) => { + Kind::Text { data, .. } => { // Step 1.2.1.1 Ensure encoding is correct per OS and locale conventions // Step 1.2.1.2 Normalize line endings according to platform conventions // Step 1.2.1.3 self.send_to_embedder(EmbedderMsg::SetClipboardText( self.webview_id(), - string.data(), + data.to_string(), )); }, - Kind::File(_) => { + Kind::File { .. } => { // Step 1.2.2 If data is of a type listed in the mandatory data types list, then // Step 1.2.2.1 Place part on clipboard with the appropriate OS clipboard format description // Step 1.2.3 Else this is left to the implementation diff --git a/components/script/drag_data_store.rs b/components/script/drag_data_store.rs index db1abda4056..f0289f55e45 100644 --- a/components/script/drag_data_store.rs +++ b/components/script/drag_data_store.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use indexmap::IndexMap; use pixels::Image; use script_traits::serializable::BlobImpl; @@ -15,53 +16,30 @@ use crate::dom::globalscope::GlobalScope; use crate::script_runtime::CanGc; /// -#[derive(Clone)] pub(crate) enum Kind { - Text(PlainString), - File(Binary), -} - -#[derive(Clone)] -pub(crate) struct PlainString { - data: DOMString, - type_: DOMString, -} - -impl PlainString { - pub(crate) fn new(data: DOMString, type_: DOMString) -> Self { - Self { data, type_ } - } - - pub fn data(&self) -> String { - self.data.to_string() - } -} - -#[derive(Clone)] -pub(crate) struct Binary { - bytes: Vec, - name: DOMString, - type_: String, -} - -impl Binary { - pub(crate) fn new(bytes: Vec, name: DOMString, type_: String) -> Self { - Self { bytes, name, type_ } - } + Text { + data: DOMString, + type_: DOMString, + }, + File { + bytes: Vec, + name: DOMString, + type_: String, + }, } impl Kind { pub(crate) fn type_(&self) -> DOMString { match self { - Kind::Text(string) => string.type_.clone(), - Kind::File(binary) => DOMString::from(binary.type_.clone()), + Kind::Text { type_, .. } => type_.clone(), + Kind::File { type_, .. } => DOMString::from(type_.clone()), } } - pub(crate) fn as_string(&self) -> Option { + pub(crate) fn as_string(&self) -> Option { match self { - Kind::Text(string) => Some(string.data.clone()), - Kind::File(_) => None, + Kind::Text { data, .. } => Some(data.to_string()), + Kind::File { .. } => None, } } @@ -69,23 +47,23 @@ impl Kind { // since File constructor requires moving it. pub(crate) fn as_file(&self, global: &GlobalScope, can_gc: CanGc) -> Option> { match self { - Kind::Text(_) => None, - Kind::File(binary) => Some(File::new( + Kind::Text { .. } => None, + Kind::File { bytes, name, type_ } => Some(File::new( global, - BlobImpl::new_from_bytes(binary.bytes.clone(), binary.type_.clone()), - binary.name.clone(), + BlobImpl::new_from_bytes(bytes.clone(), type_.clone()), + name.clone(), None, can_gc, )), } } - fn text_type_matches(&self, type_: &str) -> bool { - matches!(self, Kind::Text(string) if string.type_.eq(type_)) + fn text_type_matches(&self, text_type: &str) -> bool { + matches!(self, Kind::Text { type_, .. } if type_.eq(text_type)) } fn is_file(&self) -> bool { - matches!(self, Kind::File(_)) + matches!(self, Kind::File { .. }) } } @@ -111,7 +89,8 @@ pub(crate) enum Mode { #[allow(dead_code)] // TODO some fields are used by DragEvent. pub(crate) struct DragDataStore { /// - item_list: Vec, + item_list: IndexMap, + next_item_id: u16, /// default_feedback: Option, bitmap: Option, @@ -127,7 +106,8 @@ impl DragDataStore { #[allow(clippy::new_without_default)] pub(crate) fn new() -> DragDataStore { DragDataStore { - item_list: Vec::new(), + item_list: IndexMap::new(), + next_item_id: 0, default_feedback: None, bitmap: None, mode: Mode::Protected, @@ -154,12 +134,12 @@ impl DragDataStore { pub(crate) fn types(&self) -> Vec { let mut types = Vec::new(); - let has_files = self.item_list.iter().fold(false, |has_files, item| { + let has_files = self.item_list.values().fold(false, |has_files, item| { // Step 2.1 For each item in the item list whose kind is text, // add an entry to L consisting of the item's type string. match item { - Kind::Text(string) => types.push(string.type_.clone()), - Kind::File(_) => return true, + Kind::Text { type_, .. } => types.push(type_.clone()), + Kind::File { .. } => return true, } has_files @@ -175,28 +155,32 @@ impl DragDataStore { pub(crate) fn find_matching_text(&self, type_: &str) -> Option { self.item_list - .iter() + .values() .find(|item| item.text_type_matches(type_)) .and_then(|item| item.as_string()) + .map(DOMString::from) } - pub(crate) fn add(&mut self, kind: Kind) -> Fallible<()> { - if let Kind::Text(ref string) = kind { + pub(crate) fn add(&mut self, kind: Kind) -> Fallible { + if let Kind::Text { ref type_, .. } = kind { // Step 2.1 If there is already an item in the item list whose kind is text // and whose type string is equal to the method's second argument, throw "NotSupportedError". if self .item_list - .iter() - .any(|item| item.text_type_matches(&string.type_)) + .values() + .any(|item| item.text_type_matches(type_)) { return Err(Error::NotSupported); } } - // Step 2.2 - self.item_list.push(kind); + let item_id = self.next_item_id; - Ok(()) + // Step 2.2 + self.item_list.insert(item_id, kind); + + self.next_item_id += 1; + Ok(item_id) } pub(crate) fn set_data(&mut self, format: DOMString, data: DOMString) { @@ -206,10 +190,12 @@ impl DragDataStore { // Step 5 Remove the item in the drag data store item list whose kind is text // and whose type string is equal to format, if there is one. self.item_list - .retain(|item| !item.text_type_matches(&type_)); + .retain(|_, item| !item.text_type_matches(&type_)); // Step 6 Add an item whose kind is text, whose type is format, and whose data is the method's second argument. - self.item_list.push(Kind::Text(PlainString { data, type_ })); + self.item_list + .insert(self.next_item_id, Kind::Text { data, type_ }); + self.next_item_id += 1; } pub(crate) fn clear_data(&mut self, format: Option) -> bool { @@ -220,7 +206,7 @@ impl DragDataStore { let type_ = normalize_mime(format); // Step 6 Remove the item in the item list whose kind is text and whose type is format. - self.item_list.retain(|item| { + self.item_list.retain(|_, item| { let matches = item.text_type_matches(&type_); if matches { @@ -230,7 +216,7 @@ impl DragDataStore { }); } else { // Step 3 Remove each item in the item list whose kind is text. - self.item_list.retain(|item| { + self.item_list.retain(|_, item| { let matches = item.is_file(); if !matches { @@ -256,7 +242,7 @@ impl DragDataStore { // Step 4 For each item in the drag data store item list whose kind is File, add the item's data to the list L. self.item_list - .iter() + .values() .filter_map(|item| item.as_file(global, can_gc)) .for_each(|file| file_list.push(file)); } @@ -265,16 +251,20 @@ impl DragDataStore { self.item_list.len() } - pub(crate) fn iter_item_list(&self) -> std::slice::Iter<'_, Kind> { - self.item_list.iter() + pub(crate) fn iter_item_list(&self) -> indexmap::map::Values<'_, u16, Kind> { + self.item_list.values() } - pub(crate) fn get_item(&self, index: usize) -> Option { - self.item_list.get(index).cloned() + pub(crate) fn get_by_index(&self, index: usize) -> Option<(&u16, &Kind)> { + self.item_list.get_index(index) + } + + pub(crate) fn get_by_id(&self, id: &u16) -> Option<&Kind> { + self.item_list.get(id) } pub(crate) fn remove(&mut self, index: usize) { - self.item_list.remove(index); + self.item_list.shift_remove_index(index); } pub(crate) fn clear_list(&mut self) { diff --git a/components/script/textinput.rs b/components/script/textinput.rs index a2194b60751..4406984a34f 100644 --- a/components/script/textinput.rs +++ b/components/script/textinput.rs @@ -1140,8 +1140,8 @@ impl TextInput { fn paste_contents(&mut self, drag_data_store: &DragDataStore) { for item in drag_data_store.iter_item_list() { - if let Kind::Text(string) = item { - self.insert_string(string.data()); + if let Kind::Text { data, .. } = item { + self.insert_string(data.to_string()); } } } diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index ba197a1f837..62c374060a3 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -709557,7 +709557,7 @@ ] ], "datatransferitemlist-remove.html": [ - "19316181c5fc61f6561054fa5d628d9285de1468", + "89f8cfd320025540991f2532f47bab5c4700d1f7", [ null, {} diff --git a/tests/wpt/tests/html/editing/dnd/datastore/datatransferitemlist-remove.html b/tests/wpt/tests/html/editing/dnd/datastore/datatransferitemlist-remove.html index 19316181c5f..89f8cfd3200 100644 --- a/tests/wpt/tests/html/editing/dnd/datastore/datatransferitemlist-remove.html +++ b/tests/wpt/tests/html/editing/dnd/datastore/datatransferitemlist-remove.html @@ -20,4 +20,20 @@ test(() => { // Must not throw dt.items.remove(1); }, "remove()ing an out-of-bounds index does nothing"); + +test(() => { + const file = new File(["πŸ•ΊπŸ’ƒ"], "test.png", { + type: "image/png" + }); + + const dt = new DataTransfer(); + dt.items.add(file); + + let item = dt.items[0]; + dt.items.remove(0); + + assert_equals(item.kind, ""); + assert_equals(item.type, ""); + assert_equals(item.getAsFile(), null); +}, "remove()ing an item will put the associated DataTransferItem object in the disabled mode");