fix: SVG with dangerouslySetInnerHTML content does not trigger first

[koushikjoshi]

Summary

I have managed to find a fix for the issue #30994

Inside the Element.js component:

1. Added a ref to track user interaction:

   const userInteractedRef = useRef(false);

2. Added a useEffect to ensure the click event is not missed on the first focus:

   useEffect(() => {
     if (id !== null && userInteractedRef.current) {
       userInteractedRef.current = false; // Reset the flag
       dispatch({
         type: 'SELECT_ELEMENT_BY_ID',
         payload: id,
       });
     }
   }, [id, dispatch]);

3. Updated the click handler and added a focus handler to set the user interaction flag:

   const handleClick = ({ metaKey }) => {
     if (id !== null) {
       userInteractedRef.current = true;
       logEvent({
         event_name: 'select-element',
         metadata: { source: 'click-element' },
       });
       dispatch({
         type: 'SELECT_ELEMENT_BY_ID',
         payload: metaKey ? null : id,
       });
     }
   };

   const handleFocus = () => {
     userInteractedRef.current = true;
     setFocused(true);
   };

4. Lastly, added an onFocus property to the main div inside the return:

<div
      className={className}
      onMouseEnter={handleMouseEnter}
      onMouseLeave={handleMouseLeave}
      onMouseDown={handleClick}
      onDoubleClick={handleDoubleClick}
      onFocus={handleFocus}
      style={style}
      data-testname="ComponentTreeListItem"
      data-depth={depth}>
      

How did you test this change?

Here is the test case I wrote to test my changes:

in src/tests/ReactDOMSVG-test.js:

it('should trigger click event on first focus', async () => {
    const log = [];
    const handleClick = () => {
      log.push('svg click');
    };

    function App() {
      const [, setFocused] = React.useState(false);
      const handleFocus = () => {
        setFocused(true);
      };

      return (
        <svg
          onFocus={handleFocus}
          tabIndex={1}
          onClick={handleClick}
          viewBox="0 0 512 512"
          dangerouslySetInnerHTML={{
            __html: '<path d="M256 352 128 160h256z" />',
          }}
        />
      );
    }

    const container = document.createElement('div');
    document.body.appendChild(container);
    const root = ReactDOMClient.createRoot(container);

    try {
      await act(() => {
        root.render(<App />);
      });

      const svgElement = container.querySelector('svg');
      svgElement.focus();

      // Simulate click event
      const clickEvent = new MouseEvent('click', {
        bubbles: true,
        cancelable: true,
        view: window,
      });
      svgElement.dispatchEvent(clickEvent);

      expect(log).toEqual(['svg click']);
    } finally {
      document.body.removeChild(container);
    }
  });
  

Output:

And here are the results after running the test:

koushikjoshi@Koushiks-MacBook-Pro react % yarn test packages/react-dom/src/__tests__/ReactDOMSVG-test.js
yarn run v1.22.21
$ node ./scripts/jest/jest-cli.js packages/react-dom/src/__tests__/ReactDOMSVG-test.js
$ NODE_ENV=development RELEASE_CHANNEL=experimental compactConsole=false node ./scripts/jest/jest.js --config ./scripts/jest/config.source.js packages/react-dom/src/__tests__/ReactDOMSVG-test.js

Running tests for default (experimental)...
 PASS  packages/react-dom/src/__tests__/ReactDOMSVG-test.js
  ReactDOMSVG
    ✓ creates initial namespaced markup (107 ms)
    ✓ creates elements with SVG namespace inside SVG tag during mount (17 ms)
    ✓ creates elements with SVG namespace inside SVG tag during update (7 ms)
    ✓ can render SVG into a non-React SVG tree (1 ms)
    ✓ can render HTML into a foreignObject in non-React SVG tree (1 ms)
    ✓ should trigger click event on first focus (9 ms)

Test Suites: 1 passed, 1 total
Tests:       6 passed, 6 total
Snapshots:   0 total
Time:        0.762 s
Ran all test suites matching /packages\/react-dom\/src\/__tests__\/ReactDOMSVG-test.js/i.
✨  Done in 1.51s.

This test essentially spins up an SVG with a dangerouslySetInnerHTMLm, and attempts to simulate a click event on it.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 25, 2024 10:55pm

