Popover: Check anchorDocument default view before removing events #35832
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
Mamaduka
added
[Package] Components
|
Size Change: +26 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Oct 21, 2021
There was a problem hiding this comment.
From what I could tell, focusing the content in a Mobile/Tablet preview changes the anchor.ownerDocument to the one in the iframe, rather than the main window.
- Show a Tablet preview.
- Click on the post content. (
anchorDocument.defaultViewis the iframe) - Click Preview button. (
anchorDocument.defaultViewis main window) - Click the Desktop option, and the Preview popover closes.
- The active element (and thus the
anchorDocument) immediately changes back to the one in the iframe. - The effect cleanup function runs right when the iframe is already gone but the
anchorDocumentis still in memory.
Something like that?
If this is what’s happening, then I guess the optional chaining fix won’t cause any issues? I don’t know if there are better fixes, but given that this is an active crash bug, I think it’s good to merge a fix ASAP.
|
Thanks for the review, @mirka. Yes, that's similar to my findings. However, I'm also not sure how to fix the cause of the issue. So let's merge this band-aid fix for now. |
|
Is there a way to use |
|
I can't really think of a solution using focus/blur events, but maybe one thing we could do to prevent memory leaks here is to also remove the resize/scroll listeners in a |
Sounds like an improvement we should apply regardless of what we're trying to solve here, so I'd say yes! |
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
PR adds optional chaining before removing events on side effect cleanup.
I could not track down why
anchorDocument.defaultViewis null with these specific steps. So this is more like treating the symptom than a cause.Fixes #35807.
How has this been tested?
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).