atlassian / pragmatic-drag-and-drop

Fast drag and drop for any experience on any tech stack
https://atlassian.design/components/pragmatic-drag-and-drop
Other
7.45k stars 133 forks source link

In desktop Safari drag preview can include elements behind container. #43

Closed IanBellomy closed 2 weeks ago

IanBellomy commented 3 weeks ago

In desktop Safari 17.4.1 a drag preview created by rendering an element into the provided container via react's createRoot can result in other elements in the top left being visually included in the preview image.

https://github.com/atlassian/pragmatic-drag-and-drop/assets/17239616/8f1ec797-cae3-4211-a2c4-16731bde7c5b

Repro: https://codesandbox.io/p/sandbox/safari-pdnd-preview-issue-react-2cl6k4?file=%2Fsrc%2FDragMe.jsx

Works fine in Chrome 124 and FF 125.

alexreardon commented 2 weeks ago

We are aware of this browser bug. *shakes fist at safari

We have a fix for this bug in setCustomNativeDragPreview(). We generally recommend folks use that function when working with native drag previews as it provides the most resiliant experience.

We didn't bake the setCustomNativeDragPreview into Pragmatic drag and drop (we made it an optional helper), because:

We have tried to make decisions that result in the lowest amount of code for all consumers.

Thanks for taking the time to raise this issue

IanBellomy commented 2 weeks ago

🤔 The repro uses setCustomNativeDragPreview...

export const DragMe = () => {
  const ref = useRef(null);
  useEffect(() => {
    const element = ref.current;
    draggable({
      element,
      onGenerateDragPreview(info) {
        setCustomNativeDragPreview({
          nativeSetDragImage: info.nativeSetDragImage,
          render({ container }) {
            const root = createRoot(container);
            root.render(<DragPreview />);
            return function cleanup() {
              root.unmount();
            };
          },
        });
      },
    });
  }, []);
alexreardon commented 2 weeks ago

I can reproduce the issue. It looks like our Safari fix is not working for this case (I can see that the container.left property is not being set correctly in this case). The container has a width of 0 which is messing up our logic. I suspect the fix will be fairly minor and i'll get on it asap 🚀

alexreardon commented 2 weeks ago

Our Safari workaround is working for react@16 (a forked example). This is because I think that the new createRoot().render() renders after a microtask. I'll need to adjust our fix to accomodate for that

alexreardon commented 2 weeks ago

I have a fix.

   if (isSafari()) {
-    const rect = container.getBoundingClientRect();
-    container.style.left = `-${rect.width - 0.0001}px`;
+    /**
+     *  For some frameworks (eg `react@18+` with `ReactDOM.createRoot().render()`) the rendering
+     *  inside the `container` does not occur until after a microtask.
+     **/
+    queueMicrotask(() => {
+      const rect = container.getBoundingClientRect();
+
+      // We cannot apply a fix if nothing has been rendered into the `container`
+      if (rect.width === 0) {
+        return;
+      }
+
+      container.style.left = `-${rect.width - 0.0001}px`;
+    });
   }

You can apply this fix on your end in the short term if you like

render({ container }) {
            const root = createRoot(container);
            root.render(<DragPreview />);

+            queueMicrotask(() => {
+              const rect = container.getBoundingClientRect();
+
+              // We cannot apply this fix if nothing has been rendered into the `container`
+              if (rect.width === 0) {
+                return;
+              }
+
+             container.style.left = `-${rect.width - 0.0001}px`;
+            });

            return function cleanup() {
              root.unmount();
            };
          },

https://codesandbox.io/p/sandbox/safari-pdnd-preview-issue-react-with-fix-gsw2jw?file=%2Fsrc%2FDragMe.jsx%3A63%2C29

alexreardon commented 2 weeks ago

@IanBellomy I have raised a pull request to our mono repo with a fix. I suspect this will be released within ~12 hours.

Thanks so much for raising this!

alexreardon commented 2 weeks ago

Fix merged. Should be released to npm soon