fix: [web] Don't apply SvgTouchableMixin on every component

[zoontek]

Hi 👋

Do you remember this bug? Well, it happens again, for a different reason. 😄

By default, every component of this lib implements the SvgTouchableMixin, even if no Touchable properties are used, making every component accessible by default again.

Summary

• hasTouchableProperty is now a function, SvgTouchableMixin is applied only if it returns true.

Test Plan

What's required for testing (prerequisites)?

1. Init a project with create-react-app and react-native-web
2. Add react-native-svg dependency and render a random SVG using it

What are the steps to reproduce (after prerequisites)?

1. SVG elements should not be accessible and focusable if Touchable props are not present.
2. SVG elements should be accessible and focusable if Touchable props are present.

Checklist

• [X] I have tested this on a browser
• [ ] I added documentation in README.md
• [ ] I updated the typed files (typescript)
• [ ] I added a test for the API in the __tests__ folder

[msand]

Oh interesting, I suspect this won't handle cases where at first there's no touchable property, but in a later re-render there is. Wonder what makes it accessible to begin with, have you dug deeper into this?

[zoontek]

@msand I didn't check about this! I will make the changes to handle this case!

[msand]

Hmm, I can't seem to figure out why that return value would depend on using TouchableMixin, can't find anything inside it that should effect it. Guess I'd have to run the actual code to figure it out.

[zoontek]

@msand Instead of using a TouchableMixin, we might conditionally wrap the rendered element with a TouchableWithoutFeedback, no?

[msand]

I think that probably wont work well together with gesture responders. Not sure though.

[msand]

This seems to work:

const isWeb = Platform.OS === 'web';
const keys = Object.keys(SvgTouchableMixin);
const touchKeys = isWeb ? keys.filter(key => key !== 'componentDidMount') : keys;

[msand]

Seems related to this._isTouchableKeyboardActive in Touchable Mixin Edit: sloppy reasoning

[msand]

Actually, deciding factor seems to be: this._touchableNode.addEventListener('blur', this._touchableBlurListener); https://github.com/necolas/react-native-web/blob/1ad16930391303da511c98879fa7b2002b28c822/packages/react-native-web/src/exports/Touchable/index.js#L381

[msand]

Not sure how the presence / absence of a blur handler decides if a svg element can become the active element or not.

[msand]

I see

https://svgwg.org/svg2-draft/interact.html#Focus

User agents may treat other elements as focusable, particularly if keyboard interaction is the only or primary means of user input. In particular, user agents may support using keyboard focus to reveal ‘title’ element text as tooltips, and may allow focus to reach elements which have been assigned listeners for mouse, pointer, or focus events. Authors should not rely on this behavior; all interactive elements should directly support keyboard interaction.

[msand]

@necolas Is this behaviour intentional? Is the blur event handler partially there to make some svg elements focusable? What are the cases in which they should be focusable and not?

I'd assume if there's no focus/press/gesture related handlers, they shouldn't be focusable, while if that changes, it should adapt correctly.

Seems like disabling the blur event listener / skipping componentDidMount is not a good solution.

Could perhaps attach/detach as a side-effect based on the presence of event handlers. Wdyt?

[msand]

@necolas Sorry to ping you twice like this. Feel free to instruct me not to ping you and I won't cause any further interruptions. Was suspecting it went past your radar / forgot about this. Would you mind having a quick look and giving a bit of guidance with regards to the touchable api in react-native-web? I'm considering I should write a react-native-svg specific handler, that de/attaches the blur event listener on demand. Do you see any problems with this approach?

[msand]

@zoontek What do you think of this implementation?

[zoontek]

@msand This should work, I can try it tomorrow to be sure if you want. But this (kind of) seems fragile. Maybe react-interactions will help at some point? For now the implementation target React DOM, but I think it's their solution to get rid of Touchable Mixins at some point.

[msand]

Yeah, this certainly isn't very clean, but the closest api to the react-native implementation and most react-native-web compatible I can think of at the moment. I'd love to see a cleaner implementation of this and certainly willing to merge it.

[msand]

I think the mixin should be run on the prototype once rather than on each instance object as well. Only the event listeners need to be attached, and the state initialized, per instance.

[necolas]

I don't have enough context to understand what's going on here. You're trying to mixin Touchable on an SVG element? Why is that part of an svg library?

[msand]

