-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement dialog initial focus proposal #8199
Conversation
This implements the changes proposed here: https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal#dialog-draft-text Specifically: 1. Add a parameter to dialog.show() called preventInitialFocus, which prevents the dialog focusing steps from running. 2. Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. There are additional open issues around dialog initial focus listed here: whatwg#4184 (comment) TODO add a conformance requirement about autofocus: whatwg#7709 TODO consider adding a <p class=XXX> for tab trapping: whatwg#7707
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
This does not implement the change where calling
showModal()
on<dialog autofocus>
will cause the dialog itself to get focus. Probably the best way to spec that is to change theshowModal()
spec to check for the autofocus attribute and, if present, focus the dialog instead of going into the focus delegate steps. -
A big part of the proposal was writing up https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal#improving-conformance-requirements-and-examples as spec text, to replace the current single sentence "The dialog element represents a part of an application that a user interacts with to perform a task, for example a dialog box, inspector, or window."
Ah thanks, I totally missed this. It is now in the PR |
I added a commit to do this. Instead of doing it all in showModal I changed the dialog focusing steps because I figured that it would still be important to run the other autofocus related steps at the end of the dialog focusing steps |
application, or manually closed by the user.</p> | ||
|
||
<p>Especially for modal dialogs, which are a familiar pattern across all types of applications, | ||
authors should work to ensure that dialogs in their web applications behave in a way that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be non-normative? If so, don't use "should".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended this to be normative when I wrote it. Do you think it's a bad normative requirement, e.g. because it's too vague? I'm not sure on the precedent there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Yes, I think it's not a good requirement because it's vague and not testable. Further, the wording "authors should work to ensure" makes the requirement about the work the author does, which is a bit off.
If it's not testable, I think making it non-normative is better. e.g. s/should work/are encouraged/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any remaining normative text in the section we're talking about, so I think this is resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is still there and still says "authors should work to ensure".
Perhaps we could just change it to "authors are encouraged"?
application, or manually closed by the user.</p> | ||
|
||
<p>Especially for modal dialogs, which are a familiar pattern across all types of applications, | ||
authors should work to ensure that dialogs in their web applications behave in a way that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended this to be normative when I wrote it. Do you think it's a bad normative requirement, e.g. because it's too vague? I'm not sure on the precedent there.
Thanks for the review @domenic! I believe I've addressed all your comments |
This reverts commit 7585ed6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with merging this.
<p>In the following example, a dialog is used for editing the details of a product in an | ||
inventory management web application.</p> | ||
|
||
<pre><code class="html"><dialog> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, fair enough.
This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea
This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024}
This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024}
This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024}
… a=testonly Automatic update from web-platform-tests Implement dialog initial focus proposal This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024} -- wpt-commits: 95f9b15d9aad3354f440e4e4cced5ccc405b0069 wpt-pr: 36831
… a=testonly Automatic update from web-platform-tests Implement dialog initial focus proposal This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024} -- wpt-commits: 95f9b15d9aad3354f440e4e4cced5ccc405b0069 wpt-pr: 36831
This has been discussed here: whatwg/html#4184 whatwg/html#8199 The gist of the changes are: 1. Make the dialog focusing steps look at keyboard focusable elements instead of any focusable element. 2. Make the dialog element itself get focus if it has the autofocus attribute set. 3. Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. This patch also adds "outline:none" to several WPTs because this patch causes the dialog element to become initially focused in some cases and get a focus ring. I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/CEL3wWHrTAQ Fixed: 1193482 Change-Id: I1fee5981f72039a4467cbb35b2317832dd31bbea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3984630 Commit-Queue: Joey Arhar <[email protected]> Reviewed-by: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/main@{#1100024}
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 UltraBlame original commit: cbf5ea1b17d2b0fbea7c1849ab2a1902396ad291
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 UltraBlame original commit: cbf5ea1b17d2b0fbea7c1849ab2a1902396ad291
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 UltraBlame original commit: cbf5ea1b17d2b0fbea7c1849ab2a1902396ad291
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1811129 gecko-commit: 94d3ff1570f4ef28436d580200a642fc1ab41883 gecko-reviewers: emilio
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1811129 gecko-commit: 94d3ff1570f4ef28436d580200a642fc1ab41883 gecko-reviewers: emilio
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263
The main changes of the new algorithm are * Make the dialog focusing steps look at sequentially focusable elements instead of any focusable element. * Make the dialog element itself get focus if it has the autofocus attribute set. * Make the dialog element itself get focus as a fallback instead of focus being "reset" to the body element. Spec PR (merged): whatwg/html#8199 Differential Revision: https://phabricator.services.mozilla.com/D181263 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1811129 gecko-commit: 94d3ff1570f4ef28436d580200a642fc1ab41883 gecko-reviewers: emilio
https://bugs.webkit.org/show_bug.cgi?id=250795 Reviewed by NOBODY (OOPS!). Updates the dialog focusing steps and the focus delegate steps to match latest spec. Spec PR: whatwg/html#8199 * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/child-sequential-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-previous-iframe.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/show-modal-focusing-steps-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-focus-expected.txt: * Source/WebCore/dom/Element.cpp: (WebCore::Element::findFocusDelegateForTarget): * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::runFocusingSteps): (WebCore::HTMLDialogElement::supportsFocus const): * Source/WebCore/html/HTMLDialogElement.h:
https://bugs.webkit.org/show_bug.cgi?id=250795 Reviewed by NOBODY (OOPS!). Updates the dialog focusing steps and the focus delegate steps to match latest spec. Spec PR: whatwg/html#8199 * LayoutTests/fast/layers/layer-order-after-top-layer.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/child-sequential-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-previous-iframe.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/show-modal-focusing-steps-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-dialog-initial-focus-expected.txt: Removed. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-dialog-initial-focus.html: Removed. These tests no longer exist in upstream WPT repo. * Source/WebCore/dom/Element.cpp: (WebCore::Element::findFocusDelegateForTarget): * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::runFocusingSteps): (WebCore::HTMLDialogElement::supportsFocus const): * Source/WebCore/html/HTMLDialogElement.h:
https://bugs.webkit.org/show_bug.cgi?id=250795 Reviewed by Tim Nguyen. Updates the dialog focusing steps and the focus delegate steps to match latest spec. Spec PR: whatwg/html#8199 * LayoutTests/fast/layers/layer-order-after-top-layer.html: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/child-sequential-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focus-shadow-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/focus-previous-iframe.tentative-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/show-modal-focusing-steps-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-focus-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-dialog-initial-focus-expected.txt: Removed. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-dialog-initial-focus.html: Removed. These tests no longer exist in upstream WPT repo. * Source/WebCore/dom/Element.cpp: (WebCore::Element::findFocusDelegateForTarget): * Source/WebCore/html/HTMLDialogElement.cpp: (WebCore::HTMLDialogElement::runFocusingSteps): (WebCore::HTMLDialogElement::supportsFocus const): * Source/WebCore/html/HTMLDialogElement.h: Canonical link: https://commits.webkit.org/281215@main
This implements the changes proposed here:
https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal#dialog-draft-text
Specifically:
autofocus
attribute set.There are additional open issues around dialog initial focus listed here:
#4184 (comment)
(See WHATWG Working Mode: Changes for more details.)
/interaction.html ( diff )
/interactive-elements.html ( diff )