feat: Add testId and accessibilityLabel for BottomTabsScreen and Item

[kmichalikk]

Closes https://github.com/software-mansion/react-native-screens-labs/issues/503

Description

This PR adds testId and accessibilityLabel support for BottomTabsScreen and Item. For iOS, this maps to accessibilityIdentifier and accessibilityLabel, respectively. For android, things are more complicated when it comes to the item (the screen is handled automatically by react native in BaseViewManager):

• testId is mapped to tag since this is what appeared to be working with Detox, which uses espresso under the hood
• accessibilityLabel is mapped to the part of the contentDescription that overwrites tab title. With tab badge present, the final description is the title and badge description combined. Detox seems to be matching the parts of the description correctly with only the title/label as the selector. This prop is only available on android on API level 26 and up.

Changes

Added 4 props to the bottom tab configuration object:

• screenTestId
• tabBarItemTestId
• screenAccessibilityLabel
• tabBarItemAccessibilityLabel

Added nativeBottomTabs.e2e.ts test.

Test code and steps to reproduce

Run the e2e test. Use e2e-utils from https://github.com/software-mansion/react-native-screens/pull/3430/changes#diff-04033cb65a060a8211fbcd7be662a11be0f32c9b98d2f0d510c489e4e74af450, otherwise iOS won't complete.

[kkafar]

Good job! The code looks good here, I have just few remarks regarding it's exact structure. Please answer them.

[kkafar]

The code looks good, but I think it is in wrong place.

We should keep the flow. Instead of setting these props here, the intention of update should be propagated to tab bar controller (see how appearance is handled) and then actual props should be updated in RNSTabBarController.reactMoutingTransactionDidMount, e.g. after appearance pass is done.

[kmichalikk]

88ef668

let me know if it could be implemented this way?

[kmichalikk]

I'm not 100% positive we don't need to recreate the tab item when changing the id / label.

Please check and let me know if adding the following code to bottom tabs' index.tsx to update the props programmatically after 2s actually changes the props (on iOS, with accessibility inspector).

  const [tabConfigs, setTabConfigs] = React.useState(TAB_CONFIGS);

  useEffect(() => {
    setTimeout(() => {
      setTabConfigs([
        { ...TAB_CONFIGS[0], tabScreenProps: { ...TAB_CONFIGS[0].tabScreenProps, tabBarItemTestID: 'pomidor0', tabBarItemAccessibilityLabel: 'pomidor0' } },
        { ...TAB_CONFIGS[1], tabScreenProps: { ...TAB_CONFIGS[1].tabScreenProps, tabBarItemTestID: 'pomidor1', tabBarItemAccessibilityLabel: 'pomidor1' } },
        { ...TAB_CONFIGS[2], tabScreenProps: { ...TAB_CONFIGS[2].tabScreenProps, tabBarItemTestID: 'pomidor2', tabBarItemAccessibilityLabel: 'pomidor2' } },
        { ...TAB_CONFIGS[3], tabScreenProps: { ...TAB_CONFIGS[3].tabScreenProps, tabBarItemTestID: 'pomidor3', tabBarItemAccessibilityLabel: 'pomidor3' } },
      ])
    }, 2000);
  }, []);

...

      <BottomTabsContainer
        tabConfigs={tabConfigs}

[kkafar]

This looks good, and seems to work. Thank you!

I've left few more important remarks that need to be answered before we can land these changes.

[kkafar]

Here you define testID & accessibilityLabel for the TabScreen itself, however please note that TabScreen Android implementation does not inherit form ReactViewGroup. How does it work then? Could you describe it in PR description, please?

Is is the question of ReactViewManager using the accessibility delegate & not relying on state defined explicitly in ReactViewGroup?

[kmichalikk]

AFAIK this is set by BaseViewManager which TabScreenViewManager extends somehow

[kkafar]

Okay. So it works provided that BaseViewManager does not rely on any state defined explicitly in ReactViewGroup. This is not perfect, but I guess we should be fine.

Can we please add a note to PR description that this is the case? (that we rely on ReactAndroid here).

[kkafar]

What are these properties for here? If they are already defined on screen, then do we need to duplicate those here?

I see them being set in - [RNSBottomTabsScreenComponenView updateProps:oldProps] however I don't see a purpose. Could you explain it please?

[kmichalikk]

I don't see them being defined anywhere else? I asked if it could be implemented this way here: #3497 (comment)

[kkafar]

Don't you wanna cleanup the invalidated flag here?

[kkafar]

Ideally this update should be deferred & only signalised to the controller. Same as appearance update or orientation update. Can we please align that?

I see now, that we haven't done so for scrollview edgeeffects for some reason. It should be subject of refactor in follow up PR, can we create ticket for this?

[kmichalikk]

This is not the same, because here we want to update a specific item, and other deferred ones update the whole tabBar. I'd need to pass some id to get the item again in TabBarController, and I'm not sure it would look good.

[kkafar]

We can add state do the screen view controller (or just expose a property on controller & read the state from screen view) that can be read in the main loop of appearance coordinator to decide whether update is required or not.

I think this might be the way to go.

[kkafar]

Responded to your concern.

[kkafar]

We can add state do the screen view controller (or just expose a property on controller & read the state from screen view) that can be read in the main loop of appearance coordinator to decide whether update is required or not.

I think this might be the way to go.

[kligarski]

The code looks good, I left 2 suggestions.

[kkafar]

Looks good. Thank you.