Skip to content
This repository was archived by the owner on Jul 30, 2019. It is now read-only.

Focus the dialog instead of the first item#1331

Merged
chaals merged 5 commits intomasterfrom
pati-gallardo-dialog-focus-773
Apr 17, 2018
Merged

Focus the dialog instead of the first item#1331
chaals merged 5 commits intomasterfrom
pati-gallardo-dialog-focus-773

Conversation

@patricia-gallardo
Copy link
Copy Markdown
Contributor

@patricia-gallardo patricia-gallardo commented Apr 4, 2018

Fix #773

Focus the dialog if there are noe autofocus items available, instead of picking the first focusable item

@chaals
Copy link
Copy Markdown
Collaborator

chaals commented Apr 4, 2018

Needs to be tested - this is based on the agreement in #773 (and likewise in related whatwg/html#1929) and predecessors from bugzilla

@LJWatson
Copy link
Copy Markdown
Collaborator

LJWatson commented Apr 16, 2018 •
edited
Loading

Test case from MDN, and support data from CIU

Test: when a <dialog> element is opened and the dialog does not contain a field with autofocus enabled, is keyboard focus placed on the <dialog> element itself?

Chrome desktop/mobile: no
Edge: dialog not supported
Firefox: ? (supported behind a flag)
IE: dialog not supported
Opera Mini: dialog not supported
Safari desktop/mobile: dialog not supported
Samsung Internet: no
UC Browser: ?

@patricia-gallardo
Copy link
Copy Markdown
Contributor Author

patricia-gallardo commented Apr 16, 2018

Tested on Linux:
Chromium (Chrome/Vivaldi/Opera) all focus the dropdown
Firefox (after enabling in about:config dom.dialog_element.enabled) does not focus anything
I have not tested in Webkit (no mac) or Edge (no windows)
Both Webkit and Edge have issues open around the dialog element, however the support for the element still seems to be in its early stages.

@patricia-gallardo patricia-gallardo mentioned this pull request Apr 16, 2018
Closed
chaals
chaals approved these changes Apr 17, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@chaals chaals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Note that the change is at risk because there is only one implementation at the moment.

@chaals chaals merged commit 2a10df5 into master Apr 17, 2018
plehegar pushed a commit that referenced this pull request Apr 17, 2018
Built by Travis-CI: 2a10df5 Focus the dialog instead of the first item (
a84f12f 
#1331)
@patricia-gallardo patricia-gallardo deleted the pati-gallardo-dialog-focus-773 branch April 25, 2018 09:14
@dpogue dpogue mentioned this pull request Nov 20, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@edent edent Awaiting requested review from edent

1 more reviewer

@chaals chaals chaals approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

at risk needs tests not ready for merge substantive

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@patricia-gallardo @chaals @LJWatson