Implement <dialog> focusing steps

• Component: DOM
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

https://html.spec.whatwg.org/multipage/interactive-elements.html#dialog-focusing-steps

https://html.spec.whatwg.org/multipage/interaction.html#focusing-steps

[Radar WebKit Bug Importer]

<rdar://problem/80013845>

[Tim Nguyen (:ntim)]

Created attachment 433800 [details]
Patch

[Sam Weinig]

Comment on attachment 433800 [details]
Patch

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

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

This should likely be a RefPtr, since I would bet control->focus() can cause script to run and this to potentially be deallocated (but even if not, it is probably worth it).

You should also make sure we have a test for that.

> Source/WebCore/html/HTMLDialogElement.h:62
> +    Element* m_previouslyFocusedElement;

What is keeping this alive?

[Sam Weinig]

Comment on attachment 433800 [details]
Patch

r- since I think this will have lifetime issues.

[Ryosuke Niwa]

Comment on attachment 433800 [details]
Patch

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

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

This seems wrong with respect to the shadow DOM (i.e. spec bug).
This will mean we'd skip any element that's focusable inside a shadow tree unless the outer most host is also focusable.

> Source/WebCore/html/HTMLDialogElement.cpp:141
> +        // If there isn't one, then let control be the first non-inert descendant element of subject, in tree order.

Ditto.

> Source/WebCore/html/HTMLDialogElement.cpp:151
> +    // 3. Run the focusing steps for control.
> +    control->focus();

This isn't right. In WebKit, whether an element is focusable or not depends on the event (mouse/keyword) and its specifics.

>> Source/WebCore/html/HTMLDialogElement.h:62
>> +    Element* m_previouslyFocusedElement;
> 
> What is keeping this alive?

Yeah, this will result in UAF.

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

We shouldn't be setting this flag until right when we're about to focus this element in the callback below.

[Tim Nguyen (:ntim)]

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 433800 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433800&action=review
> 
> > Source/WebCore/html/HTMLDialogElement.cpp:131
> > +    for (Element* element = ElementTraversal::firstWithin(*this); element; element = ElementTraversal::next(*element)) {
> 
> This seems wrong with respect to the shadow DOM (i.e. spec bug).
> This will mean we'd skip any element that's focusable inside a shadow tree
> unless the outer most host is also focusable.
> 
> > Source/WebCore/html/HTMLDialogElement.cpp:141
> > +        // If there isn't one, then let control be the first non-inert descendant element of subject, in tree order.
> 
> Ditto.

This should be addressed later, seems like a minor adjustment. I'd really like to get the main bits landed. Maybe a spec issue should be filed?

> > Source/WebCore/html/HTMLDialogElement.cpp:151
> > +    // 3. Run the focusing steps for control.
> > +    control->focus();
> 
> This isn't right. In WebKit, whether an element is focusable or not depends
> on the event (mouse/keyword) and its specifics.

Do you have a suggestion on how to fix this?

> >> Source/WebCore/html/HTMLDialogElement.h:62
> >> +    Element* m_previouslyFocusedElement;
> > 
> > What is keeping this alive?
> 
> Yeah, this will result in UAF.

Made this a weak pointer.

> > Source/WebCore/html/HTMLFormControlElement.cpp:251
> > -        setAutofocused();
> > +        document().topDocument().setAutofocusProcessed();
> 
> We shouldn't be setting this flag until right when we're about to focus this
> element in the callback below.

Done, thanks for catching!

[Tim Nguyen (:ntim)]

Created attachment 433841 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 433863 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 433863 [details]
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-flag
> +    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.

[Tim Nguyen (:ntim)]

Created attachment 440840 [details]
Patch

[Chris Dumez]

Comment on attachment 440840 [details]
Patch

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

> Source/WebCore/dom/Document.h:793
> +    Deque<WeakPtr<Element>> autofocusCandidates() const { return m_autofocusCandidates; }

Do you really mean to copy the Deque here?

> Source/WebCore/html/HTMLDialogElement.cpp:149
> +    topDocument->autofocusCandidates().clear();

This is currently a no-op.

[Chris Dumez]

Comment on attachment 440840 [details]
Patch

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

>> Source/WebCore/html/HTMLDialogElement.cpp:149
>> +    topDocument->autofocusCandidates().clear();
> 
> This is currently a no-op.

You likely want to add a clearAutofocusCandidates() to Document.

[Chris Dumez]

Comment on attachment 440840 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.cpp:145
> +    if (!control->document().securityOrigin().isSameOriginAs(control->document().topOrigin()))

if (!control->document().isSameOriginAsTopDocument())

seems simpler.

[Tim Nguyen (:ntim)]

Created attachment 440845 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440893 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 440934 [details]
Patch

[Darin Adler]

Comment on attachment 440934 [details]
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-focusing-steps

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

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #17)
> Comment on attachment 440934 [details]
> 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.

Thanks for the review!

> > 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?

Good call, thanks.

> 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.

The behavior is consistent to hiding the container of a focused element, e.g.:

```
<div id="div">
  <button id="button">hi</button>
</div>
<script>
button.focus();

div.hidden = true;

console.log(document.activeElement.tagName) // returns BUTTON on every browser
</script>
```

I guess that slightly feels wrong that a hidden element is focused, but that's something that should probably be fixed in all cases (or maybe there's a good rationale for it, not sure?), not just <dialog>, with tests/spec changes in hand.

There's also this test for <dialog> which tests a very similar case: https://webkit-search.igalia.com/webkit/rev/09c09799bad5710182ea8d6008d5f5ca5213916c/LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-after-close.html#59-74


> Shouldn’t this also be clearing out m_previouslyFocusedElement?

Yes, it's part of the spec, thanks for spotting this!

[Tim Nguyen (:ntim)]

Created attachment 441113 [details]
Patch

[Tim Nguyen (:ntim)]

Committed r284116 (242936@main): <https://commits.webkit.org/242936@main>

[Darin Adler]

(In reply to Tim Nguyen (:ntim) from comment #18)
> I guess that slightly feels wrong that a hidden element is focused, but
> that's something that should probably be fixed in all cases (or maybe
> there's a good rationale for it, not sure?), not just <dialog>, with
> tests/spec changes in hand.

I really don’t care too much about what "feels wrong". I do care a lot about cases we haven’t explicitly tested, since differences in such cases often lead to real world interoperability problems. To me it seems that to be confident that the isConnected() check is both necessary and nothing else is necessary we need many tests, including ones where the element is still connected but is elsewhere in the document, cases that you mentioned about "hidden" just to verify they have no effect, and more.

Otherwise, how can we verify that isConnected() is exactly the right thing for the code to be checking?

> There's also this test for <dialog> which tests a very similar case:
> https://webkit-search.igalia.com/webkit/rev/
> 09c09799bad5710182ea8d6008d5f5ca5213916c/LayoutTests/imported/w3c/web-
> platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-
> after-close.html#59-74

Were we failing this test when our patch didn’t include the isConnected check?

[Darin Adler]

Comment on attachment 441113 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.cpp:124
> +    if (renderer() && renderer()->style().effectiveInert())
> +        return;

Occurs to me that checking for a renderer without explicitly updating layout to build the render tree first gives unpredictable results for possibly-newly-created DOM elements. Where is the code that causes the render tree to get built? Do we have test cases that cover this edge case?

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #22)
> Comment on attachment 441113 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441113&action=review
> 
> > Source/WebCore/html/HTMLDialogElement.cpp:124
> > +    if (renderer() && renderer()->style().effectiveInert())
> > +        return;
> 
> Occurs to me that checking for a renderer without explicitly updating layout
> to build the render tree first gives unpredictable results for
> possibly-newly-created DOM elements. Where is the code that causes the
> render tree to get built? Do we have test cases that cover this edge case?

This is called by show/showModal which toggle the open attribute, this toggles display: none; so this should trigger layout.

> I really don’t care too much about what "feels wrong". I do care a lot about cases we haven’t explicitly tested, since differences in such cases often lead to real world interoperability problems. To me it seems that to be confident that the isConnected() check is both necessary and nothing else is necessary we need many tests, including ones where the element is still connected but is elsewhere in the document, cases that you mentioned about "hidden" just to verify they have no effect, and more.

> Otherwise, how can we verify that isConnected() is exactly the right thing for the code to be checking?

I realize the `isConnected()` check doesn't actually make a difference, the tests in fact pass without. 

Let's say the previously focused node isn't connected, then if you call focus() on it, it will be no-op, see:

https://webkit-search.igalia.com/webkit/rev/09c09799bad5710182ea8d6008d5f5ca5213916c/Source/WebCore/dom/Element.cpp#3068-3069

If you remove the `isConnected()` check, it's also no-op for disconnected element, so we could do either way.

---

Regarding extra test coverage in the case there is no previously focused element, it will just follow rules what happens when a focused element gets hidden, I don't think this is necessary to test for <dialog> specifically, unless the spec adds special rules for it. This should be covered by normal focus tests.

[Darin Adler]

Comment on attachment 441113 [details]
Patch

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

>>> Source/WebCore/html/HTMLDialogElement.cpp:124
>>> +        return;
>> 
>> Occurs to me that checking for a renderer without explicitly updating layout to build the render tree first gives unpredictable results for possibly-newly-created DOM elements. Where is the code that causes the render tree to get built? Do we have test cases that cover this edge case?
> 
> This is called by show/showModal which toggle the open attribute, this toggles display: none; so this should trigger layout.

Changing style does not trigger layout. It triggers the need for layout, which is then done lazily at a later time. Thus I am even more concerned that this code won’t work properly and we need to make sure we’ve tested it carefully.

[Darin Adler]

(In reply to Tim Nguyen (:ntim) from comment #23)
> I realize the `isConnected()` check doesn't actually make a difference, the
> tests in fact pass without.

Got it. Let’s remove the isConnected check, then.

> Regarding extra test coverage in the case there is no previously focused
> element, it will just follow rules what happens when a focused element gets
> hidden, I don't think this is necessary to test for <dialog> specifically,
> unless the spec adds special rules for it. This should be covered by normal
> focus tests.

I am not sure I asked for extra test coverage of the case where there is no previously focused element. I am asking for extra test coverage in the case where the previously focused element is put in an usual state, like moved into a shadow tree, removed from the document, moved to a different place in the document, etc.

[Simon Fraser (smfr)]

Comment on attachment 441113 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.cpp:53
>      setBooleanAttribute(openAttr, true);

This can affect style...

> Source/WebCore/html/HTMLDialogElement.cpp:57
> +    runFocusingSteps();

So we have to update style and layout before running these steps.

> Source/WebCore/html/HTMLDialogElement.cpp:75
>          addToTopLayer();

This can affect style...

> Source/WebCore/html/HTMLDialogElement.cpp:79
> +    runFocusingSteps();

So we have to update style and layout before running these steps.

[Darin Adler]

And we should add test cases that would fail because of these mistakes.

[Simon Fraser (smfr)]

Reopen to address remaining issues.

[Tim Nguyen (:ntim)]

> I am not sure I asked for extra test coverage of the case where there is no previously focused element. I am asking for extra test coverage in the case where the previously focused element is put in an usual state, like moved into a shadow tree, removed from the document, moved to a different place in the document, etc.

"removed from the document" falls into the same category as "there is no previously focused element".

From https://html.spec.whatwg.org/multipage/interactive-elements.html#dom-dialog-close:

> If subject's previously focused element is not null, then: [...]

If it's removed from the document, then the previously focused element will be null, and the usual focus rules follow. (I've tested this manually yesterday). This isn't a thing that should be tested by <dialog> related tests. focus-after-close.html tests a similar case where the element is removed after (not before) close() is called.

moved into a shadow tree/moved to a different place in the document seem like interesting cases to test, I'll extend the following test in a WPT PR:

https://webkit-search.igalia.com/webkit/source/LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-after-close.html

> And we should add test cases that would fail because of these mistakes.

I'll add a testcase for focusing steps on an inert <dialog> in bug 231721. Probably will use <dialog inert> here, rather than opening a second modal dialog, since focus will be moved to the second dialog otherwise, which retracts the whole point of the test.

>  Reopen to address remaining issues.

I filed followups (which I'll address today), bug 231721 and bug 231722. Marked as this bug as resolved since the patch hasn't been reverted.

Thanks for the great feedback!

[Tim Nguyen (:ntim)]

Bug 231721, bug 231736, bug 231722 and bug 231721 should address everything mentioned here.