Only restore dialog focus if focus is in the dialog #9178

josepharhar · 2023-04-17T20:35:40Z

This patch prevents the behavior where closing a dialog focuses the previously focused element from before the dialog was opened only if focus is in the dialog when the dialog closes.

Without this, focus can unexpectedly shift away from an element which is not going away, like a text input that the user is currently typing into for example.

At least two implementers are interested (and none opposed):
- Chrome
- WebKit
- Firefox?
Tests are written and can be reviewed and commented upon at:
- Only restore dialog focus if focus is in the dialog web-platform-tests/wpt#39579
Implementation bugs are filed:
- Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4436533
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1832869
- WebKit: https://commits.webkit.org/263645@main

(See WHATWG Working Mode: Changes for more details.)

/interactive-elements.html ( diff )

Fixes whatwg#8904 This patch prevents the behavior where closing a dialog focuses the previously focused element from before the dialog was opened only if focus is in the dialog when the dialog closes. Without this, focus can unexpectedly shift away from an element which is not going away, like a text input that the user is currently typing into for example.

nt1m · 2023-04-17T21:08:17Z

source

+     <li><p>If <var>subject</var>'s <span>node document</span>'s <span>focused area of the
+     document</span>'s <span>DOM anchor</span> is a <span>shadow-including inclusive
+     descendant</span> of <var>element</var>, then run the <span>focusing steps</span> for
+     <var>element</var>; the viewport should not be scrolled by doing this step.</p></li>


I think we should run the focusing steps unconditionally for modal dialogs, since they make the whole page inert (so focus can never properly be outside of the dialog)

If focus can never be outside of the dialog, do you think it should be an assert?

There is one case where focus can be outside the modal dialog, which is the one where the body is focused (which means no focused element). This can be asserted though, focus is on body or within the dialog. I'm not sure it would be super useful however.

But in the case the focus is on the body (e.g. no focused element) in the modal dialog case, I think we'd want to restore focus as well. It might mean that there was no element to focus within the dialog in the first place.

Ah ok I'll just add a modal check

Oh yeah we could unconditionally restore focus when the body element is focused...
The bug report definitely doesn't have the body element focused

I would slightly prefer a modal check, the body element could be focused in the non-modal case simply because the user tried to interact with something outside the dialog. In the modal case, that intent isn't really possible due to the backdrop and the inertness.

Ok I just pushed a modal check, how does it look?

But in the case the focus is on the body (e.g. no focused element) in the modal dialog case, I think we'd want to restore focus as well. It might mean that there was no element to focus within the dialog in the first place.

In this case, the dialog element is focused - I changed this behavior somewhat recently but I don't fully remember what the context was, I can dig it up if you'd like.

However, what if you had multiple modal dialogs open?
If you open one modal dialog and then a second, should closing the first modal dialog attempt to restore focus?

annevk · 2023-04-20T14:31:24Z

Should we merge this into #9018 to make it clearer it's a holistic change to equivalent features? @domenic what do you think?

domenic · 2023-04-26T05:13:51Z

I am OK either way; small atomic commits with separate tests and implementation bugs is reasonable, but so is keeping things together. I'd generally leave it up to @josepharhar.

domenic

Editorially LGTM, but given @nt1m's previous review I'd love his explicit signoff too (which should also serve for WebKit implementer interest).

nt1m · 2023-05-12T15:37:20Z

This was implemented in WebKit in https://commits.webkit.org/263645@main

annevk · 2023-05-12T16:51:41Z

@josepharhar could you get the tests merged? And maybe file a bug against Gecko?

josepharhar · 2023-05-12T17:06:24Z

WPTs should be merged shortly. Here is a gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1832869

nt1m · 2023-06-05T21:27:27Z

@josepharhar I just noticed a small bug here on the spec, the "is modal" check will never be true, since we set is modal to false beforehand. We need to store whether the dialog was modal in a variable and use that.

