Popover: Fix issue with undefined getBoundingClientRect #27445
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirreal
added
[Package] Components
|
Size Change: +7 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
31 tasks
sirreal
requested review from
ellatrix,
ockham,
WunderBart and
fullofcaffeine
|
Oddly enough, the e2e failures do seem to be related 🤔 |
sirreal
force-pushed
the
fix/popover-anchor-undefined-get-bounding-client-rect
branch
from
fb14e14
to
92f0164
Compare
Whoops! The same fix doesn't work for |
|
Can we add a unit test for the scenario you're describing in the PR desc to |
|
When is it throwing this error? We haven't iframed the content yet? |
This seems to be happening in an environment where we iframe the editor, and use a popover for page template selection (a custom feature on our platform). |
ellatrix
reviewed
Dec 2, 2020
| if ( anchorRef instanceof window.Element ) { | ||
| if ( | ||
| anchorRef?.ownerDocument && | ||
| anchorRef instanceof anchorRef.ownerDocument.defaultView.Element |
There was a problem hiding this comment.
Maybe we can just check for .getBoundingClientRect?
There was a problem hiding this comment.
That would probably be more direct 🙂
|
If we decide that this fixes the issue, and if there will be a 9.5.1 to fix the 'Schedule...' button label issue -- can we include this PR also in that patch release? cc/ @ntsekouras @jorgefilipecosta |
ellatrix
reviewed
Dec 2, 2020
| @@ -75,7 +75,10 @@ function computeAnchorRect( | |||
| return getRectangleFromRange( anchorRef ); | |||
There was a problem hiding this comment.
Shouldn't the above be changed too? Could potentially check for .endContainer.
There was a problem hiding this comment.
Yes, I was looking at that and I think the range check is susceptible as well. I'll push the change in a moment.
This reverts commit 16727dc.
sirreal
force-pushed
the
fix/popover-anchor-undefined-get-bounding-client-rect
branch
from
0724a15
to
08c4ff3
Compare
|
I added a check for Range that should work across the iframe boundary as well and used duck-typing (check for methods) to discover Range and Element. |
sirreal
deleted the
fix/popover-anchor-undefined-get-bounding-client-rect
branch
31 tasks
ellatrix
pushed a commit
that referenced
this pull request
Dec 7, 2020
Fix an issue where the `Popover` component throws a runtime `TypeError: Cannot read property 'getBoundingClientRect' of undefined.` This may happen in WordPress if a Popover is rendered in an iframe. It's been observed by rendering a `BlockList` component containing `RichText` in an iframe 😵 The issue is that `instanceof` checks fail across iframe boundaries, where `anchorRef instanceof window.Element` fails because `anchorRef` when it _is_ an instance of Element in the frame but not `window.Element`. Instead of `instanceof` checks that fail across iframe boundaries, check for expected methods as the predicate.
WunderBart
pushed a commit
that referenced
this pull request
Dec 8, 2020
Fix an issue where the `Popover` component throws a runtime `TypeError: Cannot read property 'getBoundingClientRect' of undefined.` This may happen in WordPress if a Popover is rendered in an iframe. It's been observed by rendering a `BlockList` component containing `RichText` in an iframe 😵 The issue is that `instanceof` checks fail across iframe boundaries, where `anchorRef instanceof window.Element` fails because `anchorRef` when it _is_ an instance of Element in the frame but not `window.Element`. Instead of `instanceof` checks that fail across iframe boundaries, check for expected methods as the predicate.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix an issue where the
Popovercomponent throws a runtimeTypeError: Cannot read property 'getBoundingClientRect' of undefined.on this line:gutenberg/packages/components/src/popover/index.js
Line 89 in adacd0a
This may happen in WordPress if a Popover is rendered in an iframe. It's been observed by rendering a
BlockListcomponent containing aRichTextcomponent in an iframe 😵This is the problematic section:
gutenberg/packages/components/src/popover/index.js
Lines 64 to 86 in adacd0a
In the issue I've observed,
anchorRefis anElementon line 78, but it's not an instance ofwindow.Element. It's an instance ofanchorRef.ownerDocument.defaultView.Element😮Both of the conditional blocks are skipped and we end up calling
anchorRef.top.getBoundingClientRect()on an Element. This triggers the error:gutenberg/packages/components/src/popover/index.js
Lines 88 to 90 in adacd0a
You can verify this is problematic by running the following in a recent browser at
about:blank:The issue would apparently begin to manifest after #25332. The iframed issue described above worked with Gutenberg 9.3 but began to throw errors in 9.4.
How has this been tested?
Appears to fix the issue where observed.
Popovers should continue to work in normal usage.
Types of changes
Bug fix
Checklist: