themesberg / flowbite

Open-source UI component library and front-end development framework based on Tailwind CSS
https://flowbite.com
MIT License
7.95k stars 745 forks source link

Rails Turbo Frames multiple backdrops issues #698

Open WozniakMac opened 1 year ago

WozniakMac commented 1 year ago

Describe the bug When loading frame in rails with turbo it is initialising all components again which makes modals and drawers create multiple backdrops. It sometimes makes screen grey and you need to refresh the whole window.

It looks like this code is responsible for it. https://github.com/themesberg/flowbite/blob/main/src/index.turbo.ts#L31-L44 Instead of initialising only new components it initialise those which are already initialised again. And that makes multiple backdrops.

Removing it fixes the issue but it will stop initialising new components in turbo frames.

To Reproduce Steps to reproduce the behaviour: lazy turbo frame Demo: https://github.com/WozniakMac/flowbite-issue-demo

Desktop (please complete the following information):

zoltanszogyenyi commented 1 year ago

Hey @WozniakMac,

Thanks for reporting, I'll have a look tomorrow.

Cheers, Zoltan

zoltanszogyenyi commented 1 year ago

My first few takes on this:

I'll also post a Ruby on Rails starter on GitHub with Flowbite pre-installed.

Thanks, Zoltan

zoltanszogyenyi commented 1 year ago

Fixed the double/multiple backdrop issue for the modal. It was an init check issue. Will be added to v2.0.1.

The situation is as follows:

Will investigate further.

By the way, you can test the new modal fix by pinning this JS resource:

https://www.unpkg.com/flowbite-2.0@2.0.1/dist/flowbite.turbo.min.js

Until a fix is provided, for all Ruby on Rails users, I recommend using the latest stable version of Flowbite pre-v2.0 as there are no differences of features other than the main Instance Manager update.

WozniakMac commented 1 year ago

@zoltanszogyenyi Thanks mate! It looks like this time modal stopped working and I can't open it twice :D. Only first modal open and only once. After it all modal stopped working :D. Could be because I load turbo frame after opening the modal. I'll try to dig into it deeper later. Maybe my first diagnose was not fully correct.

Btw. I think you create new package by mistake https://www.npmjs.com/package/flowbite-2.0 https://www.npmjs.com/package/flowbite

modal

zoltanszogyenyi commented 1 year ago

Hey @WozniakMac,

Thanks for testing - it's a new package because I'm just testing things out :)

Released one more package:

https://www.unpkg.com/flowbite-2.0@2.0.3/dist/flowbite.turbo.js

This should always override the instances for the modals and drawers.

I think that if you want to dig deeper, you can fork the Flowbite project and make releases on a new repo name, something like flowbite-rails-test and then just use the automatically generated UNPKG CDN file to test on your local lib:

https://www.unpkg.com/flowbite-rails-test@2.0.1/dist/flowbite.turbo.min.js

All you need to do when you have forked the Flowbite lib is to run npm run build:npm and then npm publish --access=public, but make sure that you change the name of the package in package.json.

These are the files that you need to check out:

https://github.com/themesberg/flowbite/blob/main/src/index.turbo.ts (this is where we set up the turbo and frame load events) https://github.com/themesberg/flowbite/blob/main/src/dom/instances.ts (this is the main instance manager class that handles instances) https://github.com/themesberg/flowbite/blob/main/src/dom/events.ts (this is the class that sets up the events) https://github.com/themesberg/flowbite/blob/main/src/components/modal/index.ts (main modal component file where you also have initModals where we check the instances, this is the main part we need to debug) https://github.com/themesberg/flowbite/blob/main/src/components/drawer/index.ts (same as with the modal, but it's the drawer)

I've debugged this for over 3 hours and couldn't find a fix and I'm also not very experienced with Ruby on Rails, so your help here would be invaluable. Either way, I'll check this in the following days too. I'm here to help and collaborate on a fix.

You also may want to check for the FlowbiteInstances global object to see which instances are being created and overriden.

Cheers, Zoltan

WozniakMac commented 1 year ago

@zoltanszogyenyi Forgot to mention. This issue was on 1.8.1 too. The other issue I reported was from 2.0.0.

