fix: Change name of focus and blur events to searchFocus and searchBlur

[tboba]

Description

Right now, while trying to use search bar on new architecture, iOS throws an error, that topFocus event cannot both direct and bubbling. That's mainly because RNSSearchBar component extends from View, which already has topFocus event registered.
This PR changes the naming of this event (along with topBlur, to keep compliance of both behaviours) to topSearchFocus and topSearchBlur.

Fixes react-navigation/react-navigation#11928.

Changes

• Changed topFocus and topBlur event to topSearchFocus and topSearchBlur,
• Added type that renames onFocus and onBlur props inside SearchBar native component (to don't introduce breaking change).

Screenshots / GIFs

Before

8mb.video-rrF-K1N8IW5v.mp4

After

8mb.video-CCQ-5AnoEr2J.mp4

Test code and steps to reproduce

You can test Test758.tsx test, along with Example -> Search bar to test behaviour of the events on search bar. Eventually, you can use snippet below, that has been enhanced by adding console.logs on every search bar event.

Enhanced test

/* eslint-disable react-hooks/exhaustive-deps */
import * as React from 'react';
import { Button, NativeSyntheticEvent, ScrollView } from 'react-native';
import {
  NavigationContainer,
  NavigationProp,
  ParamListBase,
} from '@react-navigation/native';
import {
  createNativeStackNavigator,
  NativeStackScreenProps,
} from '@react-navigation/native-stack';
import { SearchBarProps } from 'react-native-screens';

const AppStack = createNativeStackNavigator();

export default function App(): JSX.Element {
  return (
    <NavigationContainer>
      <AppStack.Navigator
        screenOptions={{
          headerLargeTitle: true,
          headerTransparent: false,
        }}>
        <AppStack.Screen name="First" component={First} />
        <AppStack.Screen name="Second" component={Second} />
      </AppStack.Navigator>
    </NavigationContainer>
  );
}

function First({ navigation }: NativeStackScreenProps<ParamListBase>) {
  React.useLayoutEffect(() => {
    navigation.setOptions({
      headerSearchBarOptions: searchBarOptions,
    });
  }, [navigation]);

  const [search, setSearch] = React.useState('');

  const searchBarOptions: SearchBarProps = {
    barTintColor: 'powderblue',
    tintColor: 'red',
    textColor: 'red',
    hideWhenScrolling: true,
    obscureBackground: false,
    hideNavigationBar: false,
    autoCapitalize: 'sentences',
    placeholder: 'Some text',
    cancelButtonText: 'Some text',
    onChangeText: (e: NativeSyntheticEvent<{ text: string }>) => {
      setSearch(e.nativeEvent.text);
      console.warn('Search text:', e.nativeEvent.text);
    },
    onCancelButtonPress: () => console.warn('Cancel button pressed'),
    onSearchButtonPress: () => console.warn('Search button pressed'),
    onFocus: () => console.warn('onFocus event'),
    onBlur: () => console.warn('onBlur event'),
    onOpen: () => console.warn('onOpen event'),
    onClose: () => console.warn('onClose event'),
  };

  const items = [
    'Apples',
    'Pie',
    'Juice',
    'Cake',
    'Nuggets',
    'Some',
    'Other',
    'Stuff',
    'To',
    'Fill',
    'The',
    'Scrolling',
    'Space',
  ];

  return (
    <ScrollView
      contentInsetAdjustmentBehavior="automatic"
      keyboardDismissMode="on-drag">
      <Button
        title="Tap me for second screen"
        onPress={() => navigation.navigate('Second')}
      />
      {items
        .filter(item => item.toLowerCase().indexOf(search.toLowerCase()) !== -1)
        .map(item => (
          <Button
            title={item}
            key={item}
            onPress={() => {
              console.warn(`${item} clicked`);
            }}
          />
        ))}
    </ScrollView>
  );
}

function Second({ navigation }: { navigation: NavigationProp<ParamListBase> }) {
  return (
    <ScrollView contentInsetAdjustmentBehavior="automatic">
      <Button
        title="Tap me for first screen"
        onPress={() => navigation.navigate('First')}
      />
    </ScrollView>
  );
}

Checklist

• Included code example that can be used to test this change
• Updated TS types
• Updated documentation:
  • https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
• Ensured that CI passes

[kkafar]

Thanks for taking care of this 🙌🏻 Left important remarks below.

[kkafar]

I appreciate the idea here (and like it btw.), however do we need this? If w/o this fix setting these options would crash application anyway I believe we can safely treat this as a required fix and not a breaking change, thus removing the need for such workaround.

[tboba]

I think I require an assistance of @WoLewicki there, as I was speaking with him about this change 😁 but when I was wondering about it, I was thinking that it would also require us to introduce the breaking change inside the react-navigation, and also there will be time for introducing such breaking changes (very soon 🤭). What do you think?

[WoLewicki]

We don't want to change the user's API for sure so the SearchBarProps must stay the same but NativeSearchBar should take its props from the spec inside fabric directory so we are sure we are sending the proper ones to the native side. Or am I missing something?

[tboba]

I agree with you Wojtek and I would totally go into using Spec as the type only for codegen components, instead of reusing types from types.tsx, but tbh I'd leave this task for a separate PR. What do you think?

[WoLewicki]

But then you wouldn't have to add this RenamedSearchBarProps type at all yeah? But if you want, you can apply it in the follow-up PR. I'd also change all props to have prefix of onSearch... to keep the convention and not stumble to some similar issue later.

[kkafar]

So the conclusion would be to:

1. leave types in types.tsx intact (so the public API of our lib)
2. Make NativeSearchBar use prop types that do come from Fabric spec, not from the types.tsx
3. in SearchBar component hijack these event-related props: onBlur & onFocus and pass them to as values to onSearchBlur & onSearchFocus props on NativeSearchBar respectively

This way we avoid the need for this typescript hacks.

[tboba]

Hi @WoLewicki @kkafar, I've just added a change that doesn't impact types file and does a little bit tricky stuff inside the SearchBar, but I think this is fine 😄. Could you tell me what do you think about it? 299ce6a

[kkafar]

It is likely I do not see something, but I still don't get why do we need such workarounds.

My impression is that we can have SearchBarProps (from types & native-stack/types unchanged, and just use different types on NativeSearchBar (do we actually need this proxy component instead of just SearchBarNativeComponent?) while bridging it as it is currently done in SearchBar (forwarding props with changed names).

[tboba]

@kkafar We unfortunately cannot edit types from NativeSearchBar, since Spec allows you to only have given set of types. Because of that, you cannot mix the type as something like onFocus?: DirectEventHandler<SearchBarEvent> | (e: NativeSyntheticEvent<TargetedEvent>) => void | null and you need to do pretty tricky stuff to mix these types somewhere else (like I do, in SearchBar.tsx).

[kkafar]

Let's look into this issue on Monday, cause I want this PR landed before I do a release.

[tboba]

@kkafar sure 👍

[kkafar]

Final remarks before approving

[kkafar]

What are these changes to react navigation? Are they here on purpose?

[tboba]

Yeah, I've just bumped this submodule to main. Do you think we should keep these changes?

[kkafar]

It's not pretty, but it's better than breaking changes in public types 🎉

[WoLewicki]

Seems sad that it cannot infer one from another. Is it the same in all our libs and specs that we have to cast the type from codegen?

[kkafar]

Ok, so last thing is: what about those changes to HeaderConfig and readme in react-navigation, why do we have them here?

[tboba]

@kkafar it's just a bump of main branch on submodule. Do you think we should keep it?

[kkafar]

@tboba, it seems that these do no introduce any changes in business logic, so it's ok. But for the future let's keep that separated please.

[kkafar]

So, if you have tested this in action, let's merge this please