thednp / bootstrap.native

Bootstrap components build with Typescript
http://thednp.github.io/bootstrap.native/
MIT License
1.7k stars 177 forks source link

Modal Trigger does not trigger modal when it has more than 1 child element #368

Closed mmottau closed 4 years ago

mmottau commented 4 years ago

Version: 2.0.27 Script: /lib/V3/modal-native.js Line numbers: 138 and 158

Markup:

<button type="button" class="btn btn-warning btn-lg" data-toggle="modal" data-target="#myModal">
  <span><i class="glyphicon glyphicon-heart"></i><span> Launch simple modal
</button>

Issue: The modal trigger doesn't support buttons that have more than 1 child. In this use case, using the code as currently written, when the user clicks on the heart icon it will not find the data-target class. It will go through the logic chain and end up with clickTarget[parentNode] which in this case would be a span.

The same issue also applies to the dismiss handler as well.

Proposed Solution: Switch the target property on the event object to currentTarget, when setting the click target. Using currentTarget will always fire the event on the item where the event handler was original attached, which when using the data-api will be the element with the data-target.

Reference: https://developer.mozilla.org/en-US/docs/Web/API/Event/currentTarget

thednp commented 4 years ago

Thanks for dropping by.

We're not supporting V3 anymore, if you look into the master, it's V4 only. Can you confirm the issue there btw?

mmottau commented 4 years ago

Sure. It looks like it is. In v4.html i updated line 558 with this mark up.

<span><img src="https://picsum.photos/50" alt=""></span>Launch demo modal

So for context the whole button would look like:

<button type="button" class="btn btn-primary" data-toggle="modal" data-target="#exampleModalLive">
  <span><img src="https://picsum.photos/50" alt=""></span>Launch demo modal
</button>

Then deliberately click on the image and the modal will not trigger.

thednp commented 4 years ago

Yes... the original coding of the clickHandler was designed to check and filter out any case of incorrect [modal <> trigger] link/relation.

Now, please edit that function to this

  function clickHandler(e) {
    if ( modal.isAnimating ) return;

    let clickTarget = e.target;
    clickTarget = clickTarget.hasAttribute('data-target') || clickTarget.hasAttribute('href') ? clickTarget : clickTarget.parentNode;
    if ( (clickTarget === element || element.contains(clickTarget)) && !hasClass(modal,'show') ) { // HERE
      modal.modalTrigger = element;
      relatedTarget = element;
      self.show();
      e.preventDefault();
    }
  }

and let me know how this goes.

UPDATE: I tested this solution and works for me.

mmottau commented 4 years ago

I can confirm this works for me as well.

mmottau commented 4 years ago

Is there any way you could sneak this fix in for v3?

thednp commented 4 years ago

You can do that yourself on a fork you work with.

Thanks for reporting and confirming the fix.