For now I have a workaround for both issues. I use my own package on gihub based on 1.8 with support for load-frame events turned off. It works fine because I don't need to load flowbite components in frames. https://github.com/WozniakMac/flowbite/pkgs/npm/flowbite

Tested 2.0.3. It's funny too :D

modal-2

WozniakMac commented 1 year ago

@zoltanszogyenyi I found out what is the issue. Every time you call initModals it adds new event listener.

https://github.com/themesberg/flowbite/blob/main/src/components/modal/index.ts#L304

Because of this after loading turbo frame on page it adds new event listener to modal button which calls show and hide on click twice or x-times turbo frames on page.

Telling you so you are not wasting time debugging. Trying to figure out how to fix it now

WozniakMac commented 1 year ago

@zoltanszogyenyi It looks like I fixed it 🎉 🎉 Take a look when you are free https://github.com/themesberg/flowbite/pull/700

zoltanszogyenyi commented 1 year ago

Hey @WozniakMac,

Appreciate the PR!

I did my own fix though, because I wanted to keep everything nice and tidy in terms of classes.

Here's my commit that fixes the issue: https://github.com/themesberg/flowbite/pull/678/commits/db93b3ed61887406823831f1f15933236f09e20e

Basically, I keep a store of all event listeners for each instance and destroy them when also destroying the instance.

The problem with your solution is that you created another instance manager when we already had one.

Please install this version and test it on your repo: https://www.npmjs.com/package/flowbite-2.1.0

This will be pushed to v2.1.0 later today or tomorrow.

Thanks! Zoltan

WozniakMac commented 1 year ago

@zoltanszogyenyi It works fine until it loads anything with "turbo:frame-load". Then it breaks. You can debug it on this app I prepared. Just pin it to local files with yarn add file:./../flowbite and then you need to build flowbite and run yarn add file:./../flowbite every time you change something in flowbite to see changes in rails.

https://github.com/WozniakMac/flowbite-issue-demo Just clone it. Download rbenv and install ruby. Then run ./bin/dev

I'm going to use my fork for now.

zoltanszogyenyi commented 1 year ago

Hey @WozniakMac ,

Thanks for the feedback.

What exactly breaks? Right now, the event listeners are being added for each new instance.

I'll take a look tomorrow - this works for my local turbo frame load repository.

Cheers, Zoltan

zoltanszogyenyi commented 1 year ago

Please make sure you use this package:

https://www.npmjs.com/package/flowbite-2.1.0

AliOsm commented 1 year ago

I have the same problem with sidebar drawer. I tried the package you mentioned and the same issue still exists. In the first page load I got 2 messages saying add new drawerdrawer-navigation one from initDrawers2 and another one from initDrawers function (In this order). When I open the sidebar and navigate to another page I got add new drawerdrawer-navigation message from initDrawers2, then Drawer3 raises an error in this function:

Drawer3.prototype._destroyBackdropEl = function() {
  if (this._visible) {
    document.querySelector("[drawer-backdrop]").remove();
  }
};

Setting data-drawer-backdrop="false" didn't solve the problem, flowbite is always trying to remove the backdrop here:

https://github.com/themesberg/flowbite/blob/cc5320a39b42e65dde03916a66e513560d0ee363/src/components/drawer/index.ts#L86

Any ideas on how to solve this? Maybe just fix the always deleting issue, and we can use data-drawer-backdrop="false" for now until the bigger issue got resolved?

AliOsm commented 1 year ago

To hack the issue for now, I disabled backdrops, and added <div drawer-backdrop=""></div> to the end of my body. In this way everything works fine for the drawer sidebar.

gacharles23 commented 11 months ago

I'm seeing the same issue as @AliOsm - any navigation away from a drawer link results in Cannot read properties of null (reading 'remove') for this code:

_destroyBackdropEl() {
  if (this._visible) {
    document.querySelector('[drawer-backdrop]').remove();
  }
}

Definitely related to Turbo because if you add data-turbo=false to the link then it works without error.

Great suggestion to add the empty div at the end of the body!! Hopefully we get a cleaner fix soon.