[DevTools] Use Popover API for TraceUpdates highlighting

[yongsk0066]

Summary

When using React DevTools to highlight component updates, the highlights would sometimes appear behind elements that use the browser's top-layer (such as <dialog> elements or components using the Popover API). This made it difficult to see which components were updating when they were inside or behind top-layer elements.

This PR fixes the issue by using the Popover API to ensure that highlighting appears on top of all content, including elements in the top-layer. The implementation maintains backward compatibility with browsers that don't support the Popover API.

How did you test this change?

I tested this change in the following ways:

1. Manually tested in Chrome (which supports the Popover API) with:

   • Created a test application with React components inside <dialog> elements and custom elements using the Popover API
   • Verified that component highlighting appears above these elements when they update
   • Confirmed that highlighting displays correctly for nested components within top-layer elements

2. Verified backward compatibility:

   • Tested in browsers without Popover API support to ensure fallback behavior works correctly
   • Confirmed that no errors occur and highlighting still functions as before

3. Ran the React DevTools test suite:

   • All tests pass successfully
   • No regressions were introduced

demo-page
demo-repo

AS-IS

witout_toplayer.mov

TO-BE

with_toplayer.mov

[hoxyq]

Are you gating it with try/catch because for feature checking?

We might bump the minimum required version for Chrome / other browsers here:
https://github.com/facebook/react/blob/main/packages/react-devtools-extensions/chrome/manifest.json#L7

So you won't need to worry about if its supported or not

[yongsk0066]

Yes, that's right. I've handled it in case popover isn't supported. Would it be okay to raise the minimum version? If we do raise it, I plan to increase it to version 114 (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/showPopover#browser_compatibility) which supports the popover API.

[hoxyq]

Please do it for Edge browser as well. I believe Firefox should already list version that is higher than needed.

[yongsk0066]

I've added Edge as well! Yes, Firefox requires 125 or higher, but it's already set to 128, so we're good.

[hoxyq]

Please see the failed Flow checks, overall the changes are seemed necessary, thanks for fixing this

[yongsk0066]

@hoxyq Thanks for the review! i've fixed all the Flow errors and updated the implementation. I've added appropriate $FlowFixMe annotations since Flow doesn't recognize the Popover API yet. All tests are now passing!

[yongsk0066]

Hi @hoxyq, just wanted to check in to see if there's anything else needed from my side.
Let me know if I can clarify or help further. Thanks!

By the way, I've also added Popover-related type definitions to flow-typed.
However, since the version of Flow used in the React repository is still behind,
we can't apply them here immediately. Perhaps we can in the future once it's updated.

[hoxyq]

Thanks for updating flow-typed as well!

Flow team does upgrades frequently, so it might be updated soon, but shouldn't block you from landing this change.

Overall looks good to me, see my comment, I will try to manually test this tomorrow.

[hoxyq]

Same as above, you don't need it to wrap it in try / catch, since we bumped minimum versions for the extension

Please also check other calls below

[yongsk0066]

Thanks for pointing this out! I've removed all try/catch blocks around Popover API calls.

[hoxyq]

Had another iteration on this, few points:

1. Thanks for providing a really useful repro, could you copy it to react-devtools-shell package, so we can later use it for manual tests?
2. I believe we would still fail to highlight updates if the dialog was opened while other components were highlighted. Imagine if you have an app, where something constantly re-renders in an interval that is less then React DevTools for clearing up highlights - drawWeb will never be called with empty nodeToData map, and we will re-open popover. What if we would re-open popover everytime drawWeb was called with non-empty nodeToData, how would that look?

[hoxyq]

I think this new code changes the behaviour of the drawWeb function.

Before this change, we would clear out canvas and draw nothing. Also, even if canvas was not initialized yet and drawWeb was called, we would initialize it as empty.

I think we should preserve the previous approach:

1. Remove canvas.showPopover() from initialize.
2. In the the end of drawWeb function call, add canvas.showPopover or canvas.hidePopover calls, based on nodeToData.size and popover state.

Something like this:

function drawWeb(nodeToData: Map<HostInstance, Data>) {
 <all code before your changes>
 ...
 
 if (nodeToData.size === 0 && canvas.matches(':popover-open')) {
   canvas.hidePopover();
 } else if (!canvas.matches(':popover-open') && nodeToData.size !== 0) {
   canvas.openPopover();
 }
 }

[yongsk0066]

Ah, I see. Totally agree that we should first finish all drawWeb logic and then decide at the end whether or not to promote it to the top-layer.
I think that approach better fits the responsibilities of each method. Thanks for clarifying this!
Considering your comments, I've tested two different approaches:

1. Hiding the canvas only when there's nothing to draw (the current method):

function drawWeb(nodeToData: Map<HostInstance, Data>) {
  // ... previous code
  if (canvas !== null) {
    if (nodeToData.size === 0 && canvas.matches(':popover-open')) {
      canvas.hidePopover();
    } else if (nodeToData.size > 0 && !canvas.matches(':popover-open')) {
      canvas.showPopover();
    }
  }
} // end of drawWeb

Result: This method avoids frequent updates to the top-layer.
However, as you mentioned in your comment, if another top-layer element appears while highlights still remain, the highlight canvas might fall behind the new element.

2. Re-promoting canvas to top-layer on every update call:

function drawWeb(nodeToData: Map<HostInstance, Data>) {
  // ... previous code
  if (canvas !== null) {
    if (nodeToData.size === 0 && canvas.matches(':popover-open')) {
      canvas.hidePopover();
      return;
    }
    if (canvas.matches(':popover-open')) {
      canvas.hidePopover();
    }
    canvas.showPopover();
  }
} // end of drawWeb

Result: Frequent changes to the top-layer occur, but the highlights consistently remain on top, ensuring they never get hidden by other elements.
I think method #2 seems preferable in terms of clearly maintaining highlight visibility too.
However, I couldn't find any mentions of performance issues in either the W3C review or blink-dev discussions regarding frequent top-layer changes. Unless highlights are triggered continuously at extremely high frequency over a prolonged period, there shouldn't be any noticeable performance impact.

[yongsk0066]

Thanks again for the review, @hoxyq !
I've added the test to react-devtools-shell as suggested.

[yongsk0066]

How have you been, @hoxyq ?

It's been about a month, so I'm just leaving a comment to check if there's any additional feedback. I've been using this version for about a month without any major issues. No rush at all, just checking in. Thanks!

[hoxyq]

How have you been, @hoxyq ?

It's been about a month, so I'm just leaving a comment to check if there's any additional feedback. I've been using this version for about a month without any major issues. No rush at all, just checking in. Thanks!

Hey @yongsk0066, apologies for the delay. Don't worry, this will be in the next release of React DevTools, I was just prioritising other things. I have now finished other things and planning to get this tested next week.

Thank you for contributing to React DevTools.

[hoxyq]

Sorry for the delay, looks good. Thanks again!

#32703 is in my queue as well, I will also review and test it, probably worth rebasing it on main, once I merge this one.