Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails Turbo Frames multiple backdrops issues #698

Open
WozniakMac opened this issue Oct 30, 2023 · 15 comments
Open

Rails Turbo Frames multiple backdrops issues #698

WozniakMac opened this issue Oct 30, 2023 · 15 comments

Comments

@WozniakMac
Copy link

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):

  • OS: MacOS
  • Browser Chrome
  • Version 118.0.5993.117
@zoltanszogyenyi
Copy link
Member

Hey @WozniakMac,

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

Cheers,
Zoltan

@zoltanszogyenyi
Copy link
Member

My first few takes on this:

  • looks like the issue is limited to the Drawer and Modal examples where we check the instances (tried with Popover, Dropdown and it works across pages and turbo load)
  • I'm going to have to take a few more days to debug this as I need to test it via a CDN import map

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

Thanks,
Zoltan

@zoltanszogyenyi
Copy link
Member

zoltanszogyenyi commented Oct 31, 2023

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:

  • go to homepage (modal and drawer works)
  • go to another page (modal and drawer works on those pages)
  • navigate BACK to the homepage -> only the modal and drawer backdrop will be shown

Will investigate further.

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

https://www.unpkg.com/[email protected]/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
Copy link
Author

@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
Copy link
Member

zoltanszogyenyi commented Oct 31, 2023

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/[email protected]/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/[email protected]/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
Copy link
Author

WozniakMac commented Oct 31, 2023

@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
Copy link
Author

@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
Copy link
Author

@zoltanszogyenyi It looks like I fixed it 🎉 🎉
Take a look when you are free #700

@zoltanszogyenyi
Copy link
Member

zoltanszogyenyi commented Nov 9, 2023

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: db93b3e

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
Copy link
Author

@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
Copy link
Member

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
Copy link
Member

@AliOsm
Copy link

AliOsm commented Nov 14, 2023

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:

this._destroyBackdropEl();

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
Copy link

AliOsm commented Nov 15, 2023

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants