Skip to content
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

Focus the dialog by default #6

Merged
merged 1 commit into from
Feb 9, 2017
Merged

Focus the dialog by default #6

merged 1 commit into from
Feb 9, 2017

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Feb 8, 2017

Implements the proposed spec changes discussed in whatwg/html#1929 to focus the dialog element itself, rather than the first focusable child (unless there is a child with the autofocus attribute).

A summary of that WHATWG bug is that screenreaders and other assistive technologies give much more useful output when the dialog itself is focus on opening, and the automatic focus led to unexpected results when combined with tabindex.

In our case, this will fix the problems we've had with outlines and backgrounds showing on dialog close buttons due to automatic focusing.

I'm not entirely clear whether this should be merged ahead of the spec solidifying, but it does solve a real bug in our case.

@@ -75,6 +59,10 @@ angular.module('ayDialog', [])
' -webkit-overflow-scrolling: touch;',
'}',
'',
'dialog:focus {',
' outline: 0 none;',
Copy link
Member Author

Choose a reason for hiding this comment

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

@svachon-ayogo This would also need to be added to our reset stylesheet for Chrome (which supports dialog natively and doesn't get these styles injected)

Copy link
Contributor

@admvx admvx left a comment

Choose a reason for hiding this comment

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

👍 I think, between its short-term utility for us, and the good reasoning behind the spec change, it would make sense to merge this now IMO.

Copy link

@svachon-ayogo svachon-ayogo left a comment

Choose a reason for hiding this comment

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

👍 Agreed!

@isuda isuda merged commit 5847665 into master Feb 9, 2017
@isuda isuda deleted the better-focus branch February 9, 2017 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants