Auto merge of #26902 - jdm:focus-fixes, r=nox

Focus correctness improvements

These changes improve the behaviour of focus in Hubs rooms, and are expected to improve web compat around other dynamic pages that receive keyboard events as well.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26901 and fix #26900.
- [x] There are tests for these changes

components/script/dom/document.rs

@@ -204,6 +204,16 @@ pub enum IsHTMLDocument {
     NonHTMLDocument,
 }
 
+#[derive(JSTraceable, MallocSizeOf)]
+#[unrooted_must_root_lint::must_root]
+enum FocusTransaction {
+    /// No focus operation is in effect.
+    NotInTransaction,
+    /// A focus operation is in effect.
+    /// Contains the element that has most recently requested focus for itself.
+    InTransaction(Option<Dom<Element>>),
+}
+
 /// <https://dom.spec.whatwg.org/#document>
 #[dom_struct]
 pub struct Document {
@@ -243,8 +253,8 @@ pub struct Document {
     ready_state: Cell<DocumentReadyState>,
     /// Whether the DOMContentLoaded event has already been dispatched.
     domcontentloaded_dispatched: Cell<bool>,
-    /// The element that has most recently requested focus for itself.
+    /// The state of this document's focus transaction.
-    possibly_focused: MutNullableDom<Element>,
+    focus_transaction: DomRefCell<FocusTransaction>,
     /// The element that currently has the document focus context.
     focused: MutNullableDom<Element>,
     /// The script element that is currently executing.
@@ -1011,21 +1021,52 @@ impl Document {
 
     /// Initiate a new round of checking for elements requesting focus. The last element to call
     /// `request_focus` before `commit_focus_transaction` is called will receive focus.
-    pub fn begin_focus_transaction(&self) {
+    fn begin_focus_transaction(&self) {
-        self.possibly_focused.set(None);
+        *self.focus_transaction.borrow_mut() = FocusTransaction::InTransaction(Default::default());
+    }
+
+    /// <https://html.spec.whatwg.org/multipage/#focus-fixup-rule>
+    pub(crate) fn perform_focus_fixup_rule(&self, not_focusable: &Element) {
+        if Some(not_focusable) != self.focused.get().as_ref().map(|e| &**e) {
+            return;
+        }
+        self.request_focus(
+            self.GetBody().as_ref().map(|e| &*e.upcast()),
+            FocusType::Element,
+        )
     }
 
     /// Request that the given element receive focus once the current transaction is complete.
-    pub fn request_focus(&self, elem: &Element) {
+    /// If None is passed, then whatever element is currently focused will no longer be focused
-        if elem.is_focusable_area() {
+    /// once the transaction is complete.
-            self.possibly_focused.set(Some(elem))
+    pub(crate) fn request_focus(&self, elem: Option<&Element>, focus_type: FocusType) {
+        let implicit_transaction = matches!(
+            *self.focus_transaction.borrow(),
+            FocusTransaction::NotInTransaction
+        );
+        if implicit_transaction {
+            self.begin_focus_transaction();
+        }
+        if elem.map_or(true, |e| e.is_focusable_area()) {
+            *self.focus_transaction.borrow_mut() =
+                FocusTransaction::InTransaction(elem.map(Dom::from_ref));
+        }
+        if implicit_transaction {
+            self.commit_focus_transaction(focus_type);
         }
     }
 
     /// Reassign the focus context to the element that last requested focus during this
     /// transaction, or none if no elements requested it.
-    pub fn commit_focus_transaction(&self, focus_type: FocusType) {
+    fn commit_focus_transaction(&self, focus_type: FocusType) {
-        if self.focused == self.possibly_focused.get().as_deref() {
+        let possibly_focused = match *self.focus_transaction.borrow() {
+            FocusTransaction::NotInTransaction => unreachable!(),
+            FocusTransaction::InTransaction(ref elem) => {
+                elem.as_ref().map(|e| DomRoot::from_ref(&**e))
+            },
+        };
+        *self.focus_transaction.borrow_mut() = FocusTransaction::NotInTransaction;
+        if self.focused == possibly_focused.as_ref().map(|e| &**e) {
             return;
         }
         if let Some(ref elem) = self.focused.get() {
@@ -1040,7 +1081,7 @@ impl Document {
             }
         }
 
-        self.focused.set(self.possibly_focused.get().as_deref());
+        self.focused.set(possibly_focused.as_ref().map(|e| &**e));
 
         if let Some(ref elem) = self.focused.get() {
             elem.set_focus_state(true);
@@ -1140,6 +1181,7 @@ impl Document {
             }
 
             self.begin_focus_transaction();
+            self.request_focus(Some(&*el), FocusType::Element);
         }
 
         // https://w3c.github.io/uievents/#event-type-click
@@ -2980,7 +3022,7 @@ impl Document {
             stylesheet_list: MutNullableDom::new(None),
             ready_state: Cell::new(ready_state),
             domcontentloaded_dispatched: Cell::new(domcontentloaded_dispatched),
-            possibly_focused: Default::default(),
+            focus_transaction: DomRefCell::new(FocusTransaction::NotInTransaction),
             focused: Default::default(),
             current_script: Default::default(),
             pending_parsing_blocking_script: Default::default(),

components/script/dom/element.rs

@@ -1300,12 +1300,12 @@ impl Element {
         if self.is_actually_disabled() {
             return false;
         }
-        // TODO: Check whether the element is being rendered (i.e. not hidden).
         let node = self.upcast::<Node>();
         if node.get_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE) {
             return true;
         }
-        // https://html.spec.whatwg.org/multipage/#specially-focusable
+
+        // <a>, <input>, <select>, and <textrea> are inherently focusable.
         match node.type_id() {
             NodeTypeId::Element(ElementTypeId::HTMLElement(
                 HTMLElementTypeId::HTMLAnchorElement,
@@ -1832,6 +1832,67 @@ impl Element {
     pub fn get_name(&self) -> Option<Atom> {
         self.rare_data().as_ref()?.name_attribute.clone()
     }
+
+    fn is_sequentially_focusable(&self) -> bool {
+        let element = self.upcast::<Element>();
+        let node = self.upcast::<Node>();
+        if !node.is_connected() {
+            return false;
+        }
+
+        if element.has_attribute(&local_name!("hidden")) {
+            return false;
+        }
+
+        if self.disabled_state() {
+            return false;
+        }
+
+        if element.has_attribute(&local_name!("tabindex")) {
+            return true;
+        }
+
+        match node.type_id() {
+            // <button>, <select>, <iframe>, and <textarea> are implicitly focusable.
+            NodeTypeId::Element(ElementTypeId::HTMLElement(
+                HTMLElementTypeId::HTMLButtonElement,
+            )) |
+            NodeTypeId::Element(ElementTypeId::HTMLElement(
+                HTMLElementTypeId::HTMLSelectElement,
+            )) |
+            NodeTypeId::Element(ElementTypeId::HTMLElement(
+                HTMLElementTypeId::HTMLIFrameElement,
+            )) |
+            NodeTypeId::Element(ElementTypeId::HTMLElement(
+                HTMLElementTypeId::HTMLTextAreaElement,
+            )) => true,
+
+            // Links that generate actual links are focusable.
+            NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::HTMLLinkElement)) |
+            NodeTypeId::Element(ElementTypeId::HTMLElement(
+                HTMLElementTypeId::HTMLAnchorElement,
+            )) => element.has_attribute(&local_name!("href")),
+
+            //TODO focusable if editing host
+            //TODO focusable if "sorting interface th elements"
+            _ => {
+                // Draggable elements are focusable.
+                element.get_string_attribute(&local_name!("draggable")) == "true"
+            },
+        }
+    }
+
+    pub(crate) fn update_sequentially_focusable_status(&self) {
+        let node = self.upcast::<Node>();
+        let is_sequentially_focusable = self.is_sequentially_focusable();
+        node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, is_sequentially_focusable);
+
+        // https://html.spec.whatwg.org/multipage/#focus-fixup-rule
+        if !is_sequentially_focusable {
+            let document = document_from_node(self);
+            document.perform_focus_fixup_rule(self);
+        }
+    }
 }
 
 impl ElementMethods for Element {
@@ -2751,6 +2812,9 @@ impl VirtualMethods for Element {
         let node = self.upcast::<Node>();
         let doc = node.owner_doc();
         match attr.local_name() {
+            &local_name!("tabindex") | &local_name!("draggable") | &local_name!("hidden") => {
+                self.update_sequentially_focusable_status()
+            },
             &local_name!("style") => {
                 // Modifying the `style` attribute might change style.
                 *self.style_attribute.borrow_mut() = match mutation {
@@ -2917,6 +2981,8 @@ impl VirtualMethods for Element {
             return;
         }
 
+        self.update_sequentially_focusable_status();
+
         if let Some(ref value) = *self.id_attribute.borrow() {
             if let Some(shadow_root) = self.upcast::<Node>().containing_shadow_root() {
                 shadow_root.register_element_id(self, value.clone());
@@ -2945,6 +3011,8 @@ impl VirtualMethods for Element {
             return;
         }
 
+        self.update_sequentially_focusable_status();
+
         let doc = document_from_node(self);
 
         if let Some(ref shadow_root) = self.shadow_root() {

components/script/dom/htmlbuttonelement.rs

@@ -233,6 +233,7 @@ impl VirtualMethods for HTMLButtonElement {
                         el.check_ancestors_disabled_state_for_form_control();
                     },
                 }
+                el.update_sequentially_focusable_status();
             },
             &local_name!("type") => match mutation {
                 AttributeMutation::Set(_) => {

components/script/dom/htmlelement.rs

@@ -28,7 +28,7 @@ use crate::dom::htmlinputelement::{HTMLInputElement, InputType};
 use crate::dom::htmllabelelement::HTMLLabelElement;
 use crate::dom::htmltextareaelement::HTMLTextAreaElement;
 use crate::dom::node::{document_from_node, window_from_node};
-use crate::dom::node::{BindContext, Node, NodeFlags, ShadowIncluding};
+use crate::dom::node::{Node, ShadowIncluding};
 use crate::dom::text::Text;
 use crate::dom::virtualmethods::VirtualMethods;
 use dom_struct::dom_struct;
@@ -91,53 +91,6 @@ impl HTMLElement {
         let eventtarget = self.upcast::<EventTarget>();
         eventtarget.is::<HTMLBodyElement>() || eventtarget.is::<HTMLFrameSetElement>()
     }
-
-    fn update_sequentially_focusable_status(&self) {
-        let element = self.upcast::<Element>();
-        let node = self.upcast::<Node>();
-        if element.has_attribute(&local_name!("tabindex")) {
-            node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true);
-        } else {
-            match node.type_id() {
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLButtonElement,
-                )) |
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLSelectElement,
-                )) |
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLIFrameElement,
-                )) |
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLTextAreaElement,
-                )) => node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true),
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLLinkElement,
-                )) |
-                NodeTypeId::Element(ElementTypeId::HTMLElement(
-                    HTMLElementTypeId::HTMLAnchorElement,
-                )) => {
-                    if element.has_attribute(&local_name!("href")) {
-                        node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true);
-                    }
-                },
-                _ => {
-                    if let Some(attr) = element.get_attribute(&ns!(), &local_name!("draggable")) {
-                        let value = attr.value();
-                        let is_true = match *value {
-                            AttrValue::String(ref string) => string == "true",
-                            _ => false,
-                        };
-                        node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, is_true);
-                    } else {
-                        node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, false);
-                    }
-                    //TODO set SEQUENTIALLY_FOCUSABLE flag if editing host
-                    //TODO set SEQUENTIALLY_FOCUSABLE flag if "sorting interface th elements"
-                },
-            }
-        }
-    }
 }
 
 impl HTMLElementMethods for HTMLElement {
@@ -411,9 +364,7 @@ impl HTMLElementMethods for HTMLElement {
         // TODO: Mark the element as locked for focus and run the focusing steps.
         // https://html.spec.whatwg.org/multipage/#focusing-steps
         let document = document_from_node(self);
-        document.begin_focus_transaction();
+        document.request_focus(Some(self.upcast()), FocusType::Element);
-        document.request_focus(self.upcast());
-        document.commit_focus_transaction(FocusType::Element);
     }
 
     // https://html.spec.whatwg.org/multipage/#dom-blur
@@ -424,9 +375,7 @@ impl HTMLElementMethods for HTMLElement {
         }
         // https://html.spec.whatwg.org/multipage/#unfocusing-steps
         let document = document_from_node(self);
-        document.begin_focus_transaction();
+        document.request_focus(None, FocusType::Element);
-        // If `request_focus` is not called, focus will be set to None.
-        document.commit_focus_transaction(FocusType::Element);
     }
 
     // https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent
@@ -859,13 +808,6 @@ impl VirtualMethods for HTMLElement {
         }
     }
 
-    fn bind_to_tree(&self, context: &BindContext) {
-        if let Some(ref s) = self.super_type() {
-            s.bind_to_tree(context);
-        }
-        self.update_sequentially_focusable_status();
-    }
-
     fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
         match name {
             &local_name!("itemprop") => AttrValue::from_serialized_tokenlist(value.into()),

components/script/dom/htmlfieldsetelement.rs

@@ -182,14 +182,17 @@ impl VirtualMethods for HTMLFieldSetElement {
                         let el = field.downcast::<Element>().unwrap();
                         el.set_disabled_state(true);
                         el.set_enabled_state(false);
+                        el.update_sequentially_focusable_status();
                     }
                 } else {
                     for field in fields {
                         let el = field.downcast::<Element>().unwrap();
                         el.check_disabled_attribute();
                         el.check_ancestors_disabled_state_for_form_control();
+                        el.update_sequentially_focusable_status();
                     }
                 }
+                el.update_sequentially_focusable_status();
             },
             &local_name!("form") => {
                 self.form_attribute_mutated(mutation);

components/script/dom/htmlinputelement.rs

@@ -2291,6 +2291,8 @@ impl VirtualMethods for HTMLInputElement {
                     let read_write = !(self.ReadOnly() || el.disabled_state());
                     el.set_read_write_state(read_write);
                 }
+
+                el.update_sequentially_focusable_status();
             },
             &local_name!("checked") if !self.checked_changed.get() => {
                 let checked_state = match mutation {
@@ -2520,7 +2522,6 @@ impl VirtualMethods for HTMLInputElement {
 
             //TODO: set the editing position for text inputs
 
-            document_from_node(self).request_focus(self.upcast());
             if self.input_type().is_textual_or_password() &&
                 // Check if we display a placeholder. Layout doesn't know about this.
                 !self.textinput.borrow().is_empty()

components/script/dom/htmltextareaelement.rs

@@ -23,7 +23,7 @@ use crate::dom::htmlfieldsetelement::HTMLFieldSetElement;
 use crate::dom::htmlformelement::{FormControl, HTMLFormElement};
 use crate::dom::htmlinputelement::HTMLInputElement;
 use crate::dom::keyboardevent::KeyboardEvent;
-use crate::dom::node::{document_from_node, window_from_node};
+use crate::dom::node::window_from_node;
 use crate::dom::node::{
     BindContext, ChildrenMutation, CloneChildrenFlag, Node, NodeDamage, UnbindContext,
 };
@@ -478,6 +478,7 @@ impl VirtualMethods for HTMLTextAreaElement {
                         }
                     },
                 }
+                el.update_sequentially_focusable_status();
             },
             local_name!("maxlength") => match *attr.value() {
                 AttrValue::Int(_, value) => {
@@ -606,8 +607,6 @@ impl VirtualMethods for HTMLTextAreaElement {
 
         if event.type_() == atom!("click") && !event.DefaultPrevented() {
             //TODO: set the editing position for text inputs
-
-            document_from_node(self).request_focus(self.upcast());
         } else if event.type_() == atom!("keydown") && !event.DefaultPrevented() {
             if let Some(kevent) = event.downcast::<KeyboardEvent>() {
                 // This can't be inlined, as holding on to textinput.borrow_mut()

components/script/script_thread.rs

@@ -2504,9 +2504,7 @@ impl ScriptThread {
         let frame_element = doc.find_iframe(browsing_context_id);
 
         if let Some(ref frame_element) = frame_element {
-            doc.begin_focus_transaction();
+            doc.request_focus(Some(frame_element.upcast()), FocusType::Parent);
-            doc.request_focus(frame_element.upcast());
-            doc.commit_focus_transaction(FocusType::Parent);
         }
     }
 

tests/wpt/metadata/html/interaction/focus/processing-model/focus-fixup-rule-one-no-dialogs.html.ini

@@ -2,21 +2,6 @@
   [Disabling contenteditable]
     expected: FAIL
 
-  [Hiding the active element]
-    expected: FAIL
-
-  [Disabling the active element (making it expressly inert)]
-    expected: FAIL
-
   [Changing the first legend element in disabled <fieldset>]
     expected: FAIL
 
-  [Disabling <fieldset> affects its descendants]
-    expected: FAIL
-
-  [Removing the tabindex attribute from a div]
-    expected: FAIL
-
-  [Removing the active element from the DOM]
-    expected: FAIL
-