Backdrop: Fix stale body by removing unnecessary default #34092

weaverryan · 2021-05-24T14:20:34Z

Hi there!

I'm using Turbo, where the entire body element is swapped out at times. This causes a bug with Backdrop, as it stores the original body element in the Default object. This is the flow:

Open a Modal / Backdrop
Navigate to another page (i.e. swap the body for a new body Element)
Open another Modal / Backdrop

On (3), the Backdrop will append itself to the original body Element from (1), not the new body element.

Also, the default is unnecessary: in _getConfig(), if config.rootElement is falsy, then it already defaults to document.body (which is why I didn't add any tests: there are tests already to make sure the rootElement can be configured):

bootstrap/js/src/util/backdrop.js

Line 93 in 1366659

config.rootElement = config.rootElement || document.body

Please let me know if any tweaks are needed.

Thanks!

GeoSot · 2021-05-24T14:23:07Z

try to put any comments for future reference 😃

weaverryan · 2021-05-24T14:24:39Z

try to put any comments for future reference 😃

Oh! Do you mean comments in the code to explain the "bug"? I appreciate you helping me navigate the ecosystem over here - it's still new to me 😄

GeoSot · 2021-05-24T14:28:49Z

Yes. simple Comments that will guide us on future needs and changes.
it is valid to think of the body change, however, turbo changes only the body innerHTML, not the same? right?

You also can try use the:

rootElement: 'body',

and inside _getConfig to try use the getElement() helper

weaverryan · 2021-05-24T14:45:40Z

Yes. simple Comments that will guide us on future needs and changes.

👍

it is valid to think of the body change, however, turbo changes only the body innerHTML, not the same? right?

That had also been my impression, but it looks like it's not the case: https://github.com/hotwired/turbo/blob/aae03ada8be4b46899330364d712cc1ae57f2400/src/core/drive/page_renderer.ts#L91-L92

and inside _getConfig to try use the getElement() helper

I've made that change :). The "pro" is that the Default.rootElement is now set again, which is nice for clarity. The diff is a bit bigger - let me know what you think.

GeoSot · 2021-05-24T15:50:19Z

Before force pushing you commits, try to leave them alive for a bit. The previous commit was more valid. Try to check why was failing

weaverryan · 2021-05-24T16:07:00Z

Before force pushing you commits, try to leave them alive for a bit. The previous commit was more valid. Try to check why was failing

Sorry about that - I have that same pet peeve when I'm the maintainer :). I can always rebase/squash a bit later.

So there are 2 possible approaches:

The current approach. The tests were failing momentarily, because I had this in _getConfig():

config.rootElement = getElement(config.rootElement)

The problem is that the user SHOULD be allowed to explicitly pass rootElement: null when instantiating and it should default to document.body. With the above code, getElement() doesn't like the null value. That's why the PR was updated to fix this:

-config.rootElement = getElement(config.rootElement)
+config.rootElement = config.rootElement ? getElement(config.rootElement) : document.body

The totally different approach was my original, where the only change was in the Default options object:

const Default = {
  // ...
-  rootElement: document.body,
+  rootElement: null,
}

If you'd like any changes - or if you think option (2) might be nicer, please let me know. Thanks for the comments already :)

Cheers!

GeoSot · 2021-05-24T16:20:19Z

did you take into consideration that may the tests where wrong scoped to a false expectation? ;)

To help you on this, I am referring to this

The initial problem was that if someone had the bootstrap script in the <head> element the body wasn't initialized so , the _getConfig was failing. So the test was wrote in purpose, with null element.
However, if a developer is so sure of his choice, we should accept null (yes will throw exception) , and be default the backdrop will be able to resolve the 'body'

To come up to an end, I believe that the test is wrong (with the existing codebase)

/cc @alpadev any help on this?

alpadev · 2021-05-24T17:14:32Z

Not sure why someone would pass in null for the rootElement. Also what would be the expected result if a user does so. Use some default e.g. document.body? Throw an error?

Given that we validate the config parameters and they are defined like

const DefaultType = {
  isVisible: 'boolean',
  isAnimated: 'boolean',
  rootElement: 'element',
  clickCallback: '(function|null)'
}

it shouldn't be possible to use null, IMO. I know there was this fallback added to make it work in case the script is executed when there is no document.body available (e.g. in the head section). But I think this approach contradicts the idea of validating the user input in general.

alpadev · 2021-05-24T17:27:14Z

