Bug fix: ButtonGroup with Tooltips (#5123)

* messy stuff

* fix loading button group styles

* lint

* Create clean-camels-train.md

* remove button changes

* fix(TooltipV2): check for interactive content two levels down

* empty case

* add test

* lint

* test sxprop change

* revert for separate pr

* test(vrt): update snapshots

* test(vrt): update snapshots

---------

Co-authored-by: Armagan Ersoz <[email protected]>
Co-authored-by: Marie Lucca <[email protected]>
Co-authored-by: langermank <[email protected]>
Co-authored-by: Jon Rohan <[email protected]>

.changeset/clean-camels-train.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+Bug fix: ButtonGroup with Tooltip border-radius is incorrect

e2e/components/ButtonGroup.test.ts

@@ -114,4 +114,32 @@ test.describe('ButtonGroup', () => {
       })
     }
   })
+
+  test.describe('Overrides', () => {
+    for (const theme of themes) {
+      test.describe(theme, () => {
+        test('default @vrt', async ({page}) => {
+          await visit(page, {
+            id: 'components-buttongroup-devonly--link-button-with-icon-buttons',
+            globals: {
+              colorScheme: theme,
+            },
+          })
+
+          // Default state
+          expect(await page.screenshot()).toMatchSnapshot(`ButtonGroup.Overrides.${theme}.png`)
+        })
+
+        test('axe @aat', async ({page}) => {
+          await visit(page, {
+            id: 'components-buttongroup-devonly--link-button-with-icon-buttons',
+            globals: {
+              colorScheme: theme,
+            },
+          })
+          await expect(page).toHaveNoViolations()
+        })
+      })
+    }
+  })
 })

packages/react/src/ButtonGroup/ButtonGroup.dev.stories.tsx

@@ -0,0 +1,65 @@
+import React from 'react'
+import type {Meta} from '@storybook/react'
+import ButtonGroup from './ButtonGroup'
+import {Button, IconButton, LinkButton} from '../Button'
+import {CopilotIcon} from '@primer/octicons-react'
+import {Box, Tooltip, ThemeProvider, BaseStyles} from '..'
+
+const meta: Meta<typeof ButtonGroup> = {
+  title: 'Components/ButtonGroup/DevOnly',
+  component: ButtonGroup,
+  decorators: [
+    Story => {
+      // Add some padding to the wrapper box to make sure tooltip v1 is always in the viewport
+      return (
+        <ThemeProvider>
+          <BaseStyles>
+            <Box padding={5}>{Story()}</Box>
+          </BaseStyles>
+        </ThemeProvider>
+      )
+    },
+  ],
+}
+
+export default meta
+
+export const LinkAndButtonWithTooltip2 = () => (
+  <ButtonGroup sx={{pl: 2}}>
+    <Tooltip text="Additional info about the link">
+      <Button as="a" href="https://primer.style">
+        Link
+      </Button>
+    </Tooltip>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+  </ButtonGroup>
+)
+
+export const ButtonAndLinkWithTooltip2 = () => (
+  <ButtonGroup sx={{pl: 2}}>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <Tooltip text="Additional info about the link">
+      <Button as="a" href="https://primer.style">
+        Link
+      </Button>
+    </Tooltip>
+  </ButtonGroup>
+)
+
+export const ButtonGroupSingleButton = () => (
+  <ButtonGroup>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <div></div>
+  </ButtonGroup>
+)
+
+export const LinkButtonWithIconButtons = () => (
+  <ButtonGroup>
+    <LinkButton size="small" sx={{color: 'deeppink'}} href="https://primer.style">
+      Small link
+    </LinkButton>
+    <Button className="testCustomClassnameColor">Pink link</Button>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+  </ButtonGroup>
+)

packages/react/src/ButtonGroup/ButtonGroup.features.stories.tsx

@@ -1,10 +1,11 @@
-import React, {useState} from 'react'
+import React from 'react'
 import type {Meta} from '@storybook/react'
 import ButtonGroup from './ButtonGroup'
 import {IconButton, Button} from '../Button'
 import {PlusIcon, DashIcon, TriangleDownIcon} from '@primer/octicons-react'
 import {ActionMenu} from '../ActionMenu'
 import {ActionList} from '../ActionList'
