Changeset 276698 in webkit


Ignore:
Timestamp:
Apr 28, 2021 1:51:37 AM (15 months ago)
Author:
Manuel Rego Casasnovas
Message:

[selectors] Using a modifier key on an element makes it stop matching :focus-visible
​https://bugs.webkit.org/show_bug.cgi?id=225075

Reviewed by Ryosuke Niwa.

LayoutTests/imported/w3c:

  • web-platform-tests/css/selectors/focus-visible-021-expected.txt: Added.
  • web-platform-tests/css/selectors/focus-visible-021.html: Added.

Source/WebCore:

When we used a modifier key on a element that was matching :focus-visible, we stopped matching :focus-visible.
That was wrong, we shouldn't just start matching :focus-visible when a modifier key is used but not the other way around.

This patches fixed that without doing any work if the element is already matching :focus-visible when the user type a key.

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

  • dom/Element.cpp:

(WebCore::shouldAlwaysHaveFocusVisibleWhenFocused): Helper method for inputs and content editable elements.
(WebCore::Element::setFocus): Use the new helper method.
(WebCore::Element::setHasFocusVisible): Add asserts to avoid setting/unsetting :focus-visible flag wrongly.

  • dom/Node.cpp:

(WebCore::Node::isContentEditable const): Just mark as const.
(WebCore::Node::isContentRichlyEditable const): Ditto.

  • dom/Node.h: Ditto.
  • page/EventHandler.cpp:

(WebCore::EventHandler::internalKeyEvent): Don't do anything regarding :focus-visible flag if the element already matches :focus-visible.

LayoutTests:

  • platform/ios/TestExpectations: Skip new test.
