Skip to content

fix#1471: every element is focusable on web - #1585

Merged
WoLewicki merged 2 commits into
software-mansion:developfrom
intergalacticspacehighway:fix/web-focusable-elements
Mar 2, 2022
Merged

fix#1471: every element is focusable on web#1585
WoLewicki merged 2 commits into
software-mansion:developfrom
intergalacticspacehighway:fix/web-focusable-elements

Conversation

@intergalacticspacehighway

@intergalacticspacehighway intergalacticspacehighway commented May 10, 2021

Copy link
Copy Markdown
Contributor

Fixes #1471

Summary

As described in #1471, each SVG element is focusable/tabable on web by default. This can create issues in keyboard navigation.
As explained by @jondkoon #1471 (comment), the TouchableMixin attaches blur handler which makes SVG elements focusable.

So, to solve this, we only attach touchable mixin handlers if the element has any touchable props.

Test Plan

Before:
Each element is focusable

Screen.Recording.2021-05-11.at.12.45.41.AM.mov

After:
No element is focusable

Screen.Recording.2021-05-11.at.12.47.36.AM.mov

What's required for testing (prerequisites)?

  • Render any icon on web.

What are the steps to reproduce (after prerequisites)?

  • Verify the elements are not focusable.

Compatibility

OS Implemented
web ✅

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

@intergalacticspacehighway intergalacticspacehighway mentioned this pull request May 10, 2021
Closed
@efoken

efoken commented May 27, 2021

Copy link
Copy Markdown

Finally 👍

@TikiTDO

TikiTDO commented Jul 5, 2021

Copy link
Copy Markdown
Contributor

@amgleitman / @msand is there any chance to get this merged in? We'd really like to see this fix in cause it would solve some major UX issues.

@jpaas

jpaas commented Aug 13, 2021

Copy link
Copy Markdown

Would love to get this integrated too. Forms are not lot of fun if you're using svg.

@voidrender

voidrender commented Sep 6, 2021 •
edited
Loading

Copy link
Copy Markdown

I took these changes and created a patch you can apply to 12.1.1 with patch-package to fix this in your project today. (The patch also includes a few newer commits from the develop branch because I wasn't able to get the project to build at the 12.1.1 tag.)

@jpaas

jpaas commented Sep 7, 2021

Copy link
Copy Markdown

Thank-you @ioveracker I will check it out!

@efoken

efoken commented Nov 5, 2021

Copy link
Copy Markdown

Something new here?

@andrejstas

andrejstas commented Nov 18, 2021 •
edited
Loading

Copy link
Copy Markdown

Same question here, will this get merged?

@RWOverdijk

RWOverdijk commented Nov 23, 2021

Copy link
Copy Markdown

I don't like being part of bumpers, but bump. I'm using a lot of ts-ignores now and I'm afraid I might get attacked by the other devs if I keep adding them.

@exneval

exneval commented Dec 26, 2021 •
edited
Loading

Copy link
Copy Markdown

#1294 guess also talks about the same issue and give better workaround

@shawngustaw

shawngustaw commented Dec 30, 2021

Copy link
Copy Markdown

Would love to get this merged, but a temp workaround is adding tabIndex={-1} to Svg and Path components. Requires a @ts-ignore but seems like it works well enough temporarily.

@TikiTDO

TikiTDO commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

@brentvatne Any chance you could release a version of this lib with this fix?

@bondtiffanym

bondtiffanym commented Jan 21, 2022

Copy link
Copy Markdown

Hopping on the bump bandwagon, I would love to see this patched 👍🏻

@TikiTDO

TikiTDO commented Mar 1, 2022

Copy link
Copy Markdown
Contributor

@kmagiera @WoLewicki Is there any chance one of you could merge in this fix? This would help a whole lot with cleaning up our code.

WoLewicki
WoLewicki reviewed Mar 1, 2022
View reviewed changes
Comment thread src/lib/SvgTouchableMixin.ts Outdated
const numTouchKeys = touchKeys.length;

export default (target: { [x: string]: unknown; state: unknown }) => {
const hasTouchableProperty = getHasTouchableProperty(target.props);

@WoLewicki WoLewicki Mar 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should apply those changes only on web? Or does it not change anything on native platforms?

@intergalacticspacehighway intergalacticspacehighway Mar 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching it. Updated!

WoLewicki
WoLewicki approved these changes Mar 2, 2022
View reviewed changes

@WoLewicki WoLewicki left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 🎉 Thanks for this PR @intergalacticspacehighway

@WoLewicki
WoLewicki merged commit 69ff5f0 into software-mansion:develop Mar 2, 2022
@intergalacticspacehighway
intergalacticspacehighway deleted the fix/web-focusable-elements branch March 2, 2022 13:51
@WoLewicki WoLewicki mentioned this pull request Mar 2, 2022
Open
4 tasks
@mateusbra mateusbra mentioned this pull request Aug 10, 2022
Merged
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WoLewicki WoLewicki WoLewicki approved these changes
+1 more reviewer
@OfryL OfryL OfryL approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Every element of svg is focusable by default (web, chrome)