Skip to content

Conversation

boddunan
Copy link
Contributor

@boddunan boddunan commented May 6, 2021

Fixes #33840

The backdrop rootElement is not initialized when the javascript file loaded in the document head as the document.body is not yet initialized and Default variable in backdrop.js is loaded before it.

@boddunan boddunan requested a review from a team as a code owner May 6, 2021 17:04
@GeoSot
Copy link
Member

GeoSot commented May 6, 2021

@boddunan Seems a nice solution, but I suppose it would be more valid to solve the root of the problem on backdrop.js

do you want to give it a try?

@boddunan
Copy link
Contributor Author

boddunan commented May 7, 2021

Thank you @GeoSot for the feedback. Intializing rootElement with document.body before the document loaded would fail anyway. We have 3 options to fix it as far as I think. Please suggest.

  1. Initialize the "Default" variable inside the constructor which is executed after dom load by the plugins
  2. Intialize Default.rootElement with document.body before expanding the config values from "Default" object
  3. Intialize the Default value of rootElement on DOMContentLoaded, if it is null

Maybe there is some other good alternative, please let me know.

@GeoSot
Copy link
Member

GeoSot commented May 7, 2021

Probably I would choose the No2 (Default.rootElement || document.body) or would wait #33327 using the body selector as string

@boddunan
Copy link
Contributor Author

boddunan commented May 7, 2021

Thanks. I changed the fix to use method 2. Considering the critical nature of the issue, I would like to fix the issue asap and then apply changes from the other PR when it is merged.

@GeoSot
Copy link
Member

GeoSot commented May 7, 2021

Something like this, would be more streamlined with the rest code base

_getConfig(config) {
    
    config = {
      ...Default,
      ...(typeof config === 'object' ? config : {})
}
config .rootElement = config .rootElement || document.body
.....

(rootElement in line 14 better to be null)

Plus, at least a test

@boddunan
Copy link
Contributor Author

boddunan commented May 8, 2021

Done. Added tests and moved the intiialization part to _getConfig

@GeoSot GeoSot added js p1 Critical, and inhibits core functionality v5 labels May 9, 2021
@XhmikosR XhmikosR changed the title Fix backdrop rootElement is not initialized in Model Fix backdrop rootElement is not initialized in Modal May 10, 2021
@XhmikosR XhmikosR changed the title Fix backdrop rootElement is not initialized in Modal Fix backdrop rootElement not initialized in Modal May 10, 2021
@XhmikosR XhmikosR merged commit 741fa58 into twbs:main May 10, 2021
@boddunan boddunan deleted the patch-1 branch May 11, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
js p1 Critical, and inhibits core functionality v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initializing Modal through JS throws Uncaught TypeError: BACKDROP
3 participants