Quellcode durchsuchen

LibWeb: Properly handle when (shift+)tab wraps around the page

We have support for using (shift+)tab to move focus to the next/previous
element on the page. However, there were several ways for this to crash
as written. This updates our implementation to check if we did not find
a node to move focus to, and to reset focus to the first/last node in
the document.

This doesn't seem to work when wrapping around from the first to the
last node. A FIXME has been added for that, as this would already not
work before this patch (the main focus here is not crashing).
Timothy Flynn vor 9 Monaten
Ursprung
Commit
96b5646fc1

+ 4 - 0
Tests/LibWeb/Text/expected/UIEvents/KeyEvent-tab.txt

@@ -0,0 +1,4 @@
+input1 keydown Tab
+<INPUT id="input2" >
+input2 keydown Tab
+<INPUT id="input1" >

+ 22 - 0
Tests/LibWeb/Text/input/UIEvents/KeyEvent-tab.html

@@ -0,0 +1,22 @@
+<input id="input1" />
+<input id="input2" />
+<script src="../include.js"></script>
+<script>
+    test(() => {
+        let input1 = document.getElementById("input1");
+        let input2 = document.getElementById("input2");
+
+        input1.addEventListener("keydown", e => {
+            println(`input1 keydown ${e.key}`);
+        });
+        input2.addEventListener("keydown", e => {
+            println(`input2 keydown ${e.key}`);
+        });
+
+        internals.sendKey(input1, "Tab");
+        printElement(document.activeElement);
+
+        internals.sendKey(input2, "Tab");
+        printElement(document.activeElement);
+    });
+</script>

+ 39 - 16
Userland/Libraries/LibWeb/Page/EventHandler.cpp

@@ -763,20 +763,31 @@ bool EventHandler::focus_next_element()
     if (!m_navigable->active_document()->is_fully_active())
     if (!m_navigable->active_document()->is_fully_active())
         return false;
         return false;
 
 
-    auto* element = m_navigable->active_document()->focused_element();
-    if (!element) {
-        element = m_navigable->active_document()->first_child_of_type<DOM::Element>();
-        if (element && element->is_focusable()) {
-            m_navigable->active_document()->set_focused_element(element);
-            return true;
+    auto set_focus_to_first_focusable_element = [&]() {
+        auto* element = m_navigable->active_document()->first_child_of_type<DOM::Element>();
+
+        for (; element; element = element->next_element_in_pre_order()) {
+            if (element->is_focusable()) {
+                m_navigable->active_document()->set_focused_element(element);
+                return true;
+            }
         }
         }
-    }
+
+        return false;
+    };
+
+    auto* element = m_navigable->active_document()->focused_element();
+    if (!element)
+        return set_focus_to_first_focusable_element();
 
 
     for (element = element->next_element_in_pre_order(); element && !element->is_focusable(); element = element->next_element_in_pre_order())
     for (element = element->next_element_in_pre_order(); element && !element->is_focusable(); element = element->next_element_in_pre_order())
         ;
         ;
 
 
+    if (!element)
+        return set_focus_to_first_focusable_element();
+
     m_navigable->active_document()->set_focused_element(element);
     m_navigable->active_document()->set_focused_element(element);
-    return element;
+    return true;
 }
 }
 
 
 bool EventHandler::focus_previous_element()
 bool EventHandler::focus_previous_element()
@@ -786,20 +797,32 @@ bool EventHandler::focus_previous_element()
     if (!m_navigable->active_document()->is_fully_active())
     if (!m_navigable->active_document()->is_fully_active())
         return false;
         return false;
 
 
-    auto* element = m_navigable->active_document()->focused_element();
-    if (!element) {
-        element = m_navigable->active_document()->last_child_of_type<DOM::Element>();
-        if (element && element->is_focusable()) {
-            m_navigable->active_document()->set_focused_element(element);
-            return true;
+    auto set_focus_to_last_focusable_element = [&]() {
+        // FIXME: This often returns the HTML element itself, which has no previous sibling.
+        auto* element = m_navigable->active_document()->last_child_of_type<DOM::Element>();
+
+        for (; element; element = element->previous_element_in_pre_order()) {
+            if (element->is_focusable()) {
+                m_navigable->active_document()->set_focused_element(element);
+                return true;
+            }
         }
         }
-    }
+
+        return false;
+    };
+
+    auto* element = m_navigable->active_document()->focused_element();
+    if (!element)
+        return set_focus_to_last_focusable_element();
 
 
     for (element = element->previous_element_in_pre_order(); element && !element->is_focusable(); element = element->previous_element_in_pre_order())
     for (element = element->previous_element_in_pre_order(); element && !element->is_focusable(); element = element->previous_element_in_pre_order())
         ;
         ;
 
 
+    if (!element)
+        return set_focus_to_last_focusable_element();
+
     m_navigable->active_document()->set_focused_element(element);
     m_navigable->active_document()->set_focused_element(element);
-    return element;
+    return true;
 }
 }
 
 
 constexpr bool should_ignore_keydown_event(u32 code_point, u32 modifiers)
 constexpr bool should_ignore_keydown_event(u32 code_point, u32 modifiers)