Refactor MarkdownViewer to make it SSR-friendly (#3207)

* Refactor MarkdownViewer to make it SSR-friendly

* Add comment about container state

* Revert changes to useListInteraction

* Create chilly-dragons-crash.md

* Handle dom updates in useListInteraction

* Handle on change error

* Use isomorphic layout effect

.changeset/chilly-dragons-crash.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+`MarkdownViewer` is now SSR-compatible

src/drafts/MarkdownViewer/MarkdownViewer.test.tsx

@@ -1,6 +1,6 @@
-import React from 'react'
-import {render, waitFor, fireEvent} from '@testing-library/react'
+import {fireEvent, render, waitFor} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
+import React from 'react'
 
 import MarkdownViewer from '../MarkdownViewer'
 
@@ -89,6 +89,42 @@ text after list`
       fireEvent.change(items[0])
       await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(noItemsCheckedMarkdown))
     })
+
+    it('attaches event handlers to new tasks', async () => {
+      const onChangeMock = jest.fn()
+      const TestComponent = () => {
+        const [html, setHtml] = React.useState(taskListHtml)
+        const [markdown, setMarkdown] = React.useState(noItemsCheckedMarkdown)
+        return (
+          <>
+            <button
+              onClick={() => {
+                setMarkdown(`${markdown}
+- [ ] item 3
+`)
+                setHtml(`${html}
+<ul class='contains-task-list'>
+  <li class='task-list-item'><input type='checkbox' class='task-list-item-checkbox' disabled/> item 3</li>
+</ul>
+`)
+              }}
+            >
+              Add markdown
+            </button>
+            <MarkdownViewer dangerousRenderedHTML={{__html: html}} markdownValue={markdown} onChange={onChangeMock} />
+          </>
+        )
+      }
+      const {getByRole, getAllByRole} = render(<TestComponent />)
+
+      // Change markdown and html content
+      const button = getByRole('button', {name: 'Add markdown'})
+      fireEvent.click(button)
+
+      const items = getAllByRole('checkbox')
+      fireEvent.change(items[2])
+      await waitFor(() => expect(onChangeMock).toHaveBeenCalledWith(`${noItemsCheckedMarkdown}\n- [x] item 3\n`))
+    })
   })
 
   describe('link interception', () => {

src/drafts/MarkdownViewer/MarkdownViewer.tsx

@@ -1,5 +1,5 @@
-import React, {DOMAttributes, useCallback, useEffect, useRef, useState} from 'react'
-import {Spinner, Box} from '../..'
+import React, {DOMAttributes, useCallback} from 'react'
+import {Box, Spinner} from '../..'
 
 import {useLinkInterception} from './_useLinkInterception'
 import {useListInteraction} from './_useListInteraction'
@@ -55,12 +55,6 @@ type NoninteractiveMarkdownViewerProps = CoreMarkdownViewerProps & {
 
 export type MarkdownViewerProps = NoninteractiveMarkdownViewerProps | InteractiveMarkdownViewerProps
 
-const createRenderedContainer = (html: string): HTMLDivElement => {
-  const div = document.createElement('div')
-  div.innerHTML = html
-  return div
-}
-
 const MarkdownViewer = ({
   dangerousRenderedHTML,
   loading = false,
@@ -70,32 +64,33 @@ const MarkdownViewer = ({
   onLinkClick,
   openLinksInNewTab = false,
 }: MarkdownViewerProps) => {
-  const outputContainerRef = useRef<HTMLDivElement>(null)
-
-  // Render the HTML into an internal container element so we can modify it before it becomes visible.
-  // Using `unsafeInnerHTML` would require an effect to run after rendering which would cause flicker
-  const [htmlContainer, setHtmlContainer] = useState(() => createRenderedContainer(dangerousRenderedHTML.__html))
-  useEffect(
-    () => setHtmlContainer(createRenderedContainer(dangerousRenderedHTML.__html)),
-    [dangerousRenderedHTML.__html],
-  )
+  // We're using state to store the HTML container because we want the value
+  // to re-run effects when it changes
+  const [htmlContainer, setHtmlContainer] = React.useState<HTMLElement>()
+  const htmlContainerRef = React.useCallback((node: HTMLElement | null) => {
+    if (!node) return
+    setHtmlContainer(node)
+  }, [])
 
   const onChange = useCallback(
     async (value: string) => {
       try {
         await externalOnChange?.(value)
       } catch (error) {
-        setHtmlContainer(createRenderedContainer(dangerousRenderedHTML.__html))
+        if (htmlContainer) {
+          htmlContainer.innerHTML = dangerousRenderedHTML.__html
+        }
       }
     },
-    [externalOnChange, dangerousRenderedHTML.__html],
+    [externalOnChange, htmlContainer, dangerousRenderedHTML],
   )
 
   useListInteraction({
     onChange,
     disabled: disabled || !externalOnChange,
     htmlContainer,
     markdownValue,
+    dependencies: [dangerousRenderedHTML],
   })
 
   useLinkInterception({
@@ -104,20 +99,16 @@ const MarkdownViewer = ({
     openLinksInNewTab,
   })
 
-  // If we were to inject the `...htmlContainer.children` instead of the container element itself,
-  // those children elements would be moved from the `htmlContainer` to the `outputContainer`. Then if
-  // other effects use `htmlContainer.querySelectorAll`, they wouldn't find any elements to affect
-  useEffect(() => outputContainerRef.current?.replaceChildren(htmlContainer), [htmlContainer])
-
   return loading ? (
     <Box sx={{display: 'flex', justifyContent: 'space-around', p: 2}}>
       <Spinner aria-label="Loading content..." />
     </Box>
   ) : (
     <Box
-      ref={outputContainerRef}
+      ref={htmlContainerRef}
       className="markdown-body"
       sx={{fontSize: 1, maxWidth: '100%', '& > div > :last-child': {mb: 0}}}
+      dangerouslySetInnerHTML={dangerousRenderedHTML}
     />
   )
 }

src/drafts/MarkdownViewer/_useLinkInterception.ts

@@ -1,7 +1,7 @@
 import {useEffect} from 'react'
 
 type UseLinkInterceptionSettings = {
-  htmlContainer: HTMLDivElement
+  htmlContainer?: HTMLElement
   onLinkClick?: (event: MouseEvent) => void
   openLinksInNewTab: boolean
 }
@@ -23,6 +23,8 @@ export const useLinkInterception = ({htmlContainer, onLinkClick, openLinksInNewT
       }
     }
 
+    if (!htmlContainer) return
+
     htmlContainer.addEventListener('click', clickHandler)
 
     return () => {

src/drafts/MarkdownViewer/_useListInteraction.ts

@@ -1,4 +1,5 @@
-import {useCallback, useEffect, useLayoutEffect, useMemo, useRef} from 'react'
+import {useCallback, useEffect, useRef, useState} from 'react'
+import useIsomorphicLayoutEffect from '../../utils/useIsomorphicLayoutEffect'
 import {ListItem, listItemToString, parseListItem} from '../MarkdownEditor/_useListEditing'
 
 type TaskListItem = ListItem & {taskBox: '[ ]' | '[x]'}
@@ -11,10 +12,11 @@ const toggleTaskListItem = (item: TaskListItem): TaskListItem => ({
 })
 
 type UseListInteractionSettings = {
-  htmlContainer: HTMLDivElement | null
+  htmlContainer?: HTMLElement
   markdownValue: string
   onChange: (markdown: string) => void | Promise<void>
   disabled?: boolean
+  dependencies?: Array<unknown>
 }
 
 /**
@@ -28,11 +30,13 @@ export const useListInteraction = ({
   markdownValue,
   onChange,
   disabled = false,
+  dependencies = [],
 }: UseListInteractionSettings) => {
   // Storing the value in a ref allows not using the markdown value as a depdency of
   // onToggleItem, which would mean we'd have to re-bind the event handlers on every change
   const markdownRef = useRef(markdownValue)
-  useLayoutEffect(() => {
+
+  useIsomorphicLayoutEffect(() => {
     markdownRef.current = markdownValue
   }, [markdownValue])
 
@@ -62,12 +66,18 @@ export const useListInteraction = ({
     [onChange],
   )
 
-  const checkboxElements = useMemo(
-    () =>
-      Array.from(
-        htmlContainer?.querySelectorAll<HTMLInputElement>('input[type=checkbox].task-list-item-checkbox') ?? [],
-      ),
-    [htmlContainer],
+  const [checkboxElements, setCheckboxElements] = useState<HTMLInputElement[]>([])
+
+  useEffect(
+    () => {
+      setCheckboxElements(
+        Array.from(
+          htmlContainer?.querySelectorAll<HTMLInputElement>('input[type=checkbox].task-list-item-checkbox') ?? [],
+        ),
+      )
+    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [htmlContainer, ...dependencies],
   )
 
   // This could be combined with the other effect, but then the checkboxes might have a flicker