[webkit-reviews] review denied: [Bug 227537] Implement <dialog> focusing steps : [Attachment 433863] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 227537: Implement <dialog> focusing steps
https://bugs.webkit.org/show_bug.cgi?id=227537

Attachment 433863: Patch

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




--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 433863
  --> https://bugs.webkit.org/attachment.cgi?id=433863
Patch

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

> Source/WebCore/dom/Document.h:2074
> +    //
https://html.spec.whatwg.org/multipage/interaction.html#autofocus-processed-fla
g
> +    bool m_autofocusProcessed { false };

I don't think we need this link. It's pretty clear what this flag does given
there is the same term defined in the spec.

> Source/WebCore/html/HTMLDialogElement.cpp:103
> +    if (m_previouslyFocusedElement) {
> +	   FocusOptions options;
> +	   options.preventScroll = true;
> +	   m_previouslyFocusedElement->focus(options);
> +    }

This doesn't seem right unless this dialog element (and maybe even this
browsing context) had lost the focus.

> Source/WebCore/html/HTMLDialogElement.cpp:114
> +    Element* control = nullptr;

Use RefPtr as Sam and I have both suggested. r- because of this.

> Source/WebCore/html/HTMLDialogElement.cpp:115
> +    for (Element* element = ElementTraversal::firstWithin(*this); element;
element = ElementTraversal::next(*element)) {

Ditto. Also, I don't think we need to figure out whether we want to do tree
traversal ignoring shadow DOM or not.
I don't think we should be landing this patch without figuring out the
interaction with the shadow DOM.
r- because of this.

> Source/WebCore/html/HTMLDialogElement.cpp:117
> +	   if (!element->isFocusable())
> +	       continue;

This check isn't right either. We need to figure out what sense of focusability
we need here.
The spec doesn't tell us so we need to decide.
We probably need an Event object for keyboard / mouse actions or nullopt if it
was triggered by script.
See Element::focus and Element::isKeyboardFocusable and
Element::isMouseFocusable.

> Source/WebCore/html/HTMLDialogElement.cpp:119
> +	   // Let control be the first descendant element of subject, in tree
order, that is not inert and has the autofocus attribute specified.

Again, we shouldn't be littering our code with all these comments copied &
pasted from the spec verbatim.
It makes the code less readable.

> Source/WebCore/html/HTMLFormControlElement.cpp:256
> +		   element->document().topDocument().setAutofocusProcessed();

Why are we not checking the security origin here?

> Source/WebCore/html/HTMLFormControlElement.cpp:261
> +		   element->document().topDocument().setAutofocusProcessed();

Ditto.