Skip to content

Commit f31d345

Browse files
bors-servobors-servo
authored
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
2 parents 6b0d9af + d55424e commit f31d345

File tree

9 files changed

+135
-96
lines changed

9 files changed

+135
-96
lines changed

‎components/script/dom/document.rs‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,16 @@ pub enum IsHTMLDocument {
204204
NonHTMLDocument,
205205
}
206206

207+
#[derive(JSTraceable, MallocSizeOf)]
208+
#[unrooted_must_root_lint::must_root]
209+
enum FocusTransaction {
210+
/// No focus operation is in effect.
211+
NotInTransaction,
212+
/// A focus operation is in effect.
213+
/// Contains the element that has most recently requested focus for itself.
214+
InTransaction(Option<Dom<Element>>),
215+
}
216+
207217
/// <https://dom.spec.whatwg.org/#document>
208218
#[dom_struct]
209219
pub struct Document {
@@ -243,8 +253,8 @@ pub struct Document {
243253
ready_state: Cell<DocumentReadyState>,
244254
/// Whether the DOMContentLoaded event has already been dispatched.
245255
domcontentloaded_dispatched: Cell<bool>,
246-
/// The element that has most recently requested focus for itself.
247-
possibly_focused: MutNullableDom<Element>,
256+
/// The state of this document's focus transaction.
257+
focus_transaction: DomRefCell<FocusTransaction>,
248258
/// The element that currently has the document focus context.
249259
focused: MutNullableDom<Element>,
250260
/// The script element that is currently executing.
@@ -1011,21 +1021,52 @@ impl Document {
10111021

10121022
/// Initiate a new round of checking for elements requesting focus. The last element to call
10131023
/// `request_focus` before `commit_focus_transaction` is called will receive focus.
1014-
pub fn begin_focus_transaction(&self) {
1015-
self.possibly_focused.set(None);
1024+
fn begin_focus_transaction(&self) {
1025+
*self.focus_transaction.borrow_mut() = FocusTransaction::InTransaction(Default::default());
1026+
}
1027+
1028+
/// <https://html.spec.whatwg.org/multipage/#focus-fixup-rule>
1029+
pub(crate) fn perform_focus_fixup_rule(&self, not_focusable: &Element) {
1030+
if Some(not_focusable) != self.focused.get().as_ref().map(|e| &**e) {
1031+
return;
1032+
}
1033+
self.request_focus(
1034+
self.GetBody().as_ref().map(|e| &*e.upcast()),
1035+
FocusType::Element,
1036+
)
10161037
}
10171038

10181039
/// Request that the given element receive focus once the current transaction is complete.
1019-
pub fn request_focus(&self, elem: &Element) {
1020-
if elem.is_focusable_area() {
1021-
self.possibly_focused.set(Some(elem))
1040+
/// If None is passed, then whatever element is currently focused will no longer be focused
1041+
/// once the transaction is complete.
1042+
pub(crate) fn request_focus(&self, elem: Option<&Element>, focus_type: FocusType) {
1043+
let implicit_transaction = matches!(
1044+
*self.focus_transaction.borrow(),
1045+
FocusTransaction::NotInTransaction
1046+
);
1047+
if implicit_transaction {
1048+
self.begin_focus_transaction();
1049+
}
1050+
if elem.map_or(true, |e| e.is_focusable_area()) {
1051+
*self.focus_transaction.borrow_mut() =
1052+
FocusTransaction::InTransaction(elem.map(Dom::from_ref));
1053+
}
1054+
if implicit_transaction {
1055+
self.commit_focus_transaction(focus_type);
10221056
}
10231057
}
10241058

10251059
/// Reassign the focus context to the element that last requested focus during this
10261060
/// transaction, or none if no elements requested it.
1027-
pub fn commit_focus_transaction(&self, focus_type: FocusType) {
1028-
if self.focused == self.possibly_focused.get().as_deref() {
1061+
fn commit_focus_transaction(&self, focus_type: FocusType) {
1062+
let possibly_focused = match *self.focus_transaction.borrow() {
1063+
FocusTransaction::NotInTransaction => unreachable!(),
1064+
FocusTransaction::InTransaction(ref elem) => {
1065+
elem.as_ref().map(|e| DomRoot::from_ref(&**e))
1066+
},
1067+
};
1068+
*self.focus_transaction.borrow_mut() = FocusTransaction::NotInTransaction;
1069+
if self.focused == possibly_focused.as_ref().map(|e| &**e) {
10291070
return;
10301071
}
10311072
if let Some(ref elem) = self.focused.get() {
@@ -1040,7 +1081,7 @@ impl Document {
10401081
}
10411082
}
10421083

1043-
self.focused.set(self.possibly_focused.get().as_deref());
1084+
self.focused.set(possibly_focused.as_ref().map(|e| &**e));
10441085

10451086
if let Some(ref elem) = self.focused.get() {
10461087
elem.set_focus_state(true);
@@ -1140,6 +1181,7 @@ impl Document {
11401181
}
11411182

11421183
self.begin_focus_transaction();
1184+
self.request_focus(Some(&*el), FocusType::Element);
11431185
}
11441186

11451187
// https://w3c.github.io/uievents/#event-type-click
@@ -2980,7 +3022,7 @@ impl Document {
29803022
stylesheet_list: MutNullableDom::new(None),
29813023
ready_state: Cell::new(ready_state),
29823024
domcontentloaded_dispatched: Cell::new(domcontentloaded_dispatched),
2983-
possibly_focused: Default::default(),
3025+
focus_transaction: DomRefCell::new(FocusTransaction::NotInTransaction),
29843026
focused: Default::default(),
29853027
current_script: Default::default(),
29863028
pending_parsing_blocking_script: Default::default(),

‎components/script/dom/element.rs‎

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1300,12 +1300,12 @@ impl Element {
13001300
if self.is_actually_disabled() {
13011301
return false;
13021302
}
1303-
// TODO: Check whether the element is being rendered (i.e. not hidden).
13041303
let node = self.upcast::<Node>();
13051304
if node.get_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE) {
13061305
return true;
13071306
}
1308-
// https://html.spec.whatwg.org/multipage/#specially-focusable
1307+
1308+
// <a>, <input>, <select>, and <textrea> are inherently focusable.
13091309
match node.type_id() {
13101310
NodeTypeId::Element(ElementTypeId::HTMLElement(
13111311
HTMLElementTypeId::HTMLAnchorElement,
@@ -1832,6 +1832,67 @@ impl Element {
18321832
pub fn get_name(&self) -> Option<Atom> {
18331833
self.rare_data().as_ref()?.name_attribute.clone()
18341834
}
1835+
1836+
fn is_sequentially_focusable(&self) -> bool {
1837+
let element = self.upcast::<Element>();
1838+
let node = self.upcast::<Node>();
1839+
if !node.is_connected() {
1840+
return false;
1841+
}
1842+
1843+
if element.has_attribute(&local_name!("hidden")) {
1844+
return false;
1845+
}
1846+
1847+
if self.disabled_state() {
1848+
return false;
1849+
}
1850+
1851+
if element.has_attribute(&local_name!("tabindex")) {
1852+
return true;
1853+
}
1854+
1855+
match node.type_id() {
1856+
// <button>, <select>, <iframe>, and <textarea> are implicitly focusable.
1857+
NodeTypeId::Element(ElementTypeId::HTMLElement(
1858+
HTMLElementTypeId::HTMLButtonElement,
1859+
)) |
1860+
NodeTypeId::Element(ElementTypeId::HTMLElement(
1861+
HTMLElementTypeId::HTMLSelectElement,
1862+
)) |
1863+
NodeTypeId::Element(ElementTypeId::HTMLElement(
1864+
HTMLElementTypeId::HTMLIFrameElement,
1865+
)) |
1866+
NodeTypeId::Element(ElementTypeId::HTMLElement(
1867+
HTMLElementTypeId::HTMLTextAreaElement,
1868+
)) => true,
1869+
1870+
// Links that generate actual links are focusable.
1871+
NodeTypeId::Element(ElementTypeId::HTMLElement(HTMLElementTypeId::HTMLLinkElement)) |
1872+
NodeTypeId::Element(ElementTypeId::HTMLElement(
1873+
HTMLElementTypeId::HTMLAnchorElement,
1874+
)) => element.has_attribute(&local_name!("href")),
1875+
1876+
//TODO focusable if editing host
1877+
//TODO focusable if "sorting interface th elements"
1878+
_ => {
1879+
// Draggable elements are focusable.
1880+
element.get_string_attribute(&local_name!("draggable")) == "true"
1881+
},
1882+
}
1883+
}
1884+
1885+
pub(crate) fn update_sequentially_focusable_status(&self) {
1886+
let node = self.upcast::<Node>();
1887+
let is_sequentially_focusable = self.is_sequentially_focusable();
1888+
node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, is_sequentially_focusable);
1889+
1890+
// https://html.spec.whatwg.org/multipage/#focus-fixup-rule
1891+
if !is_sequentially_focusable {
1892+
let document = document_from_node(self);
1893+
document.perform_focus_fixup_rule(self);
1894+
}
1895+
}
18351896
}
18361897

18371898
impl ElementMethods for Element {
@@ -2751,6 +2812,9 @@ impl VirtualMethods for Element {
27512812
let node = self.upcast::<Node>();
27522813
let doc = node.owner_doc();
27532814
match attr.local_name() {
2815+
&local_name!("tabindex") | &local_name!("draggable") | &local_name!("hidden") => {
2816+
self.update_sequentially_focusable_status()
2817+
},
27542818
&local_name!("style") => {
27552819
// Modifying the `style` attribute might change style.
27562820
*self.style_attribute.borrow_mut() = match mutation {
@@ -2917,6 +2981,8 @@ impl VirtualMethods for Element {
29172981
return;
29182982
}
29192983

2984+
self.update_sequentially_focusable_status();
2985+
29202986
if let Some(ref value) = *self.id_attribute.borrow() {
29212987
if let Some(shadow_root) = self.upcast::<Node>().containing_shadow_root() {
29222988
shadow_root.register_element_id(self, value.clone());
@@ -2945,6 +3011,8 @@ impl VirtualMethods for Element {
29453011
return;
29463012
}
29473013

3014+
self.update_sequentially_focusable_status();
3015+
29483016
let doc = document_from_node(self);
29493017

29503018
if let Some(ref shadow_root) = self.shadow_root() {

‎components/script/dom/htmlbuttonelement.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@ impl VirtualMethods for HTMLButtonElement {
233233
el.check_ancestors_disabled_state_for_form_control();
234234
},
235235
}
236+
el.update_sequentially_focusable_status();
236237
},
237238
&local_name!("type") => match mutation {
238239
AttributeMutation::Set(_) => {

‎components/script/dom/htmlelement.rs‎

Lines changed: 3 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use crate::dom::htmlinputelement::{HTMLInputElement, InputType};
2828
use crate::dom::htmllabelelement::HTMLLabelElement;
2929
use crate::dom::htmltextareaelement::HTMLTextAreaElement;
3030
use crate::dom::node::{document_from_node, window_from_node};
31-
use crate::dom::node::{BindContext, Node, NodeFlags, ShadowIncluding};
31+
use crate::dom::node::{Node, ShadowIncluding};
3232
use crate::dom::text::Text;
3333
use crate::dom::virtualmethods::VirtualMethods;
3434
use dom_struct::dom_struct;
@@ -91,53 +91,6 @@ impl HTMLElement {
9191
let eventtarget = self.upcast::<EventTarget>();
9292
eventtarget.is::<HTMLBodyElement>() || eventtarget.is::<HTMLFrameSetElement>()
9393
}
94-
95-
fn update_sequentially_focusable_status(&self) {
96-
let element = self.upcast::<Element>();
97-
let node = self.upcast::<Node>();
98-
if element.has_attribute(&local_name!("tabindex")) {
99-
node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true);
100-
} else {
101-
match node.type_id() {
102-
NodeTypeId::Element(ElementTypeId::HTMLElement(
103-
HTMLElementTypeId::HTMLButtonElement,
104-
)) |
105-
NodeTypeId::Element(ElementTypeId::HTMLElement(
106-
HTMLElementTypeId::HTMLSelectElement,
107-
)) |
108-
NodeTypeId::Element(ElementTypeId::HTMLElement(
109-
HTMLElementTypeId::HTMLIFrameElement,
110-
)) |
111-
NodeTypeId::Element(ElementTypeId::HTMLElement(
112-
HTMLElementTypeId::HTMLTextAreaElement,
113-
)) => node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true),
114-
NodeTypeId::Element(ElementTypeId::HTMLElement(
115-
HTMLElementTypeId::HTMLLinkElement,
116-
)) |
117-
NodeTypeId::Element(ElementTypeId::HTMLElement(
118-
HTMLElementTypeId::HTMLAnchorElement,
119-
)) => {
120-
if element.has_attribute(&local_name!("href")) {
121-
node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, true);
122-
}
123-
},
124-
_ => {
125-
if let Some(attr) = element.get_attribute(&ns!(), &local_name!("draggable")) {
126-
let value = attr.value();
127-
let is_true = match *value {
128-
AttrValue::String(ref string) => string == "true",
129-
_ => false,
130-
};
131-
node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, is_true);
132-
} else {
133-
node.set_flag(NodeFlags::SEQUENTIALLY_FOCUSABLE, false);
134-
}
135-
//TODO set SEQUENTIALLY_FOCUSABLE flag if editing host
136-
//TODO set SEQUENTIALLY_FOCUSABLE flag if "sorting interface th elements"
137-
},
138-
}
139-
}
140-
}
14194
}
14295

14396
impl HTMLElementMethods for HTMLElement {
@@ -411,9 +364,7 @@ impl HTMLElementMethods for HTMLElement {
411364
// TODO: Mark the element as locked for focus and run the focusing steps.
412365
// https://html.spec.whatwg.org/multipage/#focusing-steps
413366
let document = document_from_node(self);
414-
document.begin_focus_transaction();
415-
document.request_focus(self.upcast());
416-
document.commit_focus_transaction(FocusType::Element);
367+
document.request_focus(Some(self.upcast()), FocusType::Element);
417368
}
418369

419370
// https://html.spec.whatwg.org/multipage/#dom-blur
@@ -424,9 +375,7 @@ impl HTMLElementMethods for HTMLElement {
424375
}
425376
// https://html.spec.whatwg.org/multipage/#unfocusing-steps
426377
let document = document_from_node(self);
427-
document.begin_focus_transaction();
428-
// If `request_focus` is not called, focus will be set to None.
429-
document.commit_focus_transaction(FocusType::Element);
378+
document.request_focus(None, FocusType::Element);
430379
}
431380

432381
// https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent
@@ -859,13 +808,6 @@ impl VirtualMethods for HTMLElement {
859808
}
860809
}
861810

862-
fn bind_to_tree(&self, context: &BindContext) {
863-
if let Some(ref s) = self.super_type() {
864-
s.bind_to_tree(context);
865-
}
866-
self.update_sequentially_focusable_status();
867-
}
868-
869811
fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
870812
match name {
871813
&local_name!("itemprop") => AttrValue::from_serialized_tokenlist(value.into()),

‎components/script/dom/htmlfieldsetelement.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,14 +182,17 @@ impl VirtualMethods for HTMLFieldSetElement {
182182
let el = field.downcast::<Element>().unwrap();
183183
el.set_disabled_state(true);
184184
el.set_enabled_state(false);
185+
el.update_sequentially_focusable_status();
185186
}
186187
} else {
187188
for field in fields {
188189
let el = field.downcast::<Element>().unwrap();
189190
el.check_disabled_attribute();
190191
el.check_ancestors_disabled_state_for_form_control();
192+
el.update_sequentially_focusable_status();
191193
}
192194
}
195+
el.update_sequentially_focusable_status();
193196
},
194197
&local_name!("form") => {
195198
self.form_attribute_mutated(mutation);

‎components/script/dom/htmlinputelement.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2291,6 +2291,8 @@ impl VirtualMethods for HTMLInputElement {
22912291
let read_write = !(self.ReadOnly() || el.disabled_state());
22922292
el.set_read_write_state(read_write);
22932293
}
2294+
2295+
el.update_sequentially_focusable_status();
22942296
},
22952297
&local_name!("checked") if !self.checked_changed.get() => {
22962298
let checked_state = match mutation {
@@ -2520,7 +2522,6 @@ impl VirtualMethods for HTMLInputElement {
25202522

25212523
//TODO: set the editing position for text inputs
25222524

2523-
document_from_node(self).request_focus(self.upcast());
25242525
if self.input_type().is_textual_or_password() &&
25252526
// Check if we display a placeholder. Layout doesn't know about this.
25262527
!self.textinput.borrow().is_empty()

‎components/script/dom/htmltextareaelement.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use crate::dom::htmlfieldsetelement::HTMLFieldSetElement;
2323
use crate::dom::htmlformelement::{FormControl, HTMLFormElement};
2424
use crate::dom::htmlinputelement::HTMLInputElement;
2525
use crate::dom::keyboardevent::KeyboardEvent;
26-
use crate::dom::node::{document_from_node, window_from_node};
26+
use crate::dom::node::window_from_node;
2727
use crate::dom::node::{
2828
BindContext, ChildrenMutation, CloneChildrenFlag, Node, NodeDamage, UnbindContext,
2929
};
@@ -478,6 +478,7 @@ impl VirtualMethods for HTMLTextAreaElement {
478478
}
479479
},
480480
}
481+
el.update_sequentially_focusable_status();
481482
},
482483
local_name!("maxlength") => match *attr.value() {
483484
AttrValue::Int(_, value) => {
@@ -606,8 +607,6 @@ impl VirtualMethods for HTMLTextAreaElement {
606607

607608
if event.type_() == atom!("click") && !event.DefaultPrevented() {
608609
//TODO: set the editing position for text inputs
609-
610-
document_from_node(self).request_focus(self.upcast());
611610
} else if event.type_() == atom!("keydown") && !event.DefaultPrevented() {
612611
if let Some(kevent) = event.downcast::<KeyboardEvent>() {
613612
// This can't be inlined, as holding on to textinput.borrow_mut()

0 commit comments

Comments
 (0)