It's used in the react-native implementation to get support for touch/gesture events, so quite a few components are implemented based on that api of react-native-svg, and thus a compatibility layer for the web components has been requested.

[msand]

@necolas Perhaps I should elaborate, the Touchable mixin has been part of the (Shape) api since v4.1.0 or 9 Jun 2016 https://github.com/react-native-community/react-native-svg/commit/73077157124c84cfa87c00055b96cfb697288d9d#diff-8a2f6ca181e875c2bffa1bebff2800b2

Many of the use cases of this library aren't static content, but interactive visualisations and complex ui elements with various touchable filled and/or stroked areas. Many of them are simple press handlers, others have turing complete dependence on the history of gesture events / other inputs.

In the case of this pr, as we've made the api work the same way on the web as it already does in native (i.e. support touch and gesture handler directly on the components), causes the dom elements to be focusable, as the componentDidMount handler from Touchable in react-native-web attaches a blur event handler to the dom elements, causing them to be considered accessible elements, and thus when you tab thru the page, the elements receive focus, even if they don't have any press / gesture event handlers actively attached to the react-native-svg elements.

This pr currently detaches the blur event handler after componentDidMount, in case there are no touchable props given, and reattaches if at a later point there is. Ideally we would have had two sets of elements, one with touchable support, and one without, but this api was grandfathered in before I made my first contributions.

@zoontek Did you have time to test this?

[zoontek]

@msand Yes, I use the last version on two projects and it works correctly now.

Otherwise if I understand the necessity of interactive SVG, why can't we just wrap elements in TouchableWithoutFeedback? (Ex: to make a Path pressable). It's added complexity and possible side effects for not so much.

[msand]

@zoontek You mean the last released version? last commit from develop branch? or from this branch/pr?

That would be ideal, but I'm not sure I could sustain the amount of opened issues that kind of breaking change to the api would cause. I would likely stop supporting this library, given the history of types of issues opened over the past few years. So, perhaps in case you would commit to handling all issues of that type I could consider it.

[zoontek]

My bad, I was thinking about the other issue I had (height & width at 100% by default). I forgot to try this one.

I installed it on my project (using this branch, up-to-date) and all the elements are still focusables.

[colinhostetter]

Hey, is there any plan to merge this? I'm encountering the same problem (all SVG elements being tabbable) that this is discussing.

[Freddy03h]

Hi! Is there any progress on this? For now, react-native-svg is not usable on an RNW project with accessibility considerations.

I'm also using react-native-svg on a RN project and wasn't aware that elements can be touchable.

I'm surely miss a point. What is preventing to use the library like this:

<Svg>
  <Touchable>
    <Rect />
  </Touchable>
</Svg>

instead of having onPress/* directly on svg elements ?

[zoontek]

IMHO, the cleanest way to do this should be dropping RNW 0.13 support and relying on usePressEvents. Exactly like the TouchableWithoutFeedback implementation.

I can easily make the change. Even if I also think that the best change should be to drop support for press events il the whole codebase (like @Freddy03h said), and rely to Touchable* and Pressable instead.

[zoontek]

Huge 🆙

I rewrote the web implementation completely without the TouchableMixin. Instead, I apply TouchableWithoutFeedback conditionally in order to keep touch events for now. Accessibility is fixed, performances are superior (even if the way the style is resolved might be improved), codebase is a bit more up to date.

I also bumped some type definitions / some dependencies versions in order to fix the build (Bob build / type generation failed with the version set), added an /example project to help future contributors.

I also switched to yarn. Sorry about this one, but npm delete the example project when your run the reinstall script in it - for some reason). As I found a bit silly to have a yarn.lock in example and a package-lock.json at root, I switched globally. I can revert it.

[lejsue]

I have this problem too, is it possible to merge these commits into main branch? Thx

[ElForastero]

I think it's time to merge this. The problem is really serious

[exneval]

already test the implementation and so far so good ty @zoontek

[WoLewicki]

Since https://github.com/react-native-svg/react-native-svg/pull/1585 has been merged, and I think this PR takes quite another approach to the problem, could you update this PR to only concern the ReactNativeSVG.web.ts file if it is possible? As for the other changes, the example app is already in the repo. As a side note, mixing multiple things in one PR makes it really hard to review and test easily 😅 Or maybe https://github.com/react-native-svg/react-native-svg/pull/1585 is enough for most use cases and we can close this? I believe the problem occurs when re-rendering the component with touchable props added. Am I right? cc @zoontek