Update the default value of unsafeDisableTooltip to false and remove redundant props and wrappers (#4702)

* Update the default value of unsafeDisableTooltip to false and remove unncessary props and wrappers

* test and lint fix

* Create clean-fireants-type.md

* update actionbar e2e test selector

* disable tooltips in button group stories

* update Dialog tests

* fix linting

* update tests

Author: broccolinisoup

.changeset/clean-fireants-type.md

@@ -0,0 +1,8 @@
+---
+"@primer/react": minor
+---
+
+IconButton: Enable tooltips by default in icon buttons by updating the default value of `unsafeDisableTooltip` to `false`.
+
+This is a behaviour change in icon buttons, please upgrade with a caution.  
+

e2e/components/drafts/ActionBar.test.ts

@@ -44,8 +44,8 @@ test.describe('ActionBar', () => {
           await expect(page.locator(toolbarButtonSelector)).toHaveCount(10)
           await page.setViewportSize({width: viewports['primer.breakpoint.xs'], height: 768})
           await expect(page.locator(toolbarButtonSelector)).toHaveCount(6)
-          const moreButtonSelector = `button[aria-label="More Comment box toolbar items"]`
-          await page.locator(moreButtonSelector).click()
+          const moreButtonSelector = page.getByLabel('More Comment box toolbar items')
+          await moreButtonSelector.click()
           await expect(page.locator('ul[role="menu"]>li')).toHaveCount(5)
         })
       })

packages/react/src/ActionBar/ActionBar.tsx

@@ -299,7 +299,7 @@ export const ActionBarIconButton = forwardRef((props: ActionBarIconButtonProps,
     const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect()
     setChildrenWidth({text, width: domRect.width})
   }, [ref, setChildrenWidth])
-  return <IconButton ref={ref} size={size} {...props} variant="invisible" unsafeDisableTooltip={false} />
+  return <IconButton ref={ref} size={size} {...props} variant="invisible" />
 })
 
 const sizeToHeight = {

packages/react/src/ActionList/TrailingAction.tsx

@@ -48,7 +48,6 @@ export const TrailingAction = forwardRef(({as = 'button', icon, label, href = nu
           aria-label={label}
           icon={icon}
           variant="invisible"
-          unsafeDisableTooltip={false}
           tooltipDirection="w"
           href={href}
           // @ts-expect-error StyledButton wants both Anchor and Button refs

packages/react/src/Button/IconButton.tsx

@@ -17,7 +17,7 @@ const IconButton = forwardRef(
       disabled,
       tooltipDirection,
       // This is planned to be a temporary prop until the default tooltip on icon buttons are fully rolled out.
-      unsafeDisableTooltip = true,
+      unsafeDisableTooltip = false,
       ...props
     },
     forwardedRef,

packages/react/src/Button/__tests__/Button.test.tsx

@@ -131,10 +131,11 @@ describe('Button', () => {
     const tooltipEl = getByText('Love is all around')
     expect(triggerEL).toHaveAttribute('aria-describedby', tooltipEl.id)
   })
-  it('should not render tooltip on an icon button by default', () => {
-    const {getByRole} = render(<IconButton icon={HeartIcon} aria-label="Heart" />)
+  it('should render tooltip on an icon button by default', () => {
+    const {getByRole, getByText} = render(<IconButton icon={HeartIcon} aria-label="Heart" />)
     const triggerEl = getByRole('button')
-    expect(triggerEl).not.toHaveAttribute('aria-labelledby')
-    expect(triggerEl).toHaveAttribute('aria-label', 'Heart')
+    const tooltipEl = getByText('Heart')
+    expect(triggerEl).toHaveAttribute('aria-labelledby', tooltipEl.id)
+    expect(triggerEl).not.toHaveAttribute('aria-label')
   })
 })

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

@@ -11,7 +11,8 @@ export default {
 
 export const IconButtons = () => (
   <ButtonGroup>
-    <IconButton icon={PlusIcon} aria-label="Add" />
-    <IconButton icon={DashIcon} aria-label="Subtract" />
+    {/* We can remove these unsafe props after we resolve https://github.com/primer/react/issues/4129 */}
+    <IconButton unsafeDisableTooltip={true} icon={PlusIcon} aria-label="Add" />
+    <IconButton unsafeDisableTooltip={true} icon={DashIcon} aria-label="Subtract" />
   </ButtonGroup>
 )

packages/react/src/Dialog.tsx

@@ -11,6 +11,7 @@ import type {ComponentProps} from './utils/types'
 import {useRefObjectAsForwardedRef} from './hooks/useRefObjectAsForwardedRef'
 import {XIcon} from '@primer/octicons-react'
 
+// Dialog v1
 const noop = () => null
 
 type StyledDialogBaseProps = {

packages/react/src/__tests__/Dialog.test.tsx

@@ -1,6 +1,5 @@
 import React, {useState, useRef} from 'react'
-import {Dialog, Box, Text} from '..'
-import {Button} from '../deprecated'
+import {Dialog, Box, Text, Button} from '..'
 import {render as HTMLRender, fireEvent} from '@testing-library/react'
 import axe from 'axe-core'
 import {behavesAsComponent, checkExports} from '../utils/testing'
@@ -71,6 +70,36 @@ const DialogWithCustomFocusRef = () => {
   )
 }
 
+const DialogWithCustomFocusRefAndReturnFocusRef = () => {
+  const [isOpen, setIsOpen] = useState(true)
+  const returnFocusRef = useRef(null)
+  const buttonRef = useRef(null)
+  return (
+    <div>
+      <Button data-testid="trigger-button" ref={returnFocusRef} onClick={() => setIsOpen(true)}>
+        Show Dialog
+      </Button>
+      <Dialog
+        returnFocusRef={returnFocusRef}
+        isOpen={isOpen}
+        initialFocusRef={buttonRef}
+        onDismiss={() => setIsOpen(false)}
+        aria-labelledby="header"
+      >
+        <div data-testid="inner">
+          <Dialog.Header id="header">Title</Dialog.Header>
+          <Box p={3}>
+            <Text fontFamily="sans-serif">Some content</Text>
+            <button data-testid="inner-button" ref={buttonRef}>
+              hi
+            </button>
+          </Box>
+        </div>
+      </Dialog>
+    </div>
+  )
+}
+
 describe('Dialog', () => {
   // because Dialog returns a React fragment the as and sx tests fail always, so they are skipped
   behavesAsComponent({
@@ -140,7 +169,9 @@ describe('Dialog', () => {
   })
 
   it('Returns focus to returnFocusRef on escape', async () => {
-    const {getByTestId, queryByTestId} = HTMLRender(<Component />)
+    const {getByTestId, queryByTestId} = HTMLRender(<DialogWithCustomFocusRefAndReturnFocusRef />)
+    const innerButton = getByTestId('inner-button')
+    expect(document.activeElement).toEqual(innerButton)
 
     expect(getByTestId('inner')).toBeTruthy()
     fireEvent.keyDown(document.body, {key: 'Escape'})

packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx

@@ -16,6 +16,8 @@ export const ToolbarButton = forwardRef<HTMLButtonElement, IconButtonProps>((pro
       onMouseDown={(e: React.MouseEvent) => e.preventDefault()}
       {...props}
       sx={{color: 'fg.muted', ...props.sx}}
+      // Keeping the tooltip disable since it is not maintained anymore and its tests were failing.
+      unsafeDisableTooltip
     />
   )
 })