Add disabled property to props being passed into Pressable component

[Yajur-Grover]

Description

Adds the value of the disabled property to props that are being passed into the Pressable component native code. Additionally, also adds a disabled property check before calling the onKeyDown event handler.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Why

Resolves microsoft/react-native-gallery#364
Resolves microsoft/react-native-gallery#363

What

Added statement to set Boolean value of prop before passed into Pressable component, and disabled property conditional check to onKeyDown handler.

Testing

Tested in RNTester and standalone example Pressable component.

Sample Pressable component code:

<Pressable
  accessibilityRole="button"
  accessibilityLabel={'example disabled pressable'}
  style={{
    width: 140,
    height: 50,
    borderRadius: 2,
    backgroundColor:'silver',
    opacity: 0.5,
  }}
  disabled={true}>
  {({pressed}) => (
    <Text
      style={{
        textAlign: 'center',
        paddingVertical: 15,
        color: 'black',
      }}>
      {pressed ? 'This will never be triggered.' : 'Disabled Pressable'}
    </Text>
  )}
</Pressable>

Changelog

Should this change be included in the release notes: yes

Microsoft Reviewers: Open in CodeFlow

[chrisglein]

Tested in RNTester and standalone example Pressable component.

Would it be possible to add an automated test for this? The sort that would have caught a regressing to this the first place?

[Yajur-Grover]

Would it be possible to add an automated test for this? The sort that would have caught a regressing to this the first place?

I plan to update the 'Disabled Pressable' example in RNTester, and will also look into adding a test to the PressableComponentTest file that tests the disabled component.

[acoates-ms]

Shouldn't we instead have pressability not call the onKeyUp/Down callbacks when the pressability is disabled.

react-native-windows/vnext/src-win/Libraries/Pressability/Pressability.windows.js

Line 659 in 4f6cbba

(event.nativeEvent.code === 'Space' ||

Otherwise, you could run into issues with a pressability that would still get clicked if there was a focusable view within a disabled pressability.

[Yajur-Grover]

Shouldn't we instead have pressability not call the onKeyUp/Down callbacks when the pressability is disabled.

react-native-windows/vnext/src-win/Libraries/Pressability/Pressability.windows.js

Line 659 in 4f6cbba

(event.nativeEvent.code === 'Space' ||

Otherwise, you could run into issues with a pressability that would still get clicked if there was a focusable view within a disabled pressability.

I had made this change initially, but after reviewing with Chiara the current solution appeared to be more optimal, since the main issue was the disabled prop was not being passed to the ViewViewManager.

I just tested adding a disabled check to the onKeyUp/Down handlers against a focusable view within a disabled pressable. This prevents the user from pressing on the pressable, but still allows focus on the view. The issue with just having a disabled prop check on the onKeyUp/Down handlers (without the current fix) is that it still allows keyboard focus on any disabled Pressable - so maybe the solution is to add both changes? @chiaramooney tagging in case you have additional input.

[acoates-ms]

Shouldn't we instead have pressability not call the onKeyUp/Down callbacks when the pressability is disabled.

react-native-windows/vnext/src-win/Libraries/Pressability/Pressability.windows.js

Line 659 in 4f6cbba

(event.nativeEvent.code === 'Space' ||

Otherwise, you could run into issues with a pressability that would still get clicked if there was a focusable view within a disabled pressability.

I had made this change initially, but after reviewing with Chiara the current solution appeared to be more optimal, since the main issue was the disabled prop was not being passed to the ViewViewManager.

I just tested adding a disabled check to the onKeyUp/Down handlers against a focusable view within a disabled pressable. This prevents the user from pressing on the pressable, but still allows focus on the view. The issue with just having a disabled prop check on the onKeyUp/Down handlers (without the current fix) is that it still allows keyboard focus on any disabled Pressable - so maybe the solution is to add both changes? @chiaramooney tagging in case you have additional input.

Wouldn't that be fixed by having the JS not set focusable when the pressable is disabled?

react-native-windows/vnext/src-win/Libraries/Components/Pressable/Pressable.windows.js

Line 327 in 4f6cbba

focusable: focusable !== false,

Generally, the meaning of "disabled" is pretty subjective, and I'd rather enable the JS to be explicit on the behavior they want, rather than having native enforce a lot of policy which may or may not be what the dev wants.

Currently the main thing I see the fabric code using disabled for on view is to control the UIA_IsEnabled property, which should really be done through a aria-disabled property.

[Yajur-Grover]

Wouldn't that be fixed by having the JS not set focusable when the pressable is disabled?

In my example, I had set the View prop to focusable, so I think the View JS code would have to be changed? I tried changing line 327 to focusable: (focusable !== false && disabled !== true) so that focusable is always false if disabled is true, but didn't seem to have an impact - could still tab onto the pressable.

Generally, the meaning of "disabled" is pretty subjective, and I'd rather enable the JS to be explicit on the behavior they want, rather than having native enforce a lot of policy which may or may not be what the dev wants.

Regarding this, the main behavior for the disabled prop I've been trying to replicate is WinUI 3 Button behaviour when disabled is set, as well as RN core behaviour for pressable. On WinUI 3, when a Button is disabled, you can't tab onto it with keyboard and can't click onto it. Currently, you can tab onto and press a disabled pressable, which is what is causing the accessibility compliance issue.

I've pushed the change for the onKeyUp/Down change, which I believe should fix the being able to press a disabled pressable when there is a focusable view.

[acoates-ms]

On WinUI 3, when a Button is disabled, you can't tab onto it with keyboard and can't click onto it.

This is one of the policy decisions that I'm trying to avoid. In WinUI there was a policy that anything "disabled" automatically meant it couldn't be focused. Then later use cases were found that required users be able to focus disabled controls, and it was impossible to do so. The way to unblock it involved having to wait for WinUI to add a AllowFocusWhenDisabled property. So now instead of just IsTabStop and Disabled there are three properties on Xaml Control, all to avoid a policy that made IsTabStop and Disabled interact with each other.

In my example, I had set the View prop to focusable

Wait, so the bug is that you have a Pressable that you set to focusable and disabled, and you want it to not be focusable? And thats RNWs fault? -- Is it not the apps fault for setting focusable?

[Yajur-Grover]

Wait, so the bug is that you have a Pressable that you set to focusable and disabled, and you want it to not be focusable? And thats RNWs fault? -- Is it not the apps fault for setting focusable?

No I may have explained incorrectly - the bug is when the disabled property is set, users can still tab onto the pressable and press it, which is what the PR initially was resolving, as the disabled property was not being passed onto the ViewViewManager. Link to bug

Otherwise, you could run into issues with a pressability that would still get clicked if there was a focusable view within a disabled pressability.

The example was something I tested today to test the case you mentioned here, which is why I added the change that adds a disabled property check to the onKeyUp/Down handlers. This is what I took the case you mentioned to look like (where there is a focusable view within a disabled pressable), which is what I tested on playground:

    <Pressable
      accessibilityRole="button"
      accessibilityLabel={'example disabled pressable'}
      disabled={true}>
      {({pressed}) => (
        <View focusable={true}>
          <Text
            style={{
              textAlign: 'center',
              paddingVertical: 15,
              color: 'black',
            }}>
            {pressed ? 'This will never be triggered.' : 'Disabled Pressable'}
          </Text>
        </View>
      )}
    </Pressable>

This is one of the policy decisions that I'm trying to avoid. In WinUI there was a policy that anything "disabled" automatically meant it couldn't be focused. Then later use cases were found that required users be able to focus disabled controls, and it was impossible to do so. The way to unblock it involved having to wait for WinUI to add a AllowFocusWhenDisabled property. So now instead of just IsTabStop and Disabled there are three properties on Xaml Control, all to avoid a policy that made IsTabStop and Disabled interact with each other.

Okay that makes a lot of sense. Regarding the accessibility bug and the precedent that has been set before regarding behavior of certain components, the guideline that I've usually been given/followed is to compare the behavior of the corresponding control on WinUI 3 Gallery. From my understanding, changing the guideline for expected behavior may fall outside the scope of this PR - mainly due to the deadline for resolving the sev2 issues. I can open a separate issue/discussion post tracking the policy decision.