[docs] Demo VirtualElementPopper crashes

[chwallen]

• I have searched for similar issues in this repository and believe that this is not a duplicate.

This is kind of a duplicate but the others are closed/incomplete due to people not responding.

Steps to Reproduce

1. Visit https://mui.com/components/popper/#virtual-element
2. Highlight some text in the demo box to show the Popper
3. Deselect the text (the Popper is still visible)
4. Move the mouse outside of the demo box
5. The demo crashes

Video of how to reproduce:

popper-virtual-element-crash.mp4

Stack trace

framework-0fcbae470ceff9fb.js:1 DOMException: Failed to execute 'getRangeAt' on 'Selection': 0 is not a valid index.
  at Object.getBoundingClientRect (https://mui.com/_next/static/chunks/10603-b653fa64cb02d616.js:1:68970)
  at p (https://mui.com/_next/static/chunks/72773-fbc21f61c9083093.js:1:611)
  at g (https://mui.com/_next/static/chunks/72773-fbc21f61c9083093.js:1:1500)
  at Object.forceUpdate (https://mui.com/_next/static/chunks/72773-fbc21f61c9083093.js:1:5523)
  at https://mui.com/_next/static/chunks/20541-e22b3aa635ff3968.js:1:1278
  at Qu (https://mui.com/_next/static/chunks/framework-0fcbae470ceff9fb.js:1:126730)
  at t.unstable_runWithPriority (https://mui.com/_next/static/chunks/framework-0fcbae470ceff9fb.js:1:156193)
  at Ql (https://mui.com/_next/static/chunks/framework-0fcbae470ceff9fb.js:1:64802)
  at ju (https://mui.com/_next/static/chunks/framework-0fcbae470ceff9fb.js:1:126173)
  at Cu (https://mui.com/_next/static/chunks/framework-0fcbae470ceff9fb.js:1:116590)
mi @ framework-0fcbae470ceff9fb.js:1
n.payload @ framework-0fcbae470ceff9fb.js:1
pa @ framework-0fcbae470ceff9fb.js:1
Xo @ framework-0fcbae470ceff9fb.js:1
Ji @ framework-0fcbae470ceff9fb.js:1
Uu @ framework-0fcbae470ceff9fb.js:1
Fu @ framework-0fcbae470ceff9fb.js:1
Ou @ framework-0fcbae470ceff9fb.js:1
Cu @ framework-0fcbae470ceff9fb.js:1
(anonymous) @ framework-0fcbae470ceff9fb.js:1
t.unstable_runWithPriority @ framework-0fcbae470ceff9fb.js:1
Ql @ framework-0fcbae470ceff9fb.js:1
Yl @ framework-0fcbae470ceff9fb.js:1
Kl @ framework-0fcbae470ceff9fb.js:1
Me @ framework-0fcbae470ceff9fb.js:1
(anonymous) @ framework-0fcbae470ceff9fb.js:1
Ir @ framework-0fcbae470ceff9fb.js:1
tn @ framework-0fcbae470ceff9fb.js:1
en @ framework-0fcbae470ceff9fb.js:1
t.unstable_runWithPriority @ framework-0fcbae470ceff9fb.js:1
Jt @ framework-0fcbae470ceff9fb.js:1

Your Environment

Tech	Version
MUI	v5.5.2
netlify deploy	https://6241c5205e762a0008653128--material-ui-docs.netlify.app
Browser	Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/99.0.4844.51 Safari/537.36

[michaldudak]

Thanks for the report! Would you like to work on a solution?

[chwallen]

Thanks for the report! Would you like to work on a solution?

If you have not started already (I see that you are assigned), I can make an attempt!

[michaldudak]

Go ahead!

[chwallen]

@michaldudak I managed to figure out why it crashes. After deselecting the text and moving my cursor out of the box, handleClose runs and sets the open state to false. However, the anchorEl still contains the virtual element callback, which gets called again on re-render. As the selection is empty, the getRangeAt call throws an error (the referenced selection object is mutated in-place). Therefore, either handleClose should be modified to include setAnchorEl(null);, or the the virtual element has to be eagerly calculated.

A separate problem, which is not covered in this issue, is why the Popper is still visible after selection. I found out that when handleMouseUp is called, the selection object retrieved by window.getSelection() is stale. Adding a wrapping setTimeout with duration 0 (for execution the next event cycle) achieves the intended behaviour, i.e., closing the Popper when the text is deselected. However, this gives me the following error on text selection

Warning: Failed prop type: MUI: The anchorEl prop provided to the component is invalid.
It should be an HTML element instance or a virtualElement

even though the anchorEl has the same values as before; either null or an object with a getBoundClientRect callback (regardless if it is eagerly or lazily calculated).

Moving the setOpen below the setAnchorEl in handleMouseUp will succeed the validation, but the modified handleClose (with a setAnchorEl(null) as the first line) will not. It logs the same error which was suppressed since the other callback already logged it.

There seems to be some kind of conflict when open is true while the anchorEl is null (but which is only present when handleMouseUp is wrapped by setTimeout). Merging the two states to a common state object, React.useState<{ open: boolean, anchorEl: PopperProps['anchorEl'] }, will succeed all validations and work as intended (no crashes and the Popper will close on text deselect).

[michaldudak]

Thanks for the investigation!

I don't think we need to keep the state for both anchorEl and open. open can be specified as simply anchorEl != null.

As for the other findings - I'm OK with wrapping the body of handleMouseUp in setTimeout and calculating boundingClientRect eagerly. I'd appreciate a PR if you can find the time.

[chwallen]

I don't think we need to keep the state for both anchorEl and open. open can be specified as simply anchorEl != null.

That is true. However, doing so will remove the fade out animation (as the anchor is now null, the popper disappears immediately). Might not be the most visually pleasing. 😄

As for the other findings - I'm OK with wrapping the body of handleMouseUp in setTimeout and calculating boundingClientRect eagerly. I'd appreciate a PR if you can find the time.

Absolutely, I will make one!