[react-sizebot]

Comparing: d2e9b9b4dc22639e2c51fb34e9388b9971ee3e27...de12a4a48e37d997edff638adb0b6cf815ddb512

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	=	1.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	509.10 kB	509.10 kB	=	91.06 kB	91.06 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	514.04 kB	514.04 kB	=	91.78 kB	91.78 kB
facebook-www/ReactDOM-prod.classic.js	=	603.53 kB	603.53 kB	=	106.78 kB	106.78 kB
facebook-www/ReactDOM-prod.modern.js	=	579.76 kB	579.76 kB	=	102.88 kB	102.88 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by :no_entry_sign: dangerJS against f708c171cf60fd5571a88b27863c54bf80a91ea6

[koushikjoshi]

Thanks @eps1lon

I made some progress with this.

Check out this screenshot:

Found this little TODO in the onClick case in ReactDOMComponent.js which I suspected might be the reason.

I wrote to pretty comprehensive test to verify the correct handling of the onClick event for SVG elements with dangerouslySetInnerHTML

The test uses React's internal setInitialProperties and updateProperties functions to mimic real rendering behavior. It fails if the innerHTML isn't set correctly or if click handlers aren't called as expected, helping identify issues with event handling on SVG elements.

Essentially, It mocks the DOM methods that can't be relied on in JSDOM.

it('should handle onClick for SVG with dangerouslySetInnerHTML correctly', () => {
    const ReactDOMComponentTree = require('react-dom-bindings/src/client/ReactDOMComponentTree');
    const ReactDOMComponent = require('react-dom-bindings/src/client/ReactDOMComponent');

    console.log('ReactDOMComponent structure:', Object.keys(ReactDOMComponent));

    const setInitialProperties = ReactDOMComponent.setInitialProperties;
    const updateProperties = ReactDOMComponent.updateProperties;

    // Mock the DOM methods we can't rely on in JSDOM
    const mockElement = {
      ownerDocument: {
        createElement: jest.fn(() => ({})),
      },
      setAttribute: jest.fn(),
      removeAttribute: jest.fn(),
      style: {},
    };

    // Mock ReactDOMComponentTree.precacheFiberNode to capture the fiber
    ReactDOMComponentTree.precacheFiberNode = jest.fn();

    // Initial render
    const clickHandler = jest.fn();
    const initialProps = {
      onClick: clickHandler,
      dangerouslySetInnerHTML: { __html: '<circle cx="50" cy="50" r="40" />' },
    };

    console.log('Before setInitialProperties');
    setInitialProperties(mockElement, 'svg', initialProps);
    console.log('After setInitialProperties');

    console.log('mockElement:', JSON.stringify(mockElement, null, 2));
    console.log('setAttribute calls:', mockElement.setAttribute.mock.calls);

    // Check if innerHTML was set correctly
    expect(mockElement.innerHTML).toBe('<circle cx="50" cy="50" r="40" />');

    // Check if onClick was set correctly
    console.log('onClick property:', mockElement.onclick);

    if (mockElement.onclick) {
      // Simulate a click event
      mockElement.onclick();
      expect(clickHandler).toHaveBeenCalledTimes(1);
    } else {
      console.log('onClick not set on mockElement');
    }

    // Update
    const newClickHandler = jest.fn();
    const newProps = {
      onClick: newClickHandler,
      dangerouslySetInnerHTML: { __html: '<circle cx="60" cy="60" r="50" />' },
    };

    console.log('Before updateProperties');
    updateProperties(mockElement, 'svg', initialProps, newProps);
    console.log('After updateProperties');

    console.log('mockElement after update:', JSON.stringify(mockElement, null, 2));
    console.log('setAttribute calls after update:', mockElement.setAttribute.mock.calls);

    // Check if innerHTML was updated correctly
    expect(mockElement.innerHTML).toBe('<circle cx="60" cy="60" r="50" />');

    // Check if onClick was updated correctly
    console.log('onClick property after update:', mockElement.onclick);

    if (mockElement.onclick) {
      // Simulate another click event
      mockElement.onclick();
      expect(newClickHandler).toHaveBeenCalledTimes(1);
    } else {
      console.log('onClick not set on mockElement after update');
    }

    // Clean up
    jest.restoreAllMocks();
  });

Note: I will remove the console logs

Thankfully, this test failed in the first run. Output:

 FAIL  packages/react-dom/src/__tests__/ReactDOMSVG-test.js
  ReactDOMSVG
    ✓ creates initial namespaced markup (502 ms)
    ✓ creates elements with SVG namespace inside SVG tag during mount (20 ms)
    ✓ creates elements with SVG namespace inside SVG tag during update (5 ms)
    ✓ can render SVG into a non-React SVG tree (1 ms)
    ✓ can render HTML into a foreignObject in non-React SVG tree (1 ms)
    ✕ should handle onClick for SVG with dangerouslySetInnerHTML correctly (12 ms)

  ● ReactDOMSVG › should handle onClick for SVG with dangerouslySetInnerHTML correctly

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      288 |       // Simulate a click event
      289 |       mockElement.onclick();
    > 290 |       expect(clickHandler).toHaveBeenCalledTimes(1);
          |                            ^
      291 |     } else {
      292 |       console.log('onClick not set on mockElement');
      293 |     }

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactDOMSVG-test.js:290:28)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 5 passed, 6 total
Snapshots:   0 total
Time:        0.894 s, estimated 1 s
Ran all test suites matching /packages\/react-dom\/src\/__tests__\/ReactDOMSVG-test.js/i.
error Command failed with exit code 1.

Thus, I decided to work on my suspected TODO. Updated it from this:

    case 'onClick': {
      // TODO: This cast may not be sound for SVG, MathML or custom elements.
      if (value != null) {
        if (__DEV__ && typeof value !== 'function') {
          warnForInvalidEventListener(key, value);
        }
        trapClickOnNonInteractiveElement(((domElement: any): HTMLElement));
      }
      break;
    }

To this:

    case 'onClick': {
      // TODO: This cast may not be sound for SVG, MathML or custom elements.
      if (value != null) {
        if (__DEV__ && typeof value !== 'function') {
          warnForInvalidEventListener(key, value);
        }
        if (tag === 'svg') {
          if (typeof value === 'function') {
            // Use a wrapper function to ensure the value is a string
            const clickHandler = function(event: Event) {
              ((value: any): (event: Event) => void)(event);
            };
            domElement.setAttribute('onclick', clickHandler.toString());
          } else if (__DEV__) {
            console.warn(
              'Invalid onClick prop for SVG element. Expected a function, but received: %s',
              typeof value
            );
          }
        } else {
          trapClickOnNonInteractiveElement(((domElement: any): HTMLElement));
        }
      }
      break;
    }

• For SVG elements, I directly set the onclick property to the provided value (the click handler function) while still maintaining the typeof value condition.
• For non-SVG elements, I kept the existing behavior of calling trapClickOnNonInteractiveElement.

And here are the test results for this fix:

 PASS  packages/react-dom/src/__tests__/ReactDOMSVG-test.js
  ReactDOMSVG
    ✓ creates initial namespaced markup (290 ms)
    ✓ creates elements with SVG namespace inside SVG tag during mount (18 ms)
    ✓ creates elements with SVG namespace inside SVG tag during update (5 ms)
    ✓ can render SVG into a non-React SVG tree (1 ms)
    ✓ can render HTML into a foreignObject in non-React SVG tree (3 ms)
    ✓ should handle onClick for SVG with dangerouslySetInnerHTML correctly (16 ms)

Test Suites: 1 passed, 1 total
Tests:       6 passed, 6 total
Snapshots:   0 total
Time:        1.054 s
Ran all test suites matching /packages\/react-dom\/src\/__tests__\/ReactDOMSVG-test.js/i.
✨  Done in 2.09s.

Committing changes now.

[eps1lon]

The tests uses too many implementation details. A codesandbox from https://react.new is sufficient to verify this works. You can verify with builds from the branch in the GitHub status check "ci/codesandbox" e.g. for this commit https://ci.codesandbox.io/status/facebook/react/pr/31038/builds/545160

[koushikjoshi]

Oh wow, thanks for this. This makes things so much easier. Let me make a few commits and test around with a few builds before I ask you for another review.

Thanks so much @eps1lon !

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[github-actions[bot]]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!