https://bugs.webkit.org/show_bug.cgi?id=256717 rdar://109572320 Reviewed by NOBODY (OOPS!). Implement whatwg/html#9178 to avoid unexpected focus shifting when focus is already outside of the dialog. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-previous-outside-expected.txt: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::close): (WebCore::HTMLDialogElement::runFocusingSteps):

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment)

josepharhar · 2023-06-06T21:19:24Z

I just noticed a small bug here on the spec, the "is modal" check will never be true, since we set is modal to false beforehand. We need to store whether the dialog was modal in a variable and use that.

I opened a spec PR to fix this here: #9391

is_modal_ was being checked after the call to SetIsModal(false). This patch caches the value of is_modal_ so that the subsequent modal check actually works as intended. This was pointed out here: whatwg/html#9178 (comment) HTML spec PR: whatwg/html#9178 Change-Id: Ic1bd87f9843bce7016ff672dd73fcc616e41a85c

The value for is modal needs to be cached. This was pointed out here: #9178 (comment).

is_modal_ was being checked after the call to SetIsModal(false). This patch caches the value of is_modal_ so that the subsequent modal check actually works as intended. This was pointed out here: whatwg/html#9178 (comment) HTML spec PR: whatwg/html#9178 Change-Id: Ic1bd87f9843bce7016ff672dd73fcc616e41a85c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4596156 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1159722}

…tonly Automatic update from web-platform-tests Fix dialog modal check in Close() is_modal_ was being checked after the call to SetIsModal(false). This patch caches the value of is_modal_ so that the subsequent modal check actually works as intended. This was pointed out here: whatwg/html#9178 (comment) HTML spec PR: whatwg/html#9178 Change-Id: Ic1bd87f9843bce7016ff672dd73fcc616e41a85c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4596156 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1159722} -- wpt-commits: 2f6058492b28367114a9195b97d1e58d8d96fae9 wpt-pr: 40404

…tonly Automatic update from web-platform-tests Fix dialog modal check in Close() is_modal_ was being checked after the call to SetIsModal(false). This patch caches the value of is_modal_ so that the subsequent modal check actually works as intended. This was pointed out here: whatwg/html#9178 (comment) HTML spec PR: whatwg/html#9178 Change-Id: Ic1bd87f9843bce7016ff672dd73fcc616e41a85c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4596156 Reviewed-by: Di Zhang <dizhanggchromium.org> Commit-Queue: Joey Arhar <jarharchromium.org> Cr-Commit-Position: refs/heads/main{#1159722} -- wpt-commits: 2f6058492b28367114a9195b97d1e58d8d96fae9 wpt-pr: 40404 UltraBlame original commit: fa9eb28958aefc0155015024205bdf4c6bb95e0a

…tonly Automatic update from web-platform-tests Fix dialog modal check in Close() is_modal_ was being checked after the call to SetIsModal(false). This patch caches the value of is_modal_ so that the subsequent modal check actually works as intended. This was pointed out here: whatwg/html#9178 (comment) HTML spec PR: whatwg/html#9178 Change-Id: Ic1bd87f9843bce7016ff672dd73fcc616e41a85c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4596156 Reviewed-by: Di Zhang <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/main@{#1159722} -- wpt-commits: 2f6058492b28367114a9195b97d1e58d8d96fae9 wpt-pr: 40404

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

https://bugs.webkit.org/show_bug.cgi?id=256717 rdar://109572320 Reviewed by NOBODY (OOPS!). Implement whatwg/html#9178 to avoid unexpected focus shifting when focus is already outside of the dialog. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-previous-outside-expected.txt: * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::close):

This was referenced Apr 17, 2023

The previous focus change that was applied to dialog close has unexpected behavior for non-modal dialogs #8904

Closed

Only restore popover focus if focus is in the popover #9018

Merged

nt1m added topic: focus topic: dialog The <dialog> element labels Apr 17, 2023

nt1m reviewed Apr 17, 2023

View reviewed changes

always restore focus for modal dialogs

08c05b0

domenic approved these changes Apr 26, 2023

View reviewed changes

nt1m approved these changes May 12, 2023

View reviewed changes

annevk merged commit 1429dac into whatwg:main May 12, 2023

