Skip to content

Performance degradation in onPress #8086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MrFlashAccount opened this issue Apr 12, 2025 · 18 comments
Open

Performance degradation in onPress #8086

MrFlashAccount opened this issue Apr 12, 2025 · 18 comments

Comments

@MrFlashAccount
Copy link

Provide a general summary of the issue here

After updating react-aria-components to the latest version (1.8.0) we faced a significant performance degradation, increasing passiveMountOnFiber duration from roughly 200ms to 1.8s(!)

After digging into the issue we found that the root cause is in usePress hook: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L821 caused by: #8047

Here's a recording from chrome devtools:

Trace-20250412T155902.json

Here's a screenshot, you can see that usePress schedules over 2000 style recalculations.

Image

🤔 Expected Behavior?


😯 Current Behavior


💁 Possible Solution


🔦 Context


🖥️ Steps to Reproduce


Version

1.8.0

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@devongovett
Copy link
Member

I was worried about this but we tested before and couldn't reproduce a slowdown. Is there anything in particular about your page that could cause this? High number of Pressable elements? Which components are you using? Anything that can help us reproduce would be helpful.

@MrFlashAccount
Copy link
Author

MrFlashAccount commented Apr 12, 2025

Thank you for the quick response!

Yeah, in this particular case, we have over 2,000 pressables. It may seem a bit more than you might expect, but for us, it’s the perfect stress test.

@nwidynski
Copy link
Contributor

nwidynski commented Apr 13, 2025

@devongovett Shouldn't the override in #8047 also be running on every render, making this degradation possibly worse? Currently the useEffect is dependent on domRef, but what if touch-action: auto were to apply conditionally?

@MrFlashAccount Unfortunately I can't reproduce the degradation just by rendering 2000 <Button />'s.

Maybe it makes sense to add a data-pressable attribute on each element instead? Optionally, we could then inject a global style with low specificity so user styles would still override. Ideally users would still just set <meta name="viewport" content="width=device-width"> whenever possible.

@MrFlashAccount
Copy link
Author

We have the ‘<meta name=“viewport” content=“width=device-width”>’ set. Is it possible to provide an option or simply query the meta once in a provider?

I can prepare a pull request to bump react-aria’s version in the meantime. However, you’d need to download an IDE and register. Also, since the product is open-source, it’s possible to explore the code.

@MrFlashAccount
Copy link
Author

MrFlashAccount commented Apr 13, 2025

Unfortunately I can't reproduce the degradation just by rendering 2000 '

it’s worth noting that we have a variety of styles, and some of them may be ineffective, such as ‘.some-selector span’ and over 25,000 nodes on the page.

@MrFlashAccount
Copy link
Author

MrFlashAccount commented Apr 14, 2025

@devongovett, @nwidynski , I’ve prepared a pull request (PR) where you can observe the degradation. Here’s the link: enso-org/enso#12846

To reproduce the issue, follow these steps:

  1. Download the IDE.
  2. Open the IDE.
  3. Register and add a large directory (preferably more than 2000 elements). A good candidate is the node_modules folder in the react-spectrum package.

Here’s the link to the job with the IDE artifacts: https://github.com/enso-org/enso/actions/runs/14442904237?pr=12846

Here's a video how to register.

CleanShot.2025-04-14.at.13.07.03.mp4

Additionally, I’ve uploaded a video demonstrating the reproduction of the issue (due to its large size, it had to be uploaded to YouTube):

https://youtu.be/Vxydt6b4Jes

@MrFlashAccount
Copy link
Author

MrFlashAccount commented Apr 14, 2025

@devongovett
Copy link
Member

You might need to add permissions for that code sandbox. I can't see it.

@MrFlashAccount
Copy link
Author

You might need to add permissions for that code sandbox. I can't see it.

Oh, I apologize for any inconvenience caused.

https://codesandbox.io/p/devbox/modern-architecture-forked-5tyh5y?workspaceId=ws_FJECc2QX6MEMWWYxSwGxqp

Hope now it works!

@nwidynski
Copy link
Contributor

nwidynski commented Apr 15, 2025

@MrFlashAccount Thanks for the repro 👍

While I can observe the forced reflow now, on my machine, the commitPassiveMountEffects only takes ~120ms?