+import {Tooltip} from '../next'
 
 export default {
   title: 'Components/ButtonGroup/Features',
@@ -14,26 +15,84 @@ export default {
 export const IconButtons = () => (
   <ButtonGroup>
     {/* We can remove these unsafe props after we resolve https://github.com/primer/react/issues/4129 */}
-    {/* eslint-disable-next-line primer-react/a11y-remove-disable-tooltip */}
-    <IconButton unsafeDisableTooltip={true} icon={PlusIcon} aria-label="Add" />
-    {/* eslint-disable-next-line primer-react/a11y-remove-disable-tooltip */}
-    <IconButton unsafeDisableTooltip={true} icon={DashIcon} aria-label="Subtract" />
+    <IconButton icon={PlusIcon} aria-label="Add" />
+    <IconButton icon={DashIcon} aria-label="Subtract" />
   </ButtonGroup>
 )
 
 export const LoadingButtons = () => {
-  const [isLoading, setIsLoading] = useState(false)
-
-  const handleClick = () => {
-    setIsLoading(true)
-  }
+  const handleClick = () => {}
   return (
     <ButtonGroup>
-      <Button loading={isLoading} onClick={handleClick}>
+      <Button loading={true} onClick={handleClick}>
         Button 1
       </Button>
       <Button onClick={handleClick}>Button 2</Button>
-      <Button onClick={handleClick}>Button 3</Button>
+      <Tooltip text="Additional info about the button">
+        <Button onClick={handleClick}>Button 3</Button>
+      </Tooltip>
+    </ButtonGroup>
+  )
+}
+
+export const IconButtonsWithTooltip = () => (
+  <ButtonGroup>
+    <IconButton icon={PlusIcon} aria-label="Add" />
+    <IconButton icon={DashIcon} aria-label="Subtract" />
+  </ButtonGroup>
+)
+export const ButtonAndLink = () => (
+  <ButtonGroup>
+    <Button>Button</Button>
+    <Button as="a" href="https://primer.style">
+      Link
+    </Button>
+  </ButtonGroup>
+)
+
+export const LinksWithTooltip = () => (
+  <ButtonGroup>
+    <Tooltip text="Additonal text for link 1" direction="s">
+      <Button as="a" href="https://primer.style">
+        Sample Link 1
+      </Button>
+    </Tooltip>
+    <Tooltip text="Additonal text for link 2" direction="s">
+      <Button as="a" href="https://primer.style">
+        Sample Link 2
+      </Button>
+    </Tooltip>
+  </ButtonGroup>
+)
+
+export const InactiveButtonsGroup = () => {
+  const primaryButton = (
+    <Button inactive={true} aria-disabled={true} onClick={() => {}}>
+      Primary Button
+    </Button>
+  )
+
+  const secondaryButton = (
+    <IconButton aria-label="Secondary Button Aria Label" aria-disabled={true} inactive={true} icon={DashIcon} />
+  )
+
+  return (
+    <ButtonGroup>
+      <Tooltip text="This button is inactive" direction="ne">
+        {primaryButton}
+      </Tooltip>
+
+      <ActionMenu open={false} onOpenChange={() => {}}>
+        <ActionMenu.Anchor>
+          <Tooltip text="this button is inactive" direction="ne" type="label">
+            {secondaryButton}
+          </Tooltip>
+        </ActionMenu.Anchor>
+        <ActionMenu.Overlay align="end">
+          <ActionList.Item>Item 1</ActionList.Item>
+          <ActionList.Item>Item 2</ActionList.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>
     </ButtonGroup>
   )
 }

packages/react/src/ButtonGroup/ButtonGroup.module.css

@@ -7,16 +7,27 @@
     /* stylelint-disable-next-line primer/spacing */
     margin-inline-end: -1px;
     position: relative;
-    border-radius: 0;
+
+    /* reset border-radius */
+    button,
+    a {
+      border-radius: 0;
+    }
 
     &:first-child {
-      border-top-left-radius: var(--borderRadius-medium);
-      border-bottom-left-radius: var(--borderRadius-medium);
+      button,
+      a {
+        border-top-left-radius: var(--borderRadius-medium);
+        border-bottom-left-radius: var(--borderRadius-medium);
+      }
     }
 
     &:last-child {
-      border-top-right-radius: var(--borderRadius-medium);
-      border-bottom-right-radius: var(--borderRadius-medium);
+      button,
+      a {
+        border-top-right-radius: var(--borderRadius-medium);
+        border-bottom-right-radius: var(--borderRadius-medium);
+      }
     }
 
     &:focus,
@@ -26,8 +37,32 @@
     }
   }
 
+  /* this is a workaround until portal based tooltips are fully removed from dotcom */
+  &:has(div:last-child:empty) {
+    button,
+    a {
+      border-radius: var(--borderRadius-medium);
+    }
+  }
+
   /* if child is loading button */
-  [data-loading-wrapper] {
+  & > *[data-loading-wrapper] {
+    /* stylelint-disable-next-line primer/spacing */
+    margin-inline-end: -1px;
+    position: relative;
+
+    /* reset border-radius */
+    button,
+    a {
+      border-radius: 0;
+    }
+
+    &:focus,
+    &:active,
+    &:hover {
+      z-index: 1;
+    }
+
     &:first-child {
       button,
       a {
@@ -44,17 +79,4 @@
       }
     }
   }
-
-  [data-loading-wrapper] > * {
-    /* stylelint-disable-next-line primer/spacing */
-    margin-inline-end: -1px;
-    position: relative;
-    border-radius: 0;
-
-    &:focus,
-    &:active,
-    &:hover {
-      z-index: 1;
-    }
-  }
 }