From 8c1c42bc5a2e2a9ceba759e3389489173e1fbf0b Mon Sep 17 00:00:00 2001
From: Robin Malfait <malfait.robin@gmail.com>
Date: Thu, 14 Mar 2024 22:17:50 +0100
Subject: [PATCH] Prefer incoming `data-*` attributes, over the ones set by
 Headless UI (#3035)

* prefer `data-*` props that already exist

If the component already had `data-foo`, and we set `data-foo` then the
`data-foo` that was already there should stay.

* update changelog
---
 packages/@headlessui-react/CHANGELOG.md       |  1 +
 .../utils/__snapshots__/render.test.tsx.snap  | 29 ++++++++++++
 .../src/utils/render.test.tsx                 | 44 +++++++++++++++++++
 .../@headlessui-react/src/utils/render.ts     | 27 +++++++++++-
 4 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
index a3bd3dddd7..8ce1ce1f50 100644
--- a/packages/@headlessui-react/CHANGELOG.md
+++ b/packages/@headlessui-react/CHANGELOG.md
@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Attempt form submission when pressing `Enter` on `Checkbox` component ([#2962](https://github.com/tailwindlabs/headlessui/pull/2962))
 - Allow setting custom `tabIndex` on the `<Switch />` component ([#2966](https://github.com/tailwindlabs/headlessui/pull/2966))
 - Forward `disabled` state to hidden inputs in form-like components ([#3004](https://github.com/tailwindlabs/headlessui/pull/3004))
+- Prefer incoming `data-*` attributes, over the ones set by Headless UI ([#3035](https://github.com/tailwindlabs/headlessui/pull/3035))
 
 ### Changed
 
diff --git a/packages/@headlessui-react/src/utils/__snapshots__/render.test.tsx.snap b/packages/@headlessui-react/src/utils/__snapshots__/render.test.tsx.snap
index ea55f73b01..fc97ee6a51 100644
--- a/packages/@headlessui-react/src/utils/__snapshots__/render.test.tsx.snap
+++ b/packages/@headlessui-react/src/utils/__snapshots__/render.test.tsx.snap
@@ -109,6 +109,22 @@ exports[`Default functionality should forward all the props to the first child w
 </div>"
 `;
 
+exports[`Default functionality should forward boolean values from \`slot\` as data attributes 1`] = `
+"<div
+  data-testid=\\"wrapper\\"
+>
+  <div
+    data-a=\\"\\"
+    data-c=\\"\\"
+    data-headlessui-state=\\"a c\\"
+  >
+    <span>
+      Contents
+    </span>
+  </div>
+</div>"
+`;
+
 exports[`Default functionality should not error when we are rendering a Fragment with multiple children when we don't passthrough additional props 1`] = `
 "<div
   data-testid=\\"wrapper\\"
@@ -122,6 +138,19 @@ exports[`Default functionality should not error when we are rendering a Fragment
 </div>"
 `;
 
+exports[`Default functionality should prefer user provided data attributes over the ones we set automatically 1`] = `
+"<div
+  data-testid=\\"wrapper\\"
+>
+  <span
+    data-accept=\\"always\\"
+    data-headlessui-state=\\"accept\\"
+  >
+    Contents
+  </span>
+</div>"
+`;
+
 exports[`Features.RenderStrategy Hidden render strategy should be possible to render an \`unmount={false}\` dummy component (show = false) 1`] = `
 "<div
   data-testid=\\"wrapper\\"
diff --git a/packages/@headlessui-react/src/utils/render.test.tsx b/packages/@headlessui-react/src/utils/render.test.tsx
index 48509cd2bc..f6844c5751 100644
--- a/packages/@headlessui-react/src/utils/render.test.tsx
+++ b/packages/@headlessui-react/src/utils/render.test.tsx
@@ -153,6 +153,50 @@ describe('Default functionality', () => {
     expect(contents()).toMatchSnapshot()
   })
 
+  it('should forward boolean values from `slot` as data attributes', () => {
+    function Dummy<TTag extends ElementType = 'div'>(
+      props: Props<TTag> & Partial<{ a: any; b: any; c: any }>
+    ) {
+      return (
+        <div data-testid="wrapper">
+          {render({
+            ourProps: {},
+            theirProps: props,
+            slot: { a: true, b: false, c: true },
+            defaultTag: 'div',
+            name: 'Dummy',
+          })}
+        </div>
+      )
+    }
+
+    testRender(<Dummy>{() => <span>Contents</span>}</Dummy>)
+
+    expect(contents()).toMatchSnapshot()
+  })
+
+  it('should prefer user provided data attributes over the ones we set automatically', () => {
+    function Dummy<TTag extends ElementType = 'div'>(
+      props: Props<TTag> & Partial<{ a: any; b: any; c: any }>
+    ) {
+      return (
+        <div data-testid="wrapper">
+          {render({
+            ourProps: {},
+            theirProps: props,
+            slot: { accept: true },
+            defaultTag: 'div',
+            name: 'Dummy',
+          })}
+        </div>
+      )
+    }
+
+    testRender(<Dummy as={Fragment}>{() => <span data-accept="always">Contents</span>}</Dummy>)
+
+    expect(contents()).toMatchSnapshot()
+  })
+
   it(
     'should error when we are rendering a Fragment with multiple children',
     suppressConsoleLogs(() => {
diff --git a/packages/@headlessui-react/src/utils/render.ts b/packages/@headlessui-react/src/utils/render.ts
index 56895f658e..85357b8d5f 100644
--- a/packages/@headlessui-react/src/utils/render.ts
+++ b/packages/@headlessui-react/src/utils/render.ts
@@ -213,12 +213,35 @@ function _render<TTag extends ElementType, TSlot>(
 
         let classNameProps = newClassName ? { className: newClassName } : {}
 
+        // Merge props from the existing element with the incoming props
+        let mergedProps = mergePropsAdvanced(
+          resolvedChildren.props as any,
+          // Filter out undefined values so that they don't override the existing values
+          compact(omit(rest, ['ref']))
+        )
+
+        // Make sure that `data-*` that already exist in the `mergedProps` are
+        // skipped.
+        //
+        // Typically we want to keep the props we set in each component because
+        // they are required to make the component work correctly. However, in
+        // case of `data-*` attributes, these are attributes that help the end
+        // user.
+        //
+        // This means that since the props are not required for the component to
+        // work, that we can safely prefer the `data-*` attributes from the
+        // component that the end user provided.
+        for (let key in dataAttributes) {
+          if (key in mergedProps) {
+            delete dataAttributes[key]
+          }
+        }
+
         return cloneElement(
           resolvedChildren,
           Object.assign(
             {},
-            // Filter out undefined values so that they don't override the existing values
-            mergePropsAdvanced(resolvedChildren.props as any, compact(omit(rest, ['ref']))),
+            mergedProps,
             dataAttributes,
             refRelatedProps,
             { ref: mergeRefs((resolvedChildren as any).ref, refRelatedProps.ref) },