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

@@ -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<RefCell<Option<DragDataStore>>>,
+    id: u16,
+    pending_callbacks: DomRefCell<Vec<PendingStringCallback>>,
+    next_callback: Cell<usize>,
+}
+
+#[derive(JSTraceable, MallocSizeOf)]
+struct PendingStringCallback {
+    id: usize,
+    #[ignore_malloc_size_of = "Rc"]
+    callback: Rc<FunctionStringCallback>,
 }
 
 impl DataTransferItem {
-    fn new_inherited(item: Kind) -> DataTransferItem {
+    fn new_inherited(data_store: Rc<RefCell<Option<DragDataStore>>>, 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<RefCell<Option<DragDataStore>>>,
+        id: u16,
         can_gc: CanGc,
-        item: Kind,
     ) -> DomRoot<DataTransferItem> {
         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<Kind>> {
+        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<crate::DomTypeHolder> for DataTransferItem {
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitem-kind>
     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"),
+            })
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitem-type>
     fn Type(&self) -> DOMString {
-        self.item.type_()
+        self.item_kind()
+            .map_or(DOMString::new(), |item| item.type_())
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitem-getasstring>
     fn GetAsString(&self, callback: Option<Rc<FunctionStringCallback>>) {
-        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);
+                    }
+                }));
         }
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitem-getasfile>
     fn GetAsFile(&self, can_gc: CanGc) -> Option<DomRoot<File>> {
-        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))
     }
 }

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<crate::DomTypeHolder> 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)
+        })
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitemlist-add>
@@ -113,18 +113,18 @@ impl DataTransferItemListMethods<crate::DomTypeHolder> 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,
+            ))
+        })
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitemlist-add>
@@ -141,24 +141,21 @@ impl DataTransferItemListMethods<crate::DomTypeHolder> 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,
+            ))
+        })
     }
 
     /// <https://html.spec.whatwg.org/multipage/#dom-datatransferitemlist-remove>

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

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;
 
 /// <https://html.spec.whatwg.org/multipage/#the-drag-data-item-kind>
-#[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<u8>,
-    name: DOMString,
-    type_: String,
-}
-
-impl Binary {
-    pub(crate) fn new(bytes: Vec<u8>, name: DOMString, type_: String) -> Self {
-        Self { bytes, name, type_ }
-    }
+    Text {
+        data: DOMString,
+        type_: DOMString,
+    },
+    File {
+        bytes: Vec<u8>,
+        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<DOMString> {
+    pub(crate) fn as_string(&self) -> Option<String> {
         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<DomRoot<File>> {
         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 {
     /// <https://html.spec.whatwg.org/multipage/#drag-data-store-item-list>
-    item_list: Vec<Kind>,
+    item_list: IndexMap<u16, Kind>,
+    next_item_id: u16,
     /// <https://html.spec.whatwg.org/multipage/#drag-data-store-default-feedback>
     default_feedback: Option<String>,
     bitmap: Option<Bitmap>,
@@ -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<DOMString> {
         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<DOMString> {
         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<u16> {
+        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<DOMString>) -> 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<Kind> {
-        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) {

components/script/textinput.rs

@@ -1140,8 +1140,8 @@ impl<T: ClipboardProvider> TextInput<T> {
 
     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());
             }
         }
     }

tests/wpt/meta/MANIFEST.json

@@ -709557,7 +709557,7 @@
         ]
        ],
        "datatransferitemlist-remove.html": [
-        "19316181c5fc61f6561054fa5d628d9285de1468",
+        "89f8cfd320025540991f2532f47bab5c4700d1f7",
         [
          null,
          {}

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");
 </script>