Location:
trunk
Files:
2 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r276694 r276698  
     12021-04-28  Manuel Rego Casasnovas  <rego@igalia.com>
     2
     3        [selectors] Using a modifier key on an element makes it stop matching :focus-visible
     4        https://bugs.webkit.org/show_bug.cgi?id=225075
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * platform/ios/TestExpectations: Skip new test.
     9
    1102021-04-28  Cameron McCormack  <heycam@apple.com>
    211

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r276697 r276698  
     12021-04-28  Manuel Rego Casasnovas  <rego@igalia.com>
     2
     3        [selectors] Using a modifier key on an element makes it stop matching :focus-visible
     4        https://bugs.webkit.org/show_bug.cgi?id=225075
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * web-platform-tests/css/selectors/focus-visible-021-expected.txt: Added.
     9        * web-platform-tests/css/selectors/focus-visible-021.html: Added.
     10
    1112021-04-28  Cameron McCormack  <heycam@apple.com>
    212

  • trunk/LayoutTests/platform/ios/TestExpectations

    r276412 r276698  
    32833283webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-013.html [ Skip ]
    32843284webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-019.html [ Skip ]
     3285webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-021.html [ Skip ]
    32853286webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-script-focus-004.html [ Skip ]
    32863287webkit.org/b/209734 imported/w3c/web-platform-tests/css/selectors/focus-visible-script-focus-005.html [ Skip ]

  • trunk/Source/WebCore/ChangeLog

    r276697 r276698  
     12021-04-28  Manuel Rego Casasnovas  <rego@igalia.com>
     2
     3        [selectors] Using a modifier key on an element makes it stop matching :focus-visible
     4        https://bugs.webkit.org/show_bug.cgi?id=225075
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        When we used a modifier key on a element that was matching :focus-visible, we stopped matching :focus-visible.
     9        That was wrong, we shouldn't just start matching :focus-visible when a modifier key is used but not the other way around.
     10
     11        This patches fixed that without doing any work if the element is already matching :focus-visible when the user type a key.
     12
     13        Test: imported/w3c/web-platform-tests/css/selectors/focus-visible-021.html
     14
     15        * dom/Element.cpp:
     16        (WebCore::shouldAlwaysHaveFocusVisibleWhenFocused): Helper method for inputs and content editable elements.
     17        (WebCore::Element::setFocus): Use the new helper method.
     18        (WebCore::Element::setHasFocusVisible): Add asserts to avoid setting/unsetting :focus-visible flag wrongly.
     19        * dom/Node.cpp:
     20        (WebCore::Node::isContentEditable const): Just mark as const.
     21        (WebCore::Node::isContentRichlyEditable const): Ditto.
     22        * dom/Node.h: Ditto.
     23        * page/EventHandler.cpp:
     24        (WebCore::EventHandler::internalKeyEvent): Don't do anything regarding :focus-visible flag if the element already matches :focus-visible.
     25
    1262021-04-28  Cameron McCormack  <heycam@apple.com>
    227

  • trunk/Source/WebCore/dom/Element.cpp

    r276628 r276698  
    746746}
    747747
     748static bool shouldAlwaysHaveFocusVisibleWhenFocused(const Element& element)
     749{
     750    return element.isTextField() || element.isContentEditable();
     751}
     752
    748753void Element::setFocus(bool flag, FocusVisibility visibility)
    749754{
    … …  
    765770        element->setHasFocusWithin(flag);
    766771
    767     // Elements that support keyboard input (form inputs and contenteditable) always match :focus-visible when focused.
    768     setHasFocusVisible(flag && (visibility == FocusVisibility::Visible || isTextField() || isContentEditable()));
     772    setHasFocusVisible(flag && (visibility == FocusVisibility::Visible || shouldAlwaysHaveFocusVisibleWhenFocused(*this)));
    769773}
    770774
    … …  
    773777    if (!document().settings().focusVisibleEnabled())
    774778        return;
     779
     780#if ASSERT_ENABLED
     781    ASSERT(!flag || focused());
     782    ASSERT(!focused() || !shouldAlwaysHaveFocusVisibleWhenFocused(*this) || flag);
     783#endif
    775784
    776785    if (hasFocusVisible() == flag)

  • trunk/Source/WebCore/dom/Node.cpp

    r275859 r276698  
    733733}
    734734
    735 bool Node::isContentEditable()
     735bool Node::isContentEditable() const
    736736{
    737737    return computeEditability(UserSelectAllDoesNotAffectEditability, ShouldUpdateStyle::Update) != Editability::ReadOnly;
    738738}
    739739
    740 bool Node::isContentRichlyEditable()
     740bool Node::isContentRichlyEditable() const
    741741{
    742742    return computeEditability(UserSelectAllIsAlwaysNonEditable, ShouldUpdateStyle::Update) == Editability::CanEditRichly;

  • trunk/Source/WebCore/dom/Node.h

    r276332 r276698  
    318318    void setHasEventTargetData(bool flag) { setNodeFlag(NodeFlag::HasEventTargetData, flag); }
    319319
    320     WEBCORE_EXPORT bool isContentEditable();
    321     bool isContentRichlyEditable();
     320    WEBCORE_EXPORT bool isContentEditable() const;
     321    bool isContentRichlyEditable() const;
    322322
    323323    WEBCORE_EXPORT void inspect();

  • trunk/Source/WebCore/page/EventHandler.cpp

    r276628 r276698  
    35443544    keydown->setTarget(element);
    35453545
    3546     auto shouldMatchFocusVisible = [initialKeyEvent, keydown](const Element& element) {
    3547         if (!element.focused())
    3548             return false;
    3549 
     3546    auto setHasFocusVisibleIfNeeded = [initialKeyEvent, keydown](Element& element) {
    35503547        // If the user interacts with the page via the keyboard, the currently focused element should match :focus-visible.
    35513548        // Just typing a modifier key is not considered user interaction with the page, but Shift + a (or Caps Lock + a) is considered an interaction.
    3552         return keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
     3549        bool userHasInteractedViaKeyword = keydown->modifierKeys().isEmpty() || ((keydown->shiftKey() || keydown->capsLockKey()) && !initialKeyEvent.text().isEmpty());
     3550
     3551        if (element.focused() && userHasInteractedViaKeyword)
     3552            element.setHasFocusVisible(true);
    35533553    };
    3554     // FIXME: This is wrong for text form controls and contenteditable elements (https://webkit.org/b/225075).
    3555     element->setHasFocusVisible(shouldMatchFocusVisible(*element));
     3554    setHasFocusVisibleIfNeeded(*element);
    35563555
    35573556    if (initialKeyEvent.type() == PlatformEvent::RawKeyDown) {
    … …  
    36003599    // But if we are dispatching a fake backward compatibility keypress, then we pretend that the keypress happened on the original element.
    36013600    if (!keydownResult) {
    3602         element->setHasFocusVisible(false);
    36033601        element = eventTargetElementForDocument(m_frame.document());
    36043602        if (!element)
    36053603            return false;
    3606         // FIXME: This is wrong for text form controls and contenteditable elements (https://webkit.org/b/225075).
    3607         element->setHasFocusVisible(shouldMatchFocusVisible(*element));
     3604        setHasFocusVisibleIfNeeded(*element);
    36083605    }
    36093606
Note: See TracChangeset for help on using the changeset viewer.