Skip to content

Fix for detached Node element exception #389

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 7, 2022
Merged

Fix for detached Node element exception #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions addon/-private/data-view/radar/radar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@runspired runspired Sep 8, 2022

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

@runspired runspired Sep 8, 2022

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

run(() => {
virtualComponents.removeObject(component);
});
Expand Down
37 changes: 37 additions & 0 deletions tests/acceptance/acceptance-tests/record-array-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';

import { click, find, findAll, visit as newVisit } from '@ember/test-helpers';
import scrollTo from '../../helpers/scroll-to';

module('Acceptance | Record Array', function(hooks) {
setupApplicationTest(hooks);
Expand All @@ -14,6 +15,42 @@ module('Acceptance | Record Array', function(hooks) {
assert.equal(find('number-slide:last-of-type').textContent.replace(/\s/g, ''), '14(14)', 'correct last item rendered');
});

test('RecordArrays update correctly after scrolling and updating items', async function(assert) {
await newVisit('/acceptance-tests/record-array');

assert.equal(findAll('number-slide').length, 15, 'correct number of items rendered');

await scrollTo('.table-wrapper', 0, 600);

await click('#update-items-button');

assert.equal(findAll('number-slide').length, 5, 'correct number of items rendered');
});

test('RecordArrays update correctly after partial update', async function(assert) {
await newVisit('/acceptance-tests/record-array');

assert.equal(findAll('number-slide').length, 15, 'correct number of items rendered');

await click('#partial-update-button');

assert.equal(findAll('number-slide').length, 5, 'correct number of items rendered');
});

test('RecordArrays update correctly after being hidden and shown', async function(assert) {
await newVisit('/acceptance-tests/record-array');

assert.equal(findAll('number-slide').length, 15, 'correct number of items rendered');

await click('#hide-vc-button');

assert.equal(findAll('number-slide').length, 0, 'correct number of items rendered');

await click('#show-vc-button');

assert.equal(findAll('number-slide').length, 15, 'correct number of items rendered');
});

test('RecordArrays updates correctly after deleting items', async function(assert) {
await newVisit('/acceptance-tests/record-array');

Expand Down
14 changes: 14 additions & 0 deletions tests/dummy/app/routes/acceptance-tests/record-array/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,29 @@ import { inject as service } from '@ember/service';
export default Controller.extend({
store: service(),
prefixed: true,
vcShown: true,

actions: {
updateItems() {
this.get('store').unloadAll('number-item');
this.get('store').query('number-item', { length: 5 });
},

partialUpdate() {
let length = this.model.content.length;
this.set('model', this.model.toArray().removeAt(0, length - 5));
},

showPrefixed() {
this.toggleProperty('prefixed');
},

hideVC() {
this.set('vcShown', false);
},

showVC() {
this.set('vcShown', true);
}
}
});
21 changes: 13 additions & 8 deletions tests/dummy/app/routes/acceptance-tests/record-array/template.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
<div class="table-wrapper dark">
{{#vertical-collection this.model
estimateHeight=50
bufferSize=5
as |item index|}}
{{number-slide item=item itemIndex=index prefixed=this.prefixed}}
{{/vertical-collection}}
</div>
{{#if this.vcShown}}
<div class="table-wrapper dark">
{{#vertical-collection this.model
estimateHeight=50
bufferSize=5
as |item index|}}
{{number-slide item=item itemIndex=index prefixed=this.prefixed}}
{{/vertical-collection}}
</div>
{{/if}}

<button id="hide-vc-button" {{action "hideVC"}}>Hide VC</button>
<button id="show-vc-button" {{action "showVC"}}>Show VC</button>
<button id="partial-update-button" {{action "partialUpdate"}}>Partial Update</button>
<button id="update-items-button" {{action "updateItems"}}>Update items</button>
<button id="show-prefixed-button" {{action "showPrefixed"}}>Show prefixed</button>