Skip to content
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

Fix for detached Node element exception #389

Merged
merged 1 commit into from
Sep 7, 2022
Merged

Conversation

twokul
Copy link
Contributor

@twokul twokul commented Sep 6, 2022

Insert the virtual component bound back to make sure Glimmer is not confused about the state of the DOM resulting in DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

I was unable to add a failing test for this but I have added a few test cases to improve test coverage and verified the fix in a consuming application.

Insert the virtual component bound back to make sure Glimmer is not confused about the state of the DOM resulting in `DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.`

I was unable to add a failing test for this but I have added a few test cases to improve test coverage and verified the fix in a consuming application.
@twokul twokul marked this pull request as ready for review September 6, 2022 16:41
@twokul twokul requested a review from mixonic September 6, 2022 16:41
Copy link
Contributor

@mixonic mixonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discovered through test coverage in Addepar's internal codebases, and the fix verified. I'd really prefer to have a test case covering it since this code is so subtle, but I'm also not willing to block landing this patch on ideal coverage.

Approved!

@mixonic mixonic merged commit fc6c710 into master Sep 7, 2022
@mixonic mixonic deleted the twokul/dom-exception branch September 7, 2022 13:32
@@ -539,6 +539,9 @@ export default class Radar {
if (item) {
insertRangeBefore(this._domPool, null, component.realUpperBound, component.realLowerBound);
} else {
// Insert the virtual component bound back to make sure Glimmer is
// not confused about the state of the DOM.
insertRangeBefore(this._itemContainer, null, component.realUpperBound, component.realLowerBound);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tracked this down in our app. Specifically the fail case is as follows:

A virtual component is still in the buffer pool at the point it is removed but has been scrolled out of the rendered area. So for instance if you have a collapsible tree structure and scroll up to the top of an open section some items in the lower portion of the section might be de-rendered but still in the pool. If you then collapse the tree this error will be triggered.

While this fix seems good enough, there's probably a more correct fix to be found here that addresses both this and the original issue the run below is papering over. I also suspect that run should be run.join.

At a minimum, I think we should follow up with a check for whether we are in the document-fragment or still in the DOM, if we are not in the fragment there's no reason to re-insert in this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linking #358 which introduced this error as part of trying to fix a similar fail case elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants