[webkit-reviews] review denied: [Bug 222028] [selectors] :focus-visible implementation : [Attachment 421926] Patch for landing

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 222028: [selectors] :focus-visible implementation
https://bugs.webkit.org/show_bug.cgi?id=222028

Attachment 421926: Patch for landing

https://bugs.webkit.org/attachment.cgi?id=421926&action=review




--- Comment #39 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 421926
  --> https://bugs.webkit.org/attachment.cgi?id=421926
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=421926&action=review

> Source/WebCore/dom/Element.cpp:753
> +    if (flag) {
> +	   // Elements that support keyboard input (form inputs and
contenteditable) always match :focus-visible when focused.
> +	   if (isTextField() || isContentEditable())
> +	       setHasFocusVisible(true);
> +    } else
> +	   setHasFocusVisible(false);

I'd use a lambda along the lines of

auto computeHasFocusVisible = [&] {
    if (!flag)
	 return false;
    return isTextField() || isContentEditable();
};
setHasFocusVisible(computeHasFocusVisible()):

> Source/WebCore/dom/Element.h:322
>      bool hovered() const { return isUserActionElement() &&
isUserActionElementHovered(); }
>      bool focused() const { return isUserActionElement() &&
isUserActionElementFocused(); }
>      bool isBeingDragged() const { return isUserActionElement() &&
isUserActionElementDragged(); }
> +    bool hasFocusVisible() const { return
hasNodeFlag(NodeFlag::HasFocusVisible); };

This could use UserActionElementSet similar to focused/hovered. On the other
hand I think that exist because earlier Node flag shortage. We currently have a
bunch free so it would be mostly for consistency. I guess adding a node flag is
fine for now too.

> Source/WebCore/page/FrameView.cpp:2277
> -	   if (anchorElement->isFocusable())
> +	   if (anchorElement->isFocusable()) {
> +	       anchorElement->setHasFocusVisible(true);
>	       document.setFocusedElement(anchorElement.get());
> -	   else {
> +	   } else {
>	       document.setFocusedElement(nullptr);
>	       document.setFocusNavigationStartingNode(anchorElement.get());
>	   }

Why doesn't the other branch clear the flag?

> Source/WebCore/style/ElementRuleCollector.cpp:161
> +    if (matchesFocusVisiblePseudoClass(element()))
> +	  
collectMatchingRulesForList(matchRequest.ruleSet->focusVisiblePseudoClassRules(
), matchRequest);

I don't think you need any of the ElementRuleCollector and RuleSet changes in
this patch. Focus is popular as universal rule *:focus so it has this special
optimization. None of this should be needed for functionality and is better
done separately if needed.