Fix: RefreshControl in FlatList makes borderWidth not working

[Nizarius]

Summary

Fixes #22752

On line 1021 you are passing base style to props:
style: [baseStyle, this.props.style],

Explicitly passing base style to ScrollView just overrides this line and doesn't let developers to customise style of any inheritors of ScrollView (not only FlatList) with custom RefreshControl.

So this line (1113) seems to be removed.

Changelog

[GENERAL] [Fixed] - fix of Android's bug that doesn't let override ScrollView's Style with custom RefreshControl.

Test Plan

Use this code snippet to test that passing RefreshController doesn't break styling of ScrollView on Android.

type Props = {};
export default class App extends Component<Props> {
  state = { isLoading: false };

  reload = () => {
    this.setState({ isLoading: true });

    setTimeout(() => {
      this.setState({ isLoading: false });
    }, 2000);
  };
  render() {
    return (
      <ScrollView
        style={{
          flex: 1,
          alignSelf: "stretch",
          borderWidth: 10,
          borderColor: "red"
        }}
        refreshControl={
          <RefreshControl
            colors={["red", "yellow"]}
            refreshing={this.state.isLoading}
            onRefresh={() => this.reload()}
          />
        }
      >
        <View
          style={{
            width: "100%",
            height: 300
          }}
        >
          <Text>3335</Text>
        </View>
        <View
          style={{
            width: "100%",
            height: 300
          }}
        />
        <View
          style={{
            width: "100%",
            height: 300
          }}
        />
      </ScrollView>
    );
  }
}

The same for FlatList:

type Props = {};
export default class App extends Component<Props> {
  state = { isLoading: false };

  reload = () => {
    this.setState({ isLoading: true });

    setTimeout(() => {
      this.setState({ isLoading: false });
    }, 2000);
  };
  render() {
    return (
       <FlatList
        style={{
          flex: 1,
          alignSelf: "stretch",
          borderWidth: 10,
          borderColor: "red"
        }}
        data={["a", "b", "c"]}
        renderItem={({ item }) => (
          <View style={{ height: 500 }}>
            <Text>{item}</Text>
          </View>
        )}
        refreshControl={
          <RefreshControl
            colors={["red", "yellow"]}
            refreshing={this.state.isLoading}
            onRefresh={() => this.reload()}
          />
        }
      />
    );
  }
}

[janicduplessis]

The issue here is that if we remove this line some styles will be applied twice, both on AndroidSwipeRefreshLayout and the ScrollView (here and here). Can you verify that adding a style like padding or margin doesn't get applied twice.

[Nizarius]

@janicduplessis yeah, you're right. In fact, margin properties only work on AndroidSwipeRefreshLayout and padding properties - only on ScrollView. So padding or margin doesn't get applied twice. But there is such a problem with opacity.
So the ideal solution seems something like that:

1. Extract all margin properties ('margin', 'marginRight' etc) and opacity from this.props.style and apply it to AndroidSwipeRefreshLayout (with baseStyle).
2. Pass other style properties (padding, borders etc) to ScrollView (with baseStyle).
   This can prevent unexpected behaviour in the future as we won't duplicate any properties to both views.

I don't know if you have some utility functions for extracting properties' family from style ("extract all margin properties" or something similar) to make code cleaner and more declarative. If you have I would use it.

What do you think?

[janicduplessis]

I don't think we have anything currently to do this. I have something like this in an app though:

const OUTER_PROPS = {
  margin: true,
  marginHorizontal: true,
  marginVertical: true,
  marginBottom: true,
  marginTop: true,
  marginLeft: true,
  marginRight: true,
  flex: true,
  flexGrow: true,
  flexShrink: true,
  flexBasis: true,
  alignSelf: true,
  height: true,
  minHeight: true,
  maxHeight: true,
  width: true,
  minWidth: true,
  maxWidth: true,
  position: true,
  left: true,
  right: true,
  bottom: true,
  top: true,
};

export function splitLayoutProps(props) {
  const inner = {};
  const outer = {};
  Object.keys(props).forEach(k => {
    if (OUTER_PROPS[k] === true) {
      outer[k] = props[k];
    } else {
      inner[k] = props[k];
    }
  });
  return { outer, inner };
}

You could also use StyleSheet.flatten to make sure to have an object from the style prop.

@sahrens What do you think about this?

[Nizarius]

@janicduplessis seems nice. We need to define where to place this utility function and OUTER_PROPS dictionary in the project.

[cpojer]

What's the status of this PR?

[Nizarius]

@cpojer I suggest, we are waiting comments for the best code styling. The implementation itself will be fast.

[sahrens]

Yeah let's add a splitLayoutProps export on StyleSheet and use that.

[Nizarius]

@sahrens @janicduplessis I've added splitLayoutProps to export with some tests to cover its functionality.
I'm also explicitly passing baseStyle to both wrapper's and container's styles to be sure that we don't break anything:

return React.cloneElement(
          refreshControl,
          {style: [baseStyle, outer]},
          <ScrollViewClass
            {...props}
            style={[baseStyle, inner]}
            // $FlowFixMe
            ref={this._setScrollViewRef}>
            {contentContainer}
          </ScrollViewClass>,
        );

But as I check it works without it. So it's up to you whether to leave it or not.

[Nizarius]

Any news?)

[cpojer]

This looks good to me. Can you rebase it on top of master and use relative requires instead + remove the .vscode/settings.json file? That file shouldn't go into this PR.

[Nizarius]

@cpojer yep, will do it soon

[Nizarius]

@cpojer done

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @Nizarius in d9a8ac5.

^{When will my fix make it into a release? | Upcoming Releases}