[webkit-reviews] review granted: [Bug 224598] [selectors] Script focus and :focus-visible : [Attachment 426209] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 224598: [selectors] Script focus and :focus-visible
https://bugs.webkit.org/show_bug.cgi?id=224598

Attachment 426209: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 426209
  --> https://bugs.webkit.org/attachment.cgi?id=426209
Patch

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

> Source/WebCore/dom/Document.h:764
> +    bool lastFocusByMouseClick() const { return m_lastFocusByMouseClick; }

This name isn’t linguistically sound. The phrase "last focus by mouse click"
sounds like it would be a "focus", not a question with a "yes/no" answer. And
the use of "mouse" here is strangely old fashioned. I assume this includes taps
on touch screens and clicks on trackpads, so being too explicit here about
"mouse" seems wrong. Unless it’s helpful to use that term because that’s what’s
done in some specification.

I suggest the name wasLastFocusByClick or focusedElementWasFocusedByClick, but
maybe you can come up with one even better. Another option would be to store
the FocusTrigger value instead of using a boolean. Since you invented this
FocusTrigger concept, why not use it?

At some point we might move both the focused element and this new flag into a
separate focus object owned by the document, perhaps along with some other
focus and keyboard navigation related things; trying to avoid this trend of
Document itself becoming a "god class".

> Source/WebCore/dom/Document.h:2130
> +    bool m_lastFocusByMouseClick { false };

Ditto.

> Source/WebCore/dom/Element.cpp:3088
> +	       newTarget->setHasFocusVisible(false);

Do we want to do this unconditionally, or only if we called
setHasFocusVisible(true) explicitly above?

> Source/WebCore/dom/FocusOptions.h:35
> +enum class FocusTrigger { Other, Click };

enum class FocusTrigger : bool

Could do the same for FocusRemovalEventsMode above, too. It’s good to specify
an underlying type so we don’t use more memory than necessary to store these
integers.

> Source/WebCore/dom/FocusOptions.h:41
> +    FocusTrigger trigger { FocusTrigger::Other };

I think FocusOptions may be the C++ representation of an IDL class from the
event specification. If so, I’m not sure we should be adding this FocusTrigger
concept. We try to keep things more aligned without adding invisible state like
that.

> Source/WebCore/page/EventHandler.cpp:2726
> +    if (page && !page->focusController().setFocusedElement(element.get(),
m_frame, { { }, { }, { }, FocusTrigger::Click, { } }))

I’m not sure this is the only place this is needed. The pointer events web API
may have introduced additional paths that handle "clicks"?

> LayoutTests/platform/mac/TestExpectations:2294
> +# Buttons are not focusable on Mac so this test doesn't work as expected.

We need to take this up with the others who are making the web standards and
the web platform tests. We don’t want a standard that prohibits web browsers
from following Apple’s platform standards for the Mac user interface, with
Safari and WebKit willfully violating that standard.