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

Treat focusable as enumerated boolean SVG attribute #13339

Merged
merged 2 commits into from
Aug 7, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 7, 2018

Fixes #12481.

It's a small behavior change but the old behavior both had a warning (in which case we don't provide strong guarantees) and was inconsistent with other similar attributes. The old behavior was also unintuitive.

Particularly, this relaxes the restriction on <svg focusable> to accept booleans. It's an enumerated attribute with 'true', 'false', and 'auto' values. This makes it consistent with how we already handle the HTML draggable attribute (which also has 'true', 'false', and 'auto' values).

Before:

screen shot 2018-08-07 at 10 07 58 am

screen shot 2018-08-07 at 10 08 19 am

After:

screen shot 2018-08-07 at 10 09 06 am

screen shot 2018-08-07 at 10 08 58 am

There's no behavior change for cases that didn't warn.

I couldn't find other attributes like this except syncMaster (which seems very niche?) so I'm not worried about this list growing.

@gaearon gaearon changed the title Treat focusable as enumerated boolean attribute Treat focusable as enumerated boolean SVG attribute Aug 7, 2018
Copy link

@KittyGiraudel KittyGiraudel left a comment

Choose a reason for hiding this comment

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

Yay! 🎉

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Does the attribute table need to be updated?

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Aug 7, 2018

What does this change do to the attribute table? Particularly, we should confirm that any change came with a warning before this change.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 7, 2018

Yep. Pushed.

@gaearon gaearon merged commit 3cfab14 into facebook:master Aug 7, 2018
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants