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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 440934: Patch

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




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

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

Looking pretty good, but I think the tests are not covering enough edge cases.

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

I’m not sure that just checking if m_previouslyFocusedElement is null is a
sufficient check. We probably need to check that it’s still connected to the
document too, or maybe even check that it’s a descendant of some element? Not
100% sure about this, but we should construct some test cases where it’s
removed before close is called to check if the behavior is consistent with
other browsers and thus interoperable.

Shouldn’t this also be clearing out m_previouslyFocusedElement?

Also, our lifetime rules for reference counted objects require putting the
element into a Ref or RefPtr before calling focus; the focus function can
assume the caller is holding a reference to the element it’s called on. So the
code would be more like:

    if (RefPtr element = std::exchange(m_previouslyFocusedElement,
nullptr).get(); element && element->isConnected()) {
	FocusOptions options;
	options.preventScroll = true;
	element->focus(options);
    }

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

The modern idiom for this is:

	for (auto& element : descendantsOfType<Element>(*this)) {

Seems like there is no downside to using that. The code in the loop body would
then use "element." and "&element" instead of "element->" and "element", but
otherwise be the same.

> Source/WebCore/html/HTMLDialogElement.h:50
> +    //
https://html.spec.whatwg.org/multipage/interactive-elements.html#dialog-focusin
g-steps

Typically we put those comments in the .cpp file instead of the header.