[React 19] [bug] SVG with dangerouslySetInnerHTML content does not trigger first click

[zdravkov]

Summary

Hi all,

Here is the scenario that we found out while testing with both the latest rc and the beta that works correctly with React 18.
We have an SVG element that does not trigger its first click if it is focusable (or positioned inside a focusable element) that changes some state on focus.

Steps to reproduce:
Open the Stackblitz example and open its console
Click 1 time on the triangle svg element
Expected:
'svg click' message is logged
Current:
no message is logged

(On the second and all next clicks the message is shown as expected - only the first click is not triggered)
Here are the stackblitz examples where the issue can be observed:
rc: https://stackblitz.com/edit/react-vsxt51-w3ktmp?file=app%2Fapp.tsx - not working
beta: https://stackblitz.com/edit/react-vsxt51-ssqptj?file=app%2Fapp.tsx - not working
And here is how it is working in React 18:
React 18: https://stackblitz.com/edit/react-vsxt51-xsg1yu?file=app%2Fapp.tsx - working

Code:

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

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

[rinika-web]

Hi,

I noticed this issue and I'd like to contribute to help resolve it. I have experience working with React, SVG elements, and event handling, and I believe I can assist in identifying the root cause or proposing a solution.

This will be my first time contributing to open source, so I’d appreciate any guidance on the next steps or specific areas of the codebase I should focus on. I’ll start by reproducing the issue locally and analysing the behaviour across different React versions.

Looking forward to contributing

[nareshmurty]

Hi,
I noticed this issue, const [focused, setFocused] = React.useState(true); // I made the value changes from false to true.
Now it is working

import * as React from 'react';

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

return (
<svg
onFocus={handleFocus}
tabIndex={1}
onClick={() => {
console.log('svg click');
}}
viewBox="0 0 512 512"
dangerouslySetInnerHTML={{
__html: '',
}}
>
);
};

export default App;

[zdravkov]

@nareshmurty - Indeed, this is part of the bug scenario - the issue is observed when any change of the state is attempted in the onFocus event - it should be possible to change the state in this event and currently it is not.

[nareshmurty]

@zdravkov - I got a solution related to the above scenario but I don't know whether it is the best way or not.

• Using the setTimeout(), may be we can delay the state update, here it is working fine during state change

import * as React from 'react';

const App = () => {
const [focused, setFocused] = React.useState(false);
const handleFocus = () => {
setTimeout(() => {
setFocused(true);
console.log(focused);
}, 500);
};

return (
<svg
onFocus={handleFocus}
tabIndex={1}
onClick={() => {
console.log('svg click');
}}
viewBox="0 0 512 512"
dangerouslySetInnerHTML={{
__html: '',
}}
>
);
};

export default App;

[zdravkov]

@nareshmurty thank you for your efforts! The suggested approach rather seems like a workaround than a solution to me. A solution here would be fixing the bug in the new React 19 codebase.

[koushikjoshi]

Hi @zdravkov
I've managed to find a fix for this issue. Let me know if it aligns with the best practices for this codebase or if it could be made better/more optimized.

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}>
      

Inside the tests/ReactDOMSVG-test.js file

Here's the test I wrote to test this out:

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

    function App() {
      const [focused, 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" />',
          }}
        ></svg>
      );
    }

    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.

Let me know if this looks good enough for me to make a PR.

Please note- I only ran the ReactDOMSVG-test to ensure nothing breaks, but if you think that there are tests elsewhere that could potentially break, let me know and I'll run a full test too.

Looking forward to your feedback!

[zdravkov]

Thank you for your help with the issue and the contribution, I am not quite familiar with the code rules of this repo, so I can't provide good opinion on your pr. I hope somebody from the core team will check it soon! :)
Greetings!

[eps1lon]

Thank you for reporting. This is a pretty obscure bug. It requires dangerouslySetInnerHTML and a re-render during the focus event when clicking. Not using dangerouslySetInnerHTML or not re-rendering will work as expected.

It started breaking in https://www.npmjs.com/package/react/v/18.3.0-next-85de6fde5-20230328

First bad: https://www.npmjs.com/package/react/v/18.3.0-next-85de6fde5-20230328
Last good: https://www.npmjs.com/package/react/v/18.3.0-next-41b4714f1-20230328

Diff: 41b4714...85de6fd

Likely candidate is #26501. It's a big diff but maybe somebody is able to spot an issue.

[zdravkov]

Hey all,
Thank you once again for taking a looks at this issue. 🙇
Just to clarify that the last release candidate didn't fix the issue- https://stackblitz.com/edit/react-vsxt51-7cdrvt?file=app%2Fapp.tsx

The pr from @koushikjoshi seems to be fixing it - #31038

Greetings,
Plamen

[koushikjoshi]

Hi @zdravkov
The issue isn't fixed yet. I have narrowed it down, though.
I'll get back to it in some time and post an update in this thread once it seems to be fixed.

Thanks,
Koushik.

[kspeyanski]

It might be implied by @eps1lon's comment, but seems that this bug not limited to svg's only, but rather all elements that use dangerouslySetInnerHTML

A sandbox with a button + span:
https://codesandbox.io/p/sandbox/sharp-varahamihira-kvc93j

[kspeyanski]

On a second thought... I think this might not be a bug with react.

Prior to #26501 it was working, since some changes (in this case the dangerouslySetInnerHTML) were dropped before the commit phase, but if you actually modify the HTML during onFocus it's still not calling the click event:

Initial example modified so that the innerHTML also diffs:
https://codesandbox.io/p/sandbox/elated-ace-2twxpt?workspaceId=cbc514bf-ce86-4c87-bd7a-42e37c58b220

Replicated what ReactDOM is doing under the hood with raw HTML/JS, and it behaves the same - setting innerHTML of an element does not keep the element's instance and since the pointerdown event originated from an element inside (which is dropped during focusin) the click is never called.
https://codesandbox.io/p/sandbox/79zq26

Referring to the original example with the svg and path, @zdravkov we could memoize the icon and the click event should work as before - dynamically changing the icon before/during the click event (e.g. focus, pointerdown) would still break but it's a side effect of how dangerouslySetInnerHTML and innerHTML works under the hood:
https://codesandbox.io/p/sandbox/sad-drake-l3pzth?workspaceId=cbc514bf-ce86-4c87-bd7a-42e37c58b220