PageHeader: Fixes layout issues on title and sub components (#4669)

* layout shift issue resolve

* do not inherit font and line styles from parent for description and navigation slot

* font initial size and line hight only weight

* set font styles to sub components

* style update

* test fix

* Create two-hotels-attend.md

* test(vrt): update snapshots

* update line height to medium

* test(vrt): update snapshots

* breadcrumb line height update

* test(vrt): update snapshots

---------

Co-authored-by: broccolinisoup <broccolinisoup@users.noreply.github.com>

.changeset/two-hotels-attend.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+PageHeader: Resolve layout shift issues on Title and Actions

packages/react/src/PageHeader/PageHeader.examples.stories.tsx

@@ -110,7 +110,7 @@ export const PullRequestPage = () => (
       </PageHeader.Actions>
       <PageHeader.Description>
         <StateLabel status="pullOpened">Open</StateLabel>
-        <Text sx={{fontSize: 1, color: 'fg.muted'}}>
+        <Text sx={{color: 'fg.muted'}}>
           <Link href="https://github.com/broccolinisoup" sx={{fontWeight: 'bold'}}>
             broccolinisoup
           </Link>{' '}
@@ -148,7 +148,7 @@ export const FilesPage = () => (
   <Box sx={{padding: 3}}>
     <PageHeader>
       <PageHeader.TitleArea sx={{alignItems: 'center'}}>
-        <Text sx={{color: 'rgb(101, 109, 118)', fontSize: '14px'}}>/</Text>
+        <Text sx={{color: 'rgb(101, 109, 118)', fontSize: '14px', fontWeight: 'normal'}}>/</Text>
         <PageHeader.Title as="h1" sx={{fontSize: '14px', height: '21px'}}>
           PageHeader.tsx
         </PageHeader.Title>

packages/react/src/PageHeader/PageHeader.test.tsx

@@ -145,7 +145,7 @@ describe('PageHeader', () => {
         <PageHeader.ContextArea>ContextArea</PageHeader.ContextArea>
       </PageHeader>,
     )
-    expect(getByText('Title')).toHaveStyle('font-size: 32px')
+    expect(getByText('Title')).toHaveStyle('font-size: 1.5em')
   })
   it('renders "aria-label" prop when Navigation is rendered as "nav" landmark', () => {
     const {getByLabelText, getByText} = render(

packages/react/src/PageHeader/PageHeader.tsx

@@ -1,4 +1,4 @@
-import React, {useEffect, useState} from 'react'
+import React, {useEffect} from 'react'
 import {Box} from '..'
 import type {ResponsiveValue} from '../hooks/useResponsiveValue'
 import {useResponsiveValue} from '../hooks/useResponsiveValue'
@@ -37,9 +37,6 @@ const CONTEXT_AREA_REGION_ORDER = {
   ContextAreaActions: 2,
 }
 
-const MEDIUM_TITLE_HEIGHT = '2rem'
-const LARGE_TITLE_HEIGHT = '3rem'
-
 // Types that are shared between PageHeader children components
 export type ChildrenPropTypes = {
   className?: string
@@ -80,26 +77,31 @@ const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeader
       'description description description description description'
       'navigation navigation navigation navigation navigation'
     `,
-      //  --custom-height is a custom property (passed by sx) that can be used to override the set height.
+      // line-height is calculated with calc(height/font-size) and the below numbers are from @primer/primitives
+      //  --custom-font-size, --custom-line-height, --custom-font-weight are custom properties (passed by sx) that can be used to override the below values
       // We don't want these values to be overriden but still want to allow consumers to override them if needed.
-      '&[data-size-variant="large"]': {
-        '[data-component="PH_LeadingAction"], [data-component="PH_TrailingAction"],[data-component="PH_Actions"], [data-component="PH_LeadingVisual"], [data-component="PH_TrailingVisual"]':
-          {
-            height: `var(--custom-height, ${LARGE_TITLE_HEIGHT})`,
-          },
+      '&:has([data-component="TitleArea"][data-size-variant="large"])': {
+        fontSize: 'var(--custom-font-size, var(--text-title-size-large, 2rem))',
+        lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-large, 1.5))', // calc(48/32)
+        fontWeight: 'var(--custom-font-weight, var(--base-text-weight-normal, 400))',
+        '--title-line-height': 'var(--custom-line-height, var(--text-title-lineHeight-large, 1.5))',
       },
-      '&[data-size-variant="medium"]': {
-        '[data-component="PH_LeadingAction"], [data-component="PH_TrailingAction"],[data-component="PH_Actions"], [data-component="PH_LeadingVisual"], [data-component="PH_TrailingVisual"]':
-          {
-            height: `var(--custom-height, ${MEDIUM_TITLE_HEIGHT})`,
-          },
+      '&:has([data-component="TitleArea"][data-size-variant="medium"])': {
+        fontSize: 'var(--custom-font-size, var(--text-title-size-medium, 1.25rem))',
+        lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))', // calc(32/20)
+        fontWeight: 'var(--custom-font-weight, var(--base-text-weight-semibold, 600))',
+        '--title-line-height': 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))',
       },
-      '&[data-size-variant="subtitle"]': {
-        '[data-component="PH_LeadingAction"], [data-component="PH_TrailingAction"],[data-component="PH_Actions"], [data-component="PH_LeadingVisual"], [data-component="PH_TrailingVisual"]':
-          {
-            height: `var(--custom-height, ${MEDIUM_TITLE_HEIGHT})`,
-          },
+      '&:has([data-component="TitleArea"][data-size-variant="subtitle"])': {
+        fontSize: 'var(--custom-font-size, var(--text-title-size-medium, 1.25rem))',
+        lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))', // calc(32/20)
+        fontWeight: 'var(--custom-font-weight, var(--base-text-weight-normal, 400))',
+        '--title-line-height': 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))',
       },
+      '[data-component="PH_LeadingAction"], [data-component="PH_TrailingAction"],[data-component="PH_Actions"], [data-component="PH_LeadingVisual"], [data-component="PH_TrailingVisual"]':
+        {
+          height: 'calc(var(--title-line-height) * 1em)',
+        },
     }
 
     const rootRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
@@ -113,58 +115,50 @@ const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeader
       )
     }
 
-    const [hasContextArea, setHasContextArea] = useState(false)
-    const [hasLeadingAction, setHasLeadingAction] = useState(false)
-    const [titleVariant, setTitleVariant] = useState<string | undefined>('')
+    useEffect(
+      function validateInteractiveElementsInTitle() {
+        if (!__DEV__) return
 
-    useEffect(() => {
-      if (!rootRef.current || rootRef.current.children.length <= 0) return
-      const titleArea = Array.from(rootRef.current.children as HTMLCollection).find(child => {
-        return child instanceof HTMLElement && child.getAttribute('data-component') === 'TitleArea'
-      })
+        let hasContextArea = false
+        let hasLeadingAction = false
 
-      // It is very unlikely to have a PageHeader without a TitleArea, but we still want to make sure we don't break the page if that happens.
-      if (!titleArea) return
+        if (!rootRef.current || rootRef.current.children.length <= 0) return
+        const titleArea = Array.from(rootRef.current.children as HTMLCollection).find(child => {
+          return child instanceof HTMLElement && child.getAttribute('data-component') === 'TitleArea'
+        })
 
-      // // grab the data-size-variant attribute from the titleArea
-      const sizeVariant = titleArea.getAttribute('data-size-variant')
-      setTitleVariant(sizeVariant as string)
+        // It is very unlikely to have a PageHeader without a TitleArea, but we still want to make sure we don't break the page if that happens.
+        if (!titleArea) return
 
-      for (const child of React.Children.toArray(children)) {
-        if (React.isValidElement(child) && child.type === ContextArea) {
-          setHasContextArea(true)
-        }
-        if (React.isValidElement(child) && child.type === LeadingAction) {
-          setHasLeadingAction(true)
+        for (const child of React.Children.toArray(children)) {
+          if (React.isValidElement(child) && child.type === ContextArea) {
+            hasContextArea = true
+          }
+          if (React.isValidElement(child) && child.type === LeadingAction) {
+            hasLeadingAction = true
+          }
         }
-      }
-      // Check if TitleArea has any interactive children or grandchildren.
-      const hasInteractiveContent = Array.from(titleArea.childNodes).some(child => {
-        return (
-          (child instanceof HTMLElement && isInteractive(child)) ||
-          Array.from(child.childNodes).some(child => {
-            return child instanceof HTMLElement && isInteractive(child)
-          })
-        )
-      })
-      // PageHeader.TitleArea should be the first element in the DOM.
-      // Motivation behind this rule to make sure context area and leading action (if they exist) are always rendered after the title (a heading tag)
-      // so that screen reader users who are navigating via heading menu won't miss these actions.
-      if (hasContextArea || hasLeadingAction) {
+        // Check if TitleArea has any interactive children or grandchildren.
+        const hasInteractiveContent = Array.from(titleArea.childNodes).some(child => {
+          return (
+            (child instanceof HTMLElement && isInteractive(child)) ||
+            Array.from(child.childNodes).some(child => {
+              return child instanceof HTMLElement && isInteractive(child)
+            })
+          )
+        })
+        // PageHeader.TitleArea is be the first element in the DOM even when it is not visually the first.
+        // Motivation behind this rule to make sure context area and leading action (if they exist) are always rendered after the title (a heading tag)
+        // so that screen reader users who are navigating via heading menu won't miss these actions.
         warning(
-          hasInteractiveContent,
+          hasInteractiveContent && (hasContextArea || hasLeadingAction),
           'When PageHeader.ContextArea or PageHeader.LeadingAction is present, we recommended not to include any interactive items in the PageHeader.TitleArea to make sure the focus order is logical.',
         )
-      }
-    }, [children, rootRef, hasContextArea, hasLeadingAction])
+      },
+      [children, rootRef],
+    )
     return (
-      <Box
-        ref={rootRef}
-        as={as}
-        className={className}
-        sx={merge<BetterSystemStyleObject>(rootStyles, sx)}
-        data-size-variant={titleVariant}
-      >
+      <Box ref={rootRef} as={as} className={className} sx={merge<BetterSystemStyleObject>(rootStyles, sx)}>
         {children}
       </Box>
     )
@@ -193,6 +187,9 @@ const ContextArea: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({
     ...getBreakpointDeclarations(hidden, 'display', value => {
       return value ? 'none' : 'flex'
     }),
+    fontWeight: 'initial',
+    lineHeight: 'var(--text-body-lineHeight-medium, 1.4285)',
+    fontSize: 'var(--text-body-size-medium, 0.875rem)',
   }
 
   return (
@@ -314,25 +311,6 @@ const TitleArea = React.forwardRef<HTMLDivElement, React.PropsWithChildren<Title
   ({children, className, sx = {}, hidden = false, variant = 'medium'}, forwardedRef) => {
     const titleAreaRef = useProvidedRefOrCreate<HTMLDivElement>(forwardedRef as React.RefObject<HTMLDivElement>)
     const currentVariant = useResponsiveValue(variant, 'medium')
-    const [fontSize, setFontSize] = useState<string | null | number | string[]>(null)
-
-    useEffect(() => {
-      if (!titleAreaRef.current || titleAreaRef.current.children.length <= 0) return
-      const title = Array.from(titleAreaRef.current.children as HTMLCollection).find(child => {
-        return child instanceof HTMLElement && child.getAttribute('data-component') === 'PH_Title'
-      })
-
-      const styles = getComputedStyle(title as HTMLHeadingElement)
-      const customfontSize = styles.getPropertyValue('--custom-font-size')
-      // This is cumbersome but needed to handle the array format of font-size
-      if (customfontSize.includes(',')) {
-        const values = customfontSize.split(',')
-        setFontSize(values)
-      } else {
-        setFontSize(customfontSize)
-      }
-      // We only need this on the pageload
-    }, [titleAreaRef])
     return (
       <Box
         className={className}
@@ -350,24 +328,6 @@ const TitleArea = React.forwardRef<HTMLDivElement, React.PropsWithChildren<Title
             }),
             flexDirection: 'row',
             alignItems: 'flex-start',
-            // line-height is calculated with calc(height/font-size) and the below numbers are from @primer/primitives
-            //  --custom-font-size, --custom-line-height, --custom-font-weight are custom properties (passed by sx) that can be used to override the below values
-            // We don't want these values to be overriden but still want to allow consumers to override them if needed.
-            '&[data-size-variant="large"] [data-component="PH_Title"]': {
-              fontSize: fontSize ? fontSize : 'var(--text-title-size-large, 2rem)',
-              lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-large, 1.5))', // calc(48/32)
-              fontWeight: 'var(--custom-font-weight, var(--base-text-weight-normal, 400))',
-            },
-            '&[data-size-variant="medium"] [data-component="PH_Title"]': {
-              fontSize: fontSize ? fontSize : 'var(--text-title-size-medium, 1.25rem)',
-              lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))', // calc(32/20)
-              fontWeight: 'var(--custom-font-weight, var(--base-text-weight-semibold, 600))',
-            },
-            '&[data-size-variant="subtitle"] [data-component="PH_Title"]': {
-              fontSize: fontSize ? fontSize : 'var(--text-title-size-medium, 1.25rem)',
-              lineHeight: 'var(--custom-line-height, var(--text-title-lineHeight-medium, 1.6))', // calc(32/20)
-              fontWeight: 'var(--custom-font-weight, var(--base-text-weight-normal, 400))',
-            },
           },
           sx,
         )}
@@ -435,6 +395,9 @@ const Breadcrumbs: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({
             return value ? 'none' : 'flex'
           }),
           alignItems: 'center',
+          fontWeight: 'initial',
+          lineHeight: 'var(--text-body-lineHeight-medium, 1.4285)',
+          fontSize: 'var(--text-body-size-medium, 0.875rem)',
         },
         sx,
       )}
@@ -510,6 +473,8 @@ const Title: React.FC<React.PropsWithChildren<TitleProps>> = ({
           ...getBreakpointDeclarations(hidden, 'display', value => {
             return value ? 'none' : 'block'
           }),
+          fontSize: 'inherit',
+          fontWeight: 'inherit',
         },
         sx,
       )}
@@ -647,6 +612,9 @@ const Description: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({
           alignItems: 'center',
           paddingTop: '0.5rem',
           gap: '0.5rem',
+          fontWeight: 'initial',
+          lineHeight: 'var(--text-body-lineHeight-medium, 1.4285)',
+          fontSize: 'var(--text-body-size-medium, 0.875rem)',
         },
         sx,
       )}
@@ -693,6 +661,9 @@ const Navigation: React.FC<React.PropsWithChildren<NavigationProps>> = ({
           ...getBreakpointDeclarations(hidden, 'display', value => {
             return value ? 'none' : 'block'
           }),
+          fontWeight: 'initial',
+          lineHeight: 'var(--text-body-lineHeight-medium, 1.4285)',
+          fontSize: 'var(--text-body-size-medium, 0.875rem)',
         },
         sx,
       )}