-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Popover: make sure that ownerDocument is always defined #42886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly tested this patch in the arrow fix PR and everything seemed to work well. It would be good for others to verify though 😄
Size Change: +18 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some thoughts just in the wider context of the general Popover work, but as for this PR I'm totally fine with merging as is 👍 Thanks for taking the time to investigate tangential issues and leave explanations.
The
ownerDocument
could sometimes beundefined
when the consumer of thePopover
uses a "modern style" ref (i.e. by using theuseRef
hook) and passes that value through theanchorRef
prop.
Would it be easy to add a quick unit test for this specific regression?
@@ -137,25 +137,26 @@ const Popover = ( | |||
: placement; | |||
|
|||
const ownerDocument = useMemo( () => { | |||
let documentToReturn; | |||
|
|||
if ( anchorRef?.top ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised anchorRef
is not even documented in the readme, when it's the prop that takes the highest priority. Looks like we have a lot of work to do (docs, stories, tests) as we work out the kinks in this component 💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! This component definitely needs a lot of work
4e09d9f
to
6d4e076
Compare
Done in 6d4e076. I checked that the test currently fails on |
What?
In the
Popover
component, the computation ofownerDocument
from the anchor could sometimes produce anundefined
value — which then would cause runtime errors in other parts of the component.This PR adds a fix which ensures that
ownerDocument
is always defined.Why?
The
ownerDocument
could sometimes beundefined
when the consumer of thePopover
uses a "modern style" ref (i.e. by using theuseRef
hook) and passes that value through theanchorRef
prop.For more details on how the bug was initially discovered, see #42874 (comment)
How?
The logic has been slightly changed to make sure that the current
document
is used as a fallback value in caseownerDocument
wasundefined
.Testing Instructions
Here's a patch to add a Storybook example that uses a "modern style" ref as the Popover's
anchorRef
Click to expand
Apply the patch on
trunk
and notice how the Storybook example generates an error. Then, apply the same patch on this PR's branch and notice how the Storybook example works as expected.Finally, do a general smoke tests across the codebase to make sure that no regressions are introduced.