Changeset 274365 in webkit

Timestamp:

Mar 12, 2021 11:03:26 AM (16 months ago)

Author:

Manuel Rego Casasnovas

Message:

[selectors] :focus-visible matches body after keyboard event
​https://bugs.webkit.org/show_bug.cgi?id=223113

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

• web-platform-tests/css/selectors/focus-visible-001.html: Modify the test to verify

that only the element matches :focus-visible (and not the body).

• web-platform-tests/css/selectors/focus-visible-019-expected.txt: Added.
• web-platform-tests/css/selectors/focus-visible-019.html: Added new test to check script focus in keyboard event,

and that again only the element matches :focus-visible (and not the body).

Source/WebCore:

Fix the bug with some changes in EventHandler::internalKeyEvent().
When you use TAB (or other key) the |element| variable in this method is the document body,
however that element is not focused (element->focused() is false).
Before this patch we were marking the element as matchin :focus-visible,
however we shouldn't do that if the element is not focused (added a condition to avoid doing that).

Apart from that this patch also fixes a related issue, if a keyboard event handler is changing focus via script
there's a part of this method that takes care of updating the |element| variable.
In that case we have to remove :focus-visible flag from the previous element, and add it to the new one.

Test: imported/w3c/web-platform-tests/css/selectors/focus-visible-001.html
Test: imported/w3c/web-platform-tests/css/selectors/focus-visible-019.html

• page/EventHandler.cpp:

(WebCore::EventHandler::internalKeyEvent):

LayoutTests:

• platform/ios/TestExpectations: Skip new test for iOS.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-001.html (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-019-expected.txt (added)
• LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-019.html (added)
• LayoutTests/platform/ios/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/EventHandler.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-03-12  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [selectors] :focus-visible matches body after keyboard event
+        https://bugs.webkit.org/show_bug.cgi?id=223113
+
+        Reviewed by Antti Koivisto.
+
+        * platform/ios/TestExpectations: Skip new test for iOS.
+
 2021-03-12  Robert Jenner  <jenner@apple.com>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2021-03-12  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [selectors] :focus-visible matches body after keyboard event
+        https://bugs.webkit.org/show_bug.cgi?id=223113
+
+        Reviewed by Antti Koivisto.
+
+        * web-platform-tests/css/selectors/focus-visible-001.html: Modify the test to verify
+        that only the element matches :focus-visible (and not the body).
+        * web-platform-tests/css/selectors/focus-visible-019-expected.txt: Added.
+        * web-platform-tests/css/selectors/focus-visible-019.html: Added new test to check script focus in keyboard event,
+        and that again only the element matches :focus-visible (and not the body).
+
 2021-03-12  Antoine Quint  <graouts@webkit.org>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/selectors/focus-visible-001.html

@@ -40 +40 @@
   <script>
     async_test(function(t) {
-      el.addEventListener("focus", t.step_func(function() {
+      el.addEventListener("focus", t.step_func_done(function() {
         assert_equals(getComputedStyle(el).outlineColor, "rgb(0, 128, 0)", `outlineColor for ${el.tagName}#${el.id} should be green`);
         assert_not_equals(getComputedStyle(el).backgroundColor, "rgb(255, 0, 0)", `backgroundColor for ${el.tagName}#${el.id} should NOT be red`);
-        t.done();
+
+        let focusVisiblePseudoAll = document.querySelectorAll(':focus-visible');
+        assert_equals(focusVisiblePseudoAll.length, 1, "Only 1 element matches ':focus-visible'");
+        let focusVisiblePseudo = document.querySelector(':focus-visible');
+        assert_equals(el, focusVisiblePseudo, "${el.tagName}#${el.id} matches ':focus-visible'");
       }));
       const tab_key = '\ue004';

trunk/LayoutTests/platform/ios/TestExpectations

@@ -3396 +3396 @@
 webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-012.html [ Skip ]
 webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-013.html [ Skip ]
+webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-019.html [ Skip ]
 webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/hover-002.html [ Skip ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-03-12  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        [selectors] :focus-visible matches body after keyboard event
+        https://bugs.webkit.org/show_bug.cgi?id=223113
+
+        Reviewed by Antti Koivisto.
+
+        Fix the bug with some changes in EventHandler::internalKeyEvent().
+        When you use TAB (or other key) the |element| variable in this method is the document body,
+        however that element is not focused (element->focused() is false).
+        Before this patch we were marking the element as matchin :focus-visible,
+        however we shouldn't do that if the element is not focused (added a condition to avoid doing that).
+
+        Apart from that this patch also fixes a related issue, if a keyboard event handler is changing focus via script
+        there's a part of this method that takes care of updating the |element| variable.
+        In that case we have to remove :focus-visible flag from the previous element, and add it to the new one.
+
+        Test: imported/w3c/web-platform-tests/css/selectors/focus-visible-001.html
+        Test: imported/w3c/web-platform-tests/css/selectors/focus-visible-019.html
+
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::internalKeyEvent):
+
 2021-03-12  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebCore/page/EventHandler.cpp

@@ -3534 +3534 @@
     keydown->setTarget(element);
 
-    // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
-    // Just typing a modifier key is not considered user interaction with the page, but Shift + a (or Caps Lock + a) is considered an interaction.
-    if (keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty()))
-        element->setHasFocusVisible(true);
+    auto shouldMatchFocusVisible = [initialKeyEvent, keydown](const Element& element) {
+        if (!element.focused())
+            return false;
+
+        // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
+        // Just typing a modifier key is not considered user interaction with the page, but Shift + a (or Caps Lock + a) is considered an interaction.
+        return keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
+    };
+    element->setHasFocusVisible(shouldMatchFocusVisible(*element));
 
     if (initialKeyEvent.type() == PlatformEvent::RawKeyDown) {
@@ -3584 +3589 @@
     // But if we are dispatching a fake backward compatibility keypress, then we pretend that the keypress happened on the original element.
     if (!keydownResult) {
+        element->setHasFocusVisible(false);
         element = eventTargetElementForDocument(m_frame.document());
         if (!element)
             return false;
+        element->setHasFocusVisible(shouldMatchFocusVisible(*element));
     }