Skip to content

Commit 4330e1a

Browse files
Guard against invalid removeChild (#1707)
* Guard against invalid removeChild * Add proper assertions to defensive DOM removal tests - Add expect.assertions(2) to ensure tests verify expected behavior - Add assertion that tether.enabled is true after enable() - Wrap destroy() calls in expect().not.toThrow() for proper error handling - Fix whitespace and formatting for consistency
1 parent 2dc7a68 commit 4330e1a

File tree

3 files changed

+133
-6
lines changed

3 files changed

+133
-6
lines changed

src/js/tether.js

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,7 @@ class TetherClass extends Evented {
683683

684684
if (getOffsetParent(this.element) !== offsetParent) {
685685
defer(() => {
686-
this.element.parentNode.removeChild(this.element);
686+
this.element?.parentNode?.removeChild(this.element);
687687
offsetParent.appendChild(this.element);
688688
});
689689
}
@@ -719,7 +719,7 @@ class TetherClass extends Evented {
719719
}
720720

721721
if (!offsetParentIsBody) {
722-
this.element.parentNode.removeChild(this.element);
722+
this.element?.parentNode?.removeChild(this.element);
723723
this.element.ownerDocument.body.appendChild(this.element);
724724
}
725725
}
@@ -794,10 +794,18 @@ Tether.modules.push({
794794
},
795795

796796
destroy() {
797-
['target', 'element'].forEach((type => {
797+
['target', 'element'].forEach((type) => {
798+
if (!this.markers || !this.markers[type]) {
799+
return;
800+
}
798801
const el = this.markers[type].el;
799-
this[type].removeChild(el);
800-
}))
802+
try {
803+
el?.parentNode?.removeChild(el);
804+
} catch (e) {
805+
// Marker elements may have already been removed if the parent was removed from DOM
806+
// This is expected behavior in dynamic applications
807+
}
808+
});
801809
},
802810

803811
position({ manualOffset, manualTargetOffset }) {

src/js/utils/bounds.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ export function getVisibleBounds(body, target) {
143143
}
144144

145145
export function removeUtilElements(body) {
146-
if (zeroElement) {
146+
if (zeroElement && zeroElement?.parentNode === body) {
147147
body.removeChild(zeroElement);
148148
}
149149
zeroElement = null;

test/unit/tether.spec.js

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,4 +159,123 @@ describe('Tether', () => {
159159
expect(target.classList.length, 'target - destroy sets classes back to initial state').toEqual(1);
160160
});
161161
});
162+
163+
describe('defensive DOM removal', () => {
164+
it('should handle destroy when target is removed from DOM', () => {
165+
expect.assertions(2);
166+
167+
const tether = new Tether({
168+
element: '.element',
169+
target: '.target',
170+
attachment: 'top left',
171+
targetAttachment: 'top right'
172+
});
173+
174+
tether.enable();
175+
176+
// Verify tether is enabled
177+
expect(tether.enabled).toBe(true);
178+
179+
// Disable first to prevent any positioning attempts
180+
tether.disable();
181+
182+
// Remove target from DOM before destroying tether
183+
document.body.removeChild(target);
184+
185+
// This should not throw an error
186+
expect(() => {
187+
tether.destroy();
188+
}).not.toThrow();
189+
190+
// Clean up element - it might have been moved by tether
191+
if (element.parentNode) {
192+
element.parentNode.removeChild(element);
193+
}
194+
195+
// Put element back in body for afterEach
196+
if (!element.parentNode) {
197+
document.body.appendChild(element);
198+
}
199+
200+
// Reset target to a new one for afterEach
201+
target = document.createElement('div');
202+
target.classList.add('target');
203+
document.body.appendChild(target);
204+
});
205+
206+
it('should handle destroy when element is removed from DOM', () => {
207+
expect.assertions(2);
208+
209+
const tether = new Tether({
210+
element: '.element',
211+
target: '.target',
212+
attachment: 'top left',
213+
targetAttachment: 'top right'
214+
});
215+
216+
tether.enable();
217+
218+
// Verify tether is enabled
219+
expect(tether.enabled).toBe(true);
220+
221+
// Disable first to prevent any positioning attempts
222+
tether.disable();
223+
224+
// Remove element from DOM before destroying tether
225+
const currentParent = element.parentNode;
226+
if (currentParent) {
227+
currentParent.removeChild(element);
228+
}
229+
230+
// This should not throw an error
231+
expect(() => {
232+
tether.destroy();
233+
}).not.toThrow();
234+
235+
// Reset element for afterEach
236+
element = document.createElement('div');
237+
element.classList.add('element');
238+
document.body.appendChild(element);
239+
});
240+
241+
it('should handle destroy when both element and target are removed from DOM', () => {
242+
expect.assertions(2);
243+
244+
const tether = new Tether({
245+
element: '.element',
246+
target: '.target',
247+
attachment: 'top left',
248+
targetAttachment: 'top right'
249+
});
250+
251+
tether.enable();
252+
253+
// Verify tether is enabled
254+
expect(tether.enabled).toBe(true);
255+
256+
// Disable first to prevent any positioning attempts
257+
tether.disable();
258+
259+
// Remove both from DOM before destroying tether
260+
if (element.parentNode) {
261+
element.parentNode.removeChild(element);
262+
}
263+
if (target.parentNode) {
264+
target.parentNode.removeChild(target);
265+
}
266+
267+
// This should not throw an error
268+
expect(() => {
269+
tether.destroy();
270+
}).not.toThrow();
271+
272+
// Reset for afterEach
273+
element = document.createElement('div');
274+
element.classList.add('element');
275+
document.body.appendChild(element);
276+
target = document.createElement('div');
277+
target.classList.add('target');
278+
document.body.appendChild(target);
279+
});
280+
});
162281
});

0 commit comments

Comments
 (0)