Fix backdrop "Cannot read property 'removeChild' of null" when removed from body

[weaverryan]

Hi!

This fixes #33946

Here is a reproducer: https://weaverryan.github.io/bootstrap-modal-close-reproducer/
Code: https://github.com/weaverryan/bootstrap-modal-close-reproducer/blob/main/index.html#L34-L46

1. Click "Launch demo modal"
2. Click "Save changes" in the modal.

Then in the console you'll see:

TypeError: Cannot read property 'removeChild' of null
(backdrop.js:119)

This occurs if you hide() a backdrop, but then the HTML of the backdrop is removed from the body while its animation is finishing. From the reproducer:

bootstrap.Modal.getInstance(document.querySelector('#exampleModal')).hide();
document.body.innerHTML = 'Page content changed';

When would this happen? It happens, for example, if you're using Turbo. For example, if you close a modal, then immediately "visit" another page, in reality, that just swaps the body.innerHTML for the page, which causes backdrop to error when it can't find its parent (after the animation).

Please let me know if you need any more clarification or tweaks to the PR :).

Thanks!

[weaverryan]

FYI - without the fix, this test causes this failure:

Chrome Headless 90.0.4430.212 (Mac OS 10.15.7) Backdrop should not error if the backdrop no longer has a parent FAILED
	Uncaught TypeError: Cannot read property 'removeChild' of null thrown

[GeoSot]

/CC @alpadev @rohit2sharma95

this is similar to #33727. We have the asynchronous process error

The script that triggers that causes the problem, deletes everything without wait for transitionend

document.querySelector('#save-changes').addEventListener('click', (event) => {
      bootstrap.Modal.getInstance(document.querySelector('#exampleModal')).hide(); // THIS IS AN ASYNC PROCESS
      document.body.innerHTML = 'Page content changed (e.g. faking an AJAX navigation). Check the console for the error.'; //THIS remove everything (plus parentElement) synchronously 
  });

Proposal: as alert, tooltip, backdrop, use the same process and checks, isn't safer to extract the functionality to utils or to Manipulator.remove() instead to make inline checks?

[weaverryan]

Proposal: as alert, tooltip, backdrop, use the same process and checks, isn't safer to extract the functionality to utils or to Manipulator.remove() instead to make inline checks?

I see what you're talking about - it looks like it's already handled correctly for the other spots:

• tooltip
• alert

So at least we know this PR is consistent with behavior elsewhere.

As far as extracting it to util/, I'm fine either way. Are you thinking... like a removeElementFromParent(element) function - in index.js?

Cheers!

[rohit2sharma95]

Proposal: as alert, tooltip, backdrop, use the same process and checks, isn't safer to extract the functionality to utils or to Manipulator.remove() instead to make inline checks?

Makes sense @GeoSot, but that can be done separately IMO.

PS: I am not sure why the tests are not being run here but failed when running locally on my machine.

[GeoSot]

@weaverryan , agreed with Rohit.

Lets fix it here, and you can open a next pr, gathering all these

[weaverryan]

I've just made the updates and rebased :).

Unfortunately, it looks like there are some pesky test problems. I can't repeat them locally, but each time the tests run here, the same test fails on 1 or 2 of the jobs - sometimes its Node 12, sometimes 14, etc :/.

Here's an example: https://github.com/twbs/bootstrap/pull/34014/checks?check_run_id=2640394953

I can't figure out how the new behavior would be affecting this other test :/

[alpadev]

The flakyness can be explained by jasmine running the tests in random order. So if some certain test is ran before the other, things could fail that wouldn't fail in a different order.

I suspect the problem here:
https://github.com/twbs/bootstrap/blob/e0b81f331dc60ddac1e6e881b49dc3696fca16c5/js/tests/unit/util/backdrop.spec.js#L144

This will remove our fixture element and may lead to problems.

Replacing that with something like document.body.removeChild(instance._getElement()) should do it.

Edit: or you can do fixtureEl = getFixture(), after you wiped the body - I guess.

[alpadev]

Looks like that did the job. 😊

LGTM 👍