Modal bugged in 4.0.0 / final release?

olivermt commented 3 years ago

This fiddle is more or less a straight up copy / paste of the official example at https://thednp.github.io/bootstrap.native/#modalJS Something has happened between Rc2 and 5.0.1 it seems, but I can't figure out what.

I also provide a GIF where I get similar bugs using code that works just fine on 5.0.1-rc2 and bootstrap.native 3.0.15. Ignore the extra attributes, that is just some housekeeping that is used to attach the javascript code to the html element (this is Elixir Liveview / Surface).

However before making this GIF I disabled all Liveview JS and linked in the BSN cdn so I could more easily set up the example. The JSFiddle itself should be repro enough I reckon. modal-bug

thednp commented 3 years ago

I'm testing with the bootstrap-native.js file linked in the <head> and I can confirm manual initialization can work wonky in this case. Also there seem to be some changes in the official BS 5.0.1 style I will have to investigate.

Can you try BSN.initCallback() to initialize?

thednp commented 3 years ago

This is a case where the time you have a manually created instance, your trigger button is not present in the DOM, or when the button is present and its event attached but no Modal is present in the DOM.

This would fix the issue in your case:

document.addEventListener('DOMContentLoaded', function(){
  // var myModalInstance = document.getElementById('myModal').Modal;

  var myModalInstance = new BSN.Modal(
    '#myModal', // target selector
    { // options object
      backdrop: 'static', // we don't want to dismiss Modal when Modal or backdrop is the click event target
      keyboard: false // we don't want to dismiss Modal on pressing [Esc] key
    }
  );  

  document.getElementById('openbtn').addEventListener('click', ()=> {
    myModalInstance.toggle();
  });
});

This is to go around and work without BSN.initCallback(), but you can simply add DATA API attributes to your trigger button if that's the end goal.

olivermt commented 3 years ago

Hey!

I also tried just new BSN.Modal(..).show(); directly and it still didnt work.

The gif I attached is running in an es6 compiled / imported scenario in Liveview. In Liveview I have full control over when the DOM is ready / not ready and this code: https://github.com/surface-ui/surface_bootstrap/blob/main/lib/surface_bootstrap/modal.hooks.js

Has worked perfectly before 5. bootstrap and 4. bootstrap.natve. As you can see I initialize on the DOM element (containing this 'hook') beeing fully mounted and the html is present in the DOM. This is guaranteed by the mounted() callback not running until Morphdom is done modifying the DOM, so there can be no race conditoins etc.

BSN.initCallback() is for bases where you do the global import right? Because in my SurfaceBootstrap code I have access to the BSN as imported from node modules and a quick test now saw no difference in my doing the BSN.initCallback().

Thank you for taking your time to respond :)

thednp commented 3 years ago

I don't know how to debug that code yet nor afford the time to investigate, however here's my 2cents:

like the original, BSN for V5 is now also delivered in ES6+ flavor;
you need to console.log inside your modal.hooks.js file and check what's what and what's NOT;
compare the working code base with current version and find where's the inconsistency (there's very little change between BSN V3.x and V4.x, perhaps the change to ES6 flavor has changed your expected behavior);
please report back any finding.

olivermt commented 3 years ago

Hello,

I appreciate that you can't 'debug' the code, it was more to show that I am simply doing new BSN.Modal(this.el, {...opts}); and that doesn't work anymore.

this.el is a mounted DOM element and not virtual or anything, so I am not quite sure what it is I can change to test otherwise. I'll try to log a bit more to figure out if the .Modal ends up on the DOM element etc.

thednp commented 3 years ago

I'm currently investigating this, it might come to the fact that we recently implemented very strict ESLint and Modal isn't working properly when bootstrap-native.js is linked in the head.

thednp commented 3 years ago

@olivermt here's my finding, it's enough to draw the conclusion:

the ESLint changes as well as ES6 flavor of the V5 distribution has nothing to do with this issue;
linking the bootstrap-native.js in the head and manually initialize and attach events in the footer as you shown in your code will never work, see below;
the right and proper order is 1: bootstrap-native.js 2: scripts.js, if you link the library in the head and add scripts to init in the footer, you must wrap the scripts in a document.addEventListener('DOMContentLoaded',() => {}) listener;
the library will initialize all targets in the DOM on DOMContentLoaded when linked in the <head> or immediately when linked in the <footer> or end of <body>, so attaching an event handler which accesses Modal instance methods will never work, since the DOM isn't loaded yet, targets are all null;

I've updated the scripts.js now for BSN V5 to make use of my solution.

thednp / bootstrap.native

Modal bugged in 4.0.0 / final release? #413