nt1m mentioned this pull request Jun 5, 2023

Only restore dialog focus if focus is in dialog WebKit/WebKit#14683

Draft

josepharhar added a commit to josepharhar/html that referenced this pull request Jun 6, 2023

Fix modal check in dialog closing algorithm

aebd2cf

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment)

josepharhar mentioned this pull request Jun 6, 2023

Fix modal check in dialog closing algorithm #9391

Merged

4 tasks

chromium-wpt-export-bot mentioned this pull request Jun 6, 2023

Fix dialog modal check in Close() web-platform-tests/wpt#40404

Merged

annevk pushed a commit that referenced this pull request Jun 7, 2023

Fix modal check in dialog closing algorithm

d5dba01

The value for is modal needs to be cached. This was pointed out here: #9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 20, 2023

Fix modal check in dialog closing algorithm

dff1658

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 20, 2023

Fix modal check in dialog closing algorithm

1be178c

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Fix modal check in dialog closing algorithm

a39f7b9

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Fix modal check in dialog closing algorithm

4436b86

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Fix modal check in dialog closing algorithm

5d818dd

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Fix modal check in dialog closing algorithm

058c4b5

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Adding final step to mask cross-origin

39e08ff

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 21, 2023

Adding final step to mask cross-origin

83941ea

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 24, 2023

Fix modal check in dialog closing algorithm

332c468

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 24, 2023

Fix modal check in dialog closing algorithm

edc61d6

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 25, 2023

Fix modal check in dialog closing algorithm

14b349d

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 25, 2023

Fix modal check in dialog closing algorithm

1813a04

The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 25, 2023

Adding final step to mask cross-origin

04a95c7

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jul 25, 2023

Adding final step to mask cross-origin

1b2149d

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Sep 5, 2023

Adding final step to mask cross-origin

b875825

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Sep 5, 2023

Adding final step to mask cross-origin

4de4124

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Oct 5, 2023

Adding final step to mask cross-origin

4eeba8d

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Oct 5, 2023

Adding final step to mask cross-origin

3ec0574

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Dec 5, 2023

Adding final step to mask cross-origin

c9df3eb

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Dec 5, 2023

Adding final step to mask cross-origin

b95c628

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

rubberyuzu pushed a commit to rubberyuzu/html that referenced this pull request Jan 12, 2024

Adding final step to mask cross-origin

fcc3449

rebase rebase Fix modal check in dialog closing algorithm The value for is modal needs to be cached. This was pointed out here: whatwg#9178 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only restore dialog focus if focus is in the dialog #9178

Only restore dialog focus if focus is in the dialog #9178

josepharhar commented Apr 17, 2023 •

edited by annevk

Loading

nt1m Apr 17, 2023

josepharhar Apr 18, 2023

nt1m Apr 18, 2023

nt1m Apr 18, 2023

josepharhar Apr 18, 2023

josepharhar Apr 18, 2023

nt1m Apr 18, 2023

josepharhar Apr 18, 2023

josepharhar Apr 18, 2023

annevk commented Apr 20, 2023 •

edited

Loading

domenic commented Apr 26, 2023

domenic left a comment

nt1m commented May 12, 2023

annevk commented May 12, 2023 •

edited

Loading

josepharhar commented May 12, 2023

nt1m commented Jun 5, 2023

josepharhar commented Jun 6, 2023

Only restore dialog focus if focus is in the dialog #9178

Only restore dialog focus if focus is in the dialog #9178

Conversation

josepharhar commented Apr 17, 2023 • edited by annevk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 20, 2023 • edited Loading

domenic commented Apr 26, 2023

domenic left a comment

Choose a reason for hiding this comment

nt1m commented May 12, 2023

annevk commented May 12, 2023 • edited Loading

josepharhar commented May 12, 2023

nt1m commented Jun 5, 2023

josepharhar commented Jun 6, 2023

josepharhar commented Apr 17, 2023 •

edited by annevk

Loading

annevk commented Apr 20, 2023 •

edited

Loading

annevk commented May 12, 2023 •

edited

Loading