microsoft / react-native-macos

A framework for building native macOS apps with React.
https://microsoft.github.io/react-native-windows/
MIT License
3.55k stars 135 forks source link

`Pressable` `onHoverIn` `onHoverOut` is too slow #1861

Closed neo773 closed 2 months ago

neo773 commented 1 year ago

Environment

react-native -v: 10.2.4
npm ls react-native-macos: 
discord_rn@0.0.1 /Users/neo/Documents/temp/discord_rn
└── react-native-macos@0.71.11
node -v: v18.12.1
npm -v: 8.19.2
yarn --version: 1.22.19
xcodebuild -version:
Xcode 14.3.1
Build version 14E300c

Steps to reproduce the bug

Pressable onHoverIn onHoverOut is too slow to be used in a list it has a huge latency

https://github.com/microsoft/react-native-macos/assets/62795688/29dbc19e-a42c-46f6-a60c-95e3b7fa3f68

My code

import {
  Text,
  FlatList,
} from 'react-native';
import React, { useState } from 'react';
import { users } from '../api/types/ready';
import type { private_channels } from '../api/types/ready';
import UserIcon from '../components/UserIcon/UserIcon';
import { store } from '../store/store';
import { Pressable, View } from 'react-native-macos';

const Friends = ({
  users,
  private_channels,
}: {
  users: users[];
  private_channels: private_channels[];
}) => {

  const [hoveredIndex, setHoveredIndex] = useState<number | null>(null);

  const renderItem = ({
    item: item,
    index: index,
  }: {
    item: private_channels;
    index: number;
  }) => {
    const user = users.find(user => user.id === item.recipient_ids[0]);
    if (user && !user.username.includes('Deleted User')) {
      return (
        <Pressable
          className="flex flex-row my-2 items-center "
          style={{
            backgroundColor: index === hoveredIndex ? '#404249' : '#2B2D31',
            cursor: 'pointer',
          }}
          onPress={() => {
            store.dispatch.appState.updateUI({
              selected_guild_id: '',
              selected_channel_id: item.id,
              selected_tab: 'FRIENDS',
              selected_channel_name: `@${user.username}`,
            });
          }}
          onHoverIn={event => {
            setHoveredIndex(index);
          }}
          onHoverOut={() => {
            setHoveredIndex(null);
          }}>
          <UserIcon user={user} width={32} height={32} />
          <Text className="text-white ml-2 font-semibold">{user.username}</Text>
        </Pressable>
      );
    } else {
      return null;
    }
  };

  return (
    <FlatList
      data={private_channels}
      renderItem={renderItem}
      keyExtractor={dm => dm.id}
      contentContainerStyle={{ padding: 8 }}
      showsVerticalScrollIndicator={false}
      ListHeaderComponent={() => {
        return (
          <View className="ml-1 my-2">
            <Text className="text-[#949BA4] text-xs font-medium">
              DIRECT MESSAGES
            </Text>
          </View>
        );
      }}
    />
  );
};

export default Friends;

Expected Behavior

No response

Actual Behavior

No response

Reproducible Demo

No response

Additional context

No response

Saadnajmi commented 1 year ago

I've personally found that Flatlist is quite slow on macOS and haven't completely root caused it quite yet. I wonder if the issue is more Flatlist than the actual prop. You might want to try just putting a bunch of views in a ScrollView to compare, or using an alternative like Flashlist.

neo773 commented 1 year ago

I've personally found that Flatlist is quite slow on macOS and haven't completely root caused it quite yet. I wonder if the issue is more Flatlist than the actual prop. You might want to try just putting a bunch of views in a ScrollView to compare, or using an alternative like Flashlist.

I've also found it slow but my current issue is with hover events it's just super slow in general. Rendering it in a normal list didn't make difference either. It's reproducible even with a single element try moving the cursor in and out quickly over it.

Saadnajmi commented 1 year ago

Side note, @neo773 are you a discord engineer? Or building a discord client using public APIs?

Saadnajmi commented 1 year ago

In a similar demo of a with lots of items that override onHoverIn and onHoverOut (which you can find here), I'm not able to repro the error. I may have a faster machine as well, but it feels like there's something inherently different between the two videos more than just hardware. From the attached video, it feels more like a symptom of extraneous re-renders or re-rendering the whole View instead of just the item for every hover. Could you try making a blank or simple app and reproducing, and/or try using something like why-did-you-render to verify the only re-rendered element on hover is the Pressable?

https://github.com/microsoft/react-native-macos/assets/6722175/920df043-34f6-4aec-8b60-e76ddce727d3

neo773 commented 1 year ago

So I looked at the test you referenced and it seems like it's rendering a native menu list I could not find any onHoverIn or onHoverOut use in the test code, I even went deeper into the component's code and didn't find any reference there either.

Anyways here's a minimal reproducible code

import { Text, FlatList } from 'react-native';
import React, { useState } from 'react';
import { Pressable, View } from 'react-native-macos';

const BugScreen = () => {
  const [hoveredIndex, setHoveredIndex] = useState<number | null>(null);

  const dummyData = Array.from({ length: 100 }, (_, index) => ({
    id: `dm-${index}`,
    user: {
      username: `User ${index}`,
    },
  }));

  const renderItem = ({
    item: item,
    index: index,
  }: {
    item: {
      id: string;
      user: {
        username: string;
      };
    };
    index: number;
  }) => {
    return (
      <Pressable
        style={{
          flexDirection: 'row',
          marginVertical: 2,
          alignItems: 'center',
          backgroundColor: index === hoveredIndex ? '#404249' : 'transparent',
          // @ts-expect-error
          cursor: 'pointer',
        }}
        onHoverIn={event => {
          setHoveredIndex(index);
        }}
        onHoverOut={() => {
          setHoveredIndex(null);
        }}>
        <Text style={{ color: 'black', marginLeft: 2, fontWeight: 'bold' }}>
          {item.user.username}
        </Text>
      </Pressable>
    );
  };

  return (
    <FlatList
      data={dummyData}
      renderItem={renderItem}
      keyExtractor={dm => dm.id}
      contentContainerStyle={{ padding: 8 }}
      showsVerticalScrollIndicator={false}
      ListHeaderComponent={() => {
        return (
          <View style={{ marginLeft: 1, marginVertical: 2 }}>
            <Text style={{ color: '#949BA4', fontSize: 12, fontWeight: '500' }}>
              DIRECT MESSAGES
            </Text>
          </View>
        );
      }}
    />
  );
};

export default BugScreen;

Side note, @neo773 are you a discord engineer? Or building a discord client using public APIs?

Nah, I'm building an open source client using private APIs

Saadnajmi commented 1 year ago

@neo773 not a native menu, a React Native drop in replacement for a menu, which each item being JS (I worked on it 🙂). 'useMenuItem' would be where where hover is used, but the code is convoluted, so don't feel the need to look deeper.

I'll look at your example code when I can!

Saadnajmi commented 1 year ago

@neo773 not a native menu, a React Native drop in replacement for a menu, which each item being JS (I worked on it 🙂). 'useMenuItem' would be where where hover is used, but the code is convoluted, so don't feel the need to look deeper.

I'll look at your example code when I can!

I'll try running your example code when I can. However just from looking at it, the useState for the hook is top level, and will re-render the whole Flatlist every time. Try using "listItemComponent' instead of renderItem (not well documented, but it's basically renderItem with better support for hooks), and see if you can perhaps wrap the item in a call to React.useMemo() to prevent unnecessary renders?

It's a busy week for me, but I'll try running your code when I can too.

neo773 commented 1 year ago

Tried the ListItemComponent prop and memoizing the item, now it just skips lot of frames

It's a busy week for me, but I'll try running your code when I can too.

I get it, take your time and thanks for looking into it.

https://github.com/microsoft/react-native-macos/assets/62795688/16d48897-0a23-464e-887d-d9e21ec11fd8