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

Fix tabIndex attribute for SVG #11033

Merged
merged 2 commits into from
Oct 2, 2017
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 2, 2017

Fixes #10987 which is a regression in 16.

Verified with a new integration test, and by manually trying https://github.com/forWorkAtML/svg-bug.

Why it broke: we removed most of the DOM whitelist recently (#10385), and tabIndex was one of the attributes that got nixed because its lowercase version (tabindex) "just works". However, we forgot it’s also a valid SVG attribute which is case sensitive.

We currently use a different logic branch for attributes that aren't in the whitelist, so it didn't get properly lowercased in 16.0.0. Adding it back to the whitelist forces it to be lowercased, which fixes it for SVG (and doesn't change how it works for HTML).

The way whitelist works is pretty confusing now but I'm going to fix this later in #10805. Let's just patch this hole for now.

I don't expect there to be more regressions like this because it's the only camelCase HTML attribute we deleted in #10385 that also happens to apply to SVG.

@reactjs-bot
Copy link

reactjs-bot commented Oct 2, 2017

Deploy preview ready!

Built with commit c079727

https://deploy-preview-11033--reactjs.netlify.com

@@ -10768,7 +10768,7 @@
| `tabIndex=(string 'false')`| (initial)| `<number: -1>` |
| `tabIndex=(string 'on')`| (initial)| `<number: -1>` |
| `tabIndex=(string 'off')`| (initial)| `<number: -1>` |
| `tabIndex=(symbol)`| (initial, warning)| `<number: -1>` |
| `tabIndex=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<number: -1>` |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ssr error, ssr mismatch for Symbols doesn’t sound great, but it’s an existing issue with SVG attributes (you can search the file for many more matches). We should fix that separately.

@@ -10781,19 +10781,19 @@
| `tabIndex=(array with string)`| (initial)| `<number: -1>` |
| `tabIndex=(empty array)`| (initial)| `<number: -1>` |
| `tabIndex=(object)`| (initial)| `<number: -1>` |
| `tabIndex=(numeric string)`| (initial, ssr mismatch)| `<number: -1>` |
| `tabIndex=(numeric string)`| (changed)| `<number: 42>` |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This shows it worked.

@gaearon gaearon merged commit fbd6b9d into facebook:master Oct 2, 2017
@gaearon gaearon mentioned this pull request Oct 3, 2017
19 tasks
let e = await render(<svg tabindex="1" />, 1);
expect(e.tabIndex).toBe(1);
},
);
Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 9, 2017

Choose a reason for hiding this comment

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

Why is this expected to work if SVG is case sensitive? I see that there is a warning, but how does it actually get set?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might differ in another browser, but a quick check in Chrome shows that it works, and it gets assigned as tabindex:

https://codepen.io/anon/pen/PJeoeE

screen shot 2017-10-09 at 2 51 16 pm

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.

<svg tabIndex="2"> doesn't work
6 participants