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

[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

[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.

[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!

from react.

[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!

[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.