Problem/Motivation

Bootstrap 5 has different attribute names for the modal options:

  • data-dismiss --> data-bs-dismiss
  • data-keyboard --> data-bs-keyboard
  • data-backdrop --> data-bs-backdrop

However, the modal-page-modal.twig.html always uses the bs3 names, so dismiss buttons don't work and changing the keyboard / the backdrop configurations has no effect when using bs5.

Steps to reproduce

  1. Go to /admin/config/user-interface/modal-page/settings and select Bootstrap 5 in the version dropdown.
  2. Go to /admin/structure/modal and edit a modal
  3. On the Modal buttons tab, check "Display button X to close"
  4. On the Modal close tab, uncheck both "Close Modal pressing ESC key" and "Close Modal clicking outside the Modal" options.

Result:

The modal will show properly, but:

  • The X button will not close the modal
  • The overlay background will close the modal (the default bs5 behaviour)
  • Pressing ESC will close the modal (the default bs5 behaviour)
CommentFileSizeAuthor
#16 AwesomeScreenshot-7_13_2023,6 55 57PM.mp41.83 MBaldibier
#4 3326868-4.patch4.5 KBjnlar

Issue fork modal_page-3326868

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ruthcj created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 5.0.0 » 5.0.x-dev
Assigned: ruthcj » Unassigned

I am resetting the Assigned field, since it should be used from people who are going to work on the issue in the next hours, not from people who need more than 12 hours before starting to work on the issue or more than 12 hours to complete the work on the issue.

renatog’s picture

renatog commented
Issue tags: +Novice

Hello @ruthcj how are you?

Thanks a lot for using and for reporting. It helps a lot!

Yeah, good catch. We need to work on this.

My agenda is full on these days but seem that isn't so complex to fix so I'm putting the tag "Novice" to see if someone in the Drupal.org have interest to try to help contributing

jnlar’s picture

jnlar
Primary language English
Location Sydney, Australia
commented
Assigned: Unassigned » jnlar
Status: Active » Needs review
StatusFileSize
new 3326868-4.patch4.5 KB
jnlar’s picture

jnlar
Primary language English
Location Sydney, Australia
commented

Attached a patch that'll alternate the data attributes, noticed a typo as well:

-  $bootstrapVersion = \Drupal::config('modal_page.settings')->get('boostrap_version');
+  $bootstrapVersion = \Drupal::config('modal_page.settings')->get('bootstrap_version');

Which was affecting some logic in js/modal-page.js

renatog’s picture

renatog commented
Issue tags: +Needs manual testing

@jonarcher really good catch! Thanks a lot

@ruthcj could you verify if patch #4 solves your issue, please?

If yes I'll commit the fix for us

Thank you so much, team

shubham rathore’s picture

shubham rathore commented
Assigned: jnlar » shubham rathore
shubham rathore’s picture

shubham rathore commented
Assigned: shubham rathore » Unassigned

Hi patch#4 work properly for me & resolve the issue.
Thank You

sahilgidwani’s picture

sahilgidwani commented
Assigned: Unassigned » sahilgidwani
sahilgidwani’s picture

sahilgidwani commented
Status: Needs review » Reviewed & tested by the community

@RenatoG, I applied patch and it is working fine as per the problem description.

SahilGidwani opened merge request !31

sahilgidwani’s picture

sahilgidwani commented

@RenatoG, I have created a issue fork and applied patch and created MR so that it can be merged easily. Please review the PR.

sahilgidwani’s picture

sahilgidwani commented
Assigned: sahilgidwani » Unassigned
kevinquillen’s picture

kevinquillen commented
Priority: Normal » Major

This is pretty major and needs to be fixed. Selecting Bootstrap 5 doesn't work at all, and since that is the latest version most people are probably going to select that.

kevinquillen’s picture

kevinquillen commented
Status: Reviewed & tested by the community » Needs review

Setting back because there are new commits.

aldibier’s picture

aldibier
Primary language Spanish
Location Manizales
commented
StatusFileSize
new AwesomeScreenshot-7_13_2023,6 55 57PM.mp41.83 MB

Hi,

I've tested this fix using the branch with the latest commits and everything looks good.

This test was performed using Drupal 9.5 and Drupal 10.1 in both versions the fix works perfectly!

I'm attaching a video with all steps.

aldibier’s picture

aldibier
Primary language Spanish
Location Manizales
commented
Status: Needs review » Reviewed & tested by the community

renatog’s picture

renatog commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Novice, -Needs manual testing

Thanks everyone

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.