unovue / radix-vue

Vue port of Radix UI Primitives. An open-source UI component library for building high-quality, accessible design systems and web apps.
https://radix-vue.com
MIT License
3.64k stars 226 forks source link

[Bug]: Dialog causes layout reflow which causes bad performance for large DOM trees #786

Open Mokkapps opened 7 months ago

Mokkapps commented 7 months ago

Environment

Development/Production OS: Linux / StackBlitz
Node version: v18.18.0
Package manager: npm@10.2.3
Radix Vue version: 1.5.3
Vue version: 3.4.15
CSS framework: tailwindcss@3.4.1
Browser: 123.0.6312.59 (Official Build) (arm64)

Link to minimal reproduction

https://stackblitz.com/edit/vitejs-vite-p7dnnw?file=src%2FApp.vue

Steps to reproduce

Describe the bug

Dialog opens very slow on slow devices (emulated via 6x CPU slowdown via browser DevTools) on a page with many DOM nodes.

Performance profiler indicates that a layout reflow is triggered.

Expected behavior

Dialog opens smoothly on slow devices with many DOM nodes and does not trigger a layout reflow.

Context & Screenshots (if applicable)

screenshot-2024-03-25-06 54 12

zernonia commented 4 months ago

Intend to look deeper into this and hopefully able to improve the performance on this in v2 😁

Mokkapps commented 4 months ago

Intend to look deeper into this and hopefully able to improve the performance on this in v2 😁

That would be amazing, thanks 👍🏻

zernonia commented 3 months ago

There's a couple factor that causes the layout reflow:

  1. aria-hidden - we need to hide away content outside of modal for screen reader
  2. dismissableLayer - we need to set pointerEvent to none in body
  3. bodyScrollLock - we need this to prevent document to be scrollable when modal is opened

Unfortunately, I can't find a reliable work around that wouldn't sacrifice a11y and UX 😬.

cyyynthia commented 3 months ago

I've looked a bit into this and it seems the largest contributor to the poor performance is pointer-events. While the "obvious" workaround for dialogs is to just not set it at all (the overlay is already swallowing any interaction outside of the dialog, so defining pointer events here is redundant)

However this remains an issue for any component that uses the DismissableLayer and can't use this trick (i.e. they are not teleported at root where they can use an invisible div that swallows all pointer interactions instead of the content below it). FWIW, I recall React Aria does not rely on pointer-events either and use an invisible div.

While this requires everything to be teleported, this may actually simplify a bunch of logic: listening to events outside of the layer is a matter of listening to events on this div, and the dialog overlay gets implemented "for free". The layer being responsible for the teleportation this also means all Teleport components can be dropped and the to property moved to the element itself 🤔

Mokkapps commented 3 months ago

There's a couple factor that causes the layout reflow:

  1. aria-hidden - we need to hide away content outside of modal for screen reader
  2. dismissableLayer - we need to set pointerEvent to none in body
  3. bodyScrollLock - we need this to prevent document to be scrollable when modal is opened

Unfortunately, I can't find a reliable work around that wouldn't sacrifice a11y and UX 😬.

Thanks for the update.

I think, it would be helpful if we could disable some of the features via props to prevent the layout reflow for certain components that cause these performance problems.