I mean, I was thinking of a different approach back then (#33889). But since I missed there was already some PR in progress and someone started to argue that my solution might be overkill, I closed this in favor of the merged one. I guess my solution would have worked for your use-case too, so not sure what to propose here.

Also wouldn't it be possible to explicitly pass in document.body (or 'body' in case getElement will be used) when the modal is executed to make it work with Turbo?

weaverryan · 2021-05-24T19:01:47Z

Hi!

Ok then, I've pushed another approach. This:

A) Does not allow null to be explicitly passed (I could add a test asserting an error if we want to be that explicit about it). This is a behavior change - but it seems there is some consensus that purposely passing rootElement: null is nonsense (and it doesn't fit the 'element' type advertised.

B) This is still "lazy" thanks to getElement(): we won't try to find the rootElement until Backdrop is instantiated. This means that it will still work for #33840. In fact, what we're doing here is making rootElement even lazier: making sure we always fetch a fresh rootElement on each instantiation, instead of allowing it to be fetched once, then reused forever after.

Also wouldn't it be possible to explicitly pass in document.body (or 'body' in case getElement will be used) when the modal is executed to make it work with Turbo?

That's interesting. If I understand you correctly, you're saying that an alternative fix would be to "leave Backdrop alone", but explicitly pass rootElement: document.body from Modal._initializeBackDrop(). Yes, that would also definitely work :)

Cheers!

alpadev · 2021-05-25T15:50:34Z

Your last approach seems valid to me. 👍 It follows the idea of delaying things until the component is used and by using a string we make sure that some replaced element wouldn't be stuck in the memory because we hold a reference to it.

On a sidenote, initially we pushed that implementation in kind of a rush because it prevented people from using the script files in the head section and blocked their development. It wasn't the best solution at that time but solved the problem people had.

@GeoSot WDYT? It's your baby 😊

The current config can cause the "body" to become stale. Specifically, if the entire body element is swapped out for a new body element, then the backdrop will continue to append itself to the original body element, since it's stored in memory as a reference on this object. This also no longer allows an explicit null to be passed to Backdrop's rootElement This still accomplishes the laziness of "not finding the rootElement until the Backdrop is created" to avoid problems of the JavaScript being included inside <head> (so, before body is available).

GeoSot · 2021-05-31T08:00:48Z

@GeoSot WDYT? It's your baby blush

🤣 ❤️

Seems fine to my eyes, too

The current config can cause the "body" to become stale. Specifically, if the entire body element is swapped out for a new body element, then the backdrop will continue to append itself to the original body element, since it's stored in memory as a reference on this object. This also no longer allows an explicit null to be passed to Backdrop's rootElement This still accomplishes the laziness of "not finding the rootElement until the Backdrop is created" to avoid problems of the JavaScript being included inside <head> (so, before body is available).

weaverryan requested a review from a team as a code owner May 24, 2021 14:20

GeoSot added js v5 labels May 24, 2021

weaverryan force-pushed the fresh-backdrop-body branch from 24f8a09 to e94beac Compare May 24, 2021 14:45

weaverryan force-pushed the fresh-backdrop-body branch from e94beac to 64fcbd7 Compare May 24, 2021 14:53

weaverryan force-pushed the fresh-backdrop-body branch from 64fcbd7 to 767ad7c Compare May 24, 2021 16:08

weaverryan force-pushed the fresh-backdrop-body branch from beb2691 to 3094983 Compare May 26, 2021 14:53

Merge branch 'main' into fresh-backdrop-body

6fb8b84

GeoSot approved these changes May 31, 2021

View reviewed changes

Merge branch 'main' into fresh-backdrop-body

2d384b9

XhmikosR changed the title ~~[Backdrop] Fixing stale body by removing unnecessary default~~ Backdrop: Fix stale body by removing unnecessary default May 31, 2021

XhmikosR merged commit 0cb70e2 into twbs:main May 31, 2021

weaverryan deleted the fresh-backdrop-body branch May 31, 2021 23:25

Uh oh!

Backdrop: Fix stale body by removing unnecessary default #34092

Backdrop: Fix stale body by removing unnecessary default #34092

Uh oh!

Conversation

weaverryan commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoSot commented May 24, 2021

Uh oh!

weaverryan commented May 24, 2021

Uh oh!

GeoSot commented May 24, 2021

Uh oh!

weaverryan commented May 24, 2021

Uh oh!

GeoSot commented May 24, 2021

Uh oh!

weaverryan commented May 24, 2021

Uh oh!

GeoSot commented May 24, 2021

Uh oh!

alpadev commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alpadev commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan commented May 24, 2021

Uh oh!

alpadev commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoSot commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

weaverryan commented May 24, 2021 •

edited

Loading

alpadev commented May 24, 2021 •

edited

Loading

alpadev commented May 24, 2021 •

edited

Loading

alpadev commented May 25, 2021 •

edited

Loading

GeoSot commented May 31, 2021 •

edited

Loading