@MrFlashAccount
Copy link
Author

@MrFlashAccount Thanks for the repro 👍

While I can observe the forced reflow now, on my machine, the commitPassiveMountEffects only takes ~120ms?

That's a lot, don't you think?

@devongovett
Copy link
Member

As @nwidynski mentioned, we could potentially try inserting a CSS rule for this and see if that helps. It would need to be in the lowest CSS @layer possible, with as low specificity as possible. Basically like this:

<style>
@layer {
  :where([data-react-aria-pressable]) {
    touch-action: pan-x pan-y pinch-zoom;
  }
}
</style>

If this rule is inserted at the start of the <head> it should be in the lowest possible layer. That way other CSS on the page could override it when needed.

@MrFlashAccount
Copy link
Author

I think ':where' + putting this as the topmost style tag might be enough

@uncvrd
Copy link

uncvrd commented Apr 22, 2025

I think I'm running in to some performance jank with usePress as well, but is because of a different useEffect (at least the line number is different) 🤔 Maybe I missed something but should I be adding a touch-action css attribute so that it doesn't have to apply this for me?

Image

This is the code the screenshot is referencing

  useEffect(() => {
    let element = domRef?.current;
    if (element && (element instanceof getOwnerWindow(element).Element)) {
      // Only apply touch-action if not already set by another CSS rule.
      let style = getOwnerWindow(element).getComputedStyle(element);
      if (style.touchAction === 'auto') {
        // touchAction: 'manipulation' is supposed to be equivalent, but in 
        // Safari it causes onPointerCancel not to fire on scroll.
        // https://bugs.webkit.org/show_bug.cgi?id=240917
        (element as HTMLElement).style.touchAction = 'pan-x pan-y pinch-zoom';
      }
    }
  }, [domRef]);

I'm noticing performance issues with 30 table rows with an href assigned to them and when I switch to the tab with the 30 rows there's a noticeable delay on switching the tab as the rows are mounted (which is where the performance output above is from).

<Table aria-label="Fan list segments">
                <TableHeader columns={columns}>{(column) => <Column {...column} />}</TableHeader>
                <TableBody items={items} dependencies={dependencies}>
                    {(item) => (
                        <Row columns={columns} href={"https://test.com"}>
                            {(column) => {
                                switch (column.id) {
                                    case "name":
                                        return <Cell typography={"h5"}>{item.name}</Cell>
                                    default:
                                        return <></>
                                }
                            }}
                        </Row>
                    )}
                </TableBody>
            </Table>

Just wanted to raise and maybe can add another repro, but mostly just wanted to ask if I should add a CSS attribute to prevent the use effect from doing this for me? Thanks a lot for this library : )

Edit: here's a quick video showing this useEffect running on mount for each "cell" in my table and applying the style

https://streamable.com/aa19sz

@nwidynski
Copy link
Contributor

nwidynski commented Apr 22, 2025

@uncvrd Yeah, it appears to be the same issue. You can try setting touch-action manually for now:

.react-aria-Cell {
  touch-action: pan-x pan-y pinch-zoom;
}

or possibly even globally, depending on your circumstances

html {
  touch-action: pan-x pan-y pinch-zoom;
}

See https://developer.chrome.com/blog/300ms-tap-delay-gone-away for more info.

@MrFlashAccount
Copy link
Author

@uncvrd Yeah, it appears to be the same issue. You can try setting touch-action manually for now:

.react-aria-Cell {
  touch-action: pan-x pan-y pinch-zoom;
}

or possibly even globally, depending on your circumstances

html {
  touch-action: pan-x pan-y pinch-zoom;
}

See https://developer.chrome.com/blog/300ms-tap-delay-gone-away for more info.

Unfortunately this won't help, because the slowest part is the 'getComputedStyles' which is executed in either case.

@uncvrd
Copy link

uncvrd commented Apr 25, 2025

yea agreed, it did help a little bit but most of the performance hit comes from the getComputedStyles execution

@MrFlashAccount
Copy link
Author

@devongovett @nwidynski

It seems tailwind significantly slows down the style recalculation step: https://www.developerway.com/posts/tailwind-vs-linaria-performance

So the 'gcs' significantly affects the projects with tailwind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants