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
Merged

Conversation

@gaearon
Copy link
Member

@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 requested review from sebmarkbage and acdlite Aug 7, 2018
@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

Yay! 🎉

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Does the attribute table need to be updated?

Loading

@sebmarkbage
Copy link
Member

@sebmarkbage 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.

Loading

@gaearon
Copy link
Member Author

@gaearon gaearon commented Aug 7, 2018

Yep. Pushed.

Loading

@gaearon gaearon merged commit 3cfab14 into facebook:master Aug 7, 2018
1 check was pending
Loading
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Treat focusable as enumerated boolean attribute

* Update attribute table
@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants