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

• 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
7307715#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]

I added a working web example with RNW + kept my implementation (using TouchableWithoutFeedback) because the current implementation did not worked when I tried it.

There's still a small issue with ref forwarding, but it could be fixed in another PR.
I also deleted the PanReponder example (it just does not work, either on web or mobile)

[zoontek]

My recommandation: Cleanup this repository to the max, update dependencies, and schedule some breaking changes.
Delete all press handling props / methods (for the web and native).

It just add a lot of complexity for nothing. Developers can wrap Svg elements with Pressable, View + PanResponder, Animated.View… it's fine!

[WoLewicki]

I think it is not a good idea to mix many different changes in one PR since it makes it a lot harder to spot bugs and revert them later. As for the PanResponder example, it works for me both on Android and iOS, so I am not sure what you meant by this. Could you maybe split this PR to one adding web support for Example app, and maybe another adding examples of how to combine Pressable etc. with Svg to obtain the same result as it is now with all this code?

[zoontek]

@WoLewicki I had several errors in the console. I can create a new PR to fix the example app + add react-native-web for it (and maybe delete TestsExample? I think this directory is useless)

[WoLewicki]

TestsExample will be used for providing tests examples for new PRs, which can be then tested to check if there are no regressions e.g. before a release. Example app is to be treated as a showcase of how to do things with the library, you can see the same structure in react-native-screens. PR with the changes you mentioned would be very welcome for sure 🎉

[zoontek]

@WoLewicki The PR is here: #1717

Like I said here, the onPress* are currently broken on the web, the latest fix removed the focus on all SVG elements, but also broke pressability. And the PanResponder example doesn't work.

But IMHO it's a good start to implement breaking changes. I can help creating a roadmap for v13.

[WoLewicki]

I added a review of #1717 and it seems to work just fine, thanks for that 😄 Are you sure #1585 broke the pressability? I commented the if checking for pressable options and on web the elements still do not react to being pressed, but I may be doing something wrong.

[zoontek]

@WoLewicki Indeed, it might was super broken with focus on all elements and pressability broken too, I didn't checked 😄

[WoLewicki]

Could you merge the current main to resolve conflicts?

[zoontek]

@WoLewicki Done. However, I recommend using that as a temporary solution and remove press handling in the next major version 🙏

It sures fixes the onPress related events, but:

• svg are not focusables
• some press responders errors are printed in the console when onPress* is set. It's working tho (but it should be removed, it's easier to wrap the whole Svg with a Pressable)
• PanResponder does not work when set directly on the Svg (but who does that? just wrap them with a View, they accept GestureHandlers)

[WoLewicki]

Sorry for the late response. So do you think this PR provides better solution than the current implementation? About wrapping the whole Svg with a Pressable, do you think it won't add new problems, as the clickable area?

[zoontek]

It's better than the current implementation for sure. But not perfect.

Advices to wrap SVG with Pressable / Animated.View / View with PanResponder will allow deletion of a lot of code / the suppression of a lot of issues 🙂

[msand]

I think quite a few people depend on the press related functionality to only respond when interacting with filled / stroked parts of them canvas. I'm quite sure that wrapping with Pressable / PanResponder won't respect that part of the api. Or?

[zoontek]

@msand IMHO, if a part of the SVG should be animated / move using PanResponder but not the rest, that's not an SVG, but two.

And even with that, components are implemented using basic UIView / View, it shouldn't be an issue.

[msand]

Well, in practise, if the pressability would change to be based on bounding boxes rather than fill / stroke, a bunch of use cases would no longer be possible or would be buggy. The ability to have events based on this comes from the svg spec, so I'd say it is in scope of what should be supported by react-native-svg for single svg components.

[cloudratha]

This is a blocker for our A11y audits. I've moved our implementation to module alias react-native-svg to react-native-svg-web in the meantime.