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

Fix clicks on child elements of a tab #253

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

chaitanyagupta
Copy link
Contributor

I am using a bootstrap theme which has a slightly complex heirarchy of elements inside a tab:

<div class="card active show" data-toggle="tab" href="#content-1" role="tab">
  <div class="card-body">
    <h5>headline</h5>
    <p>Some more details</p>
  </div>
</div>

A click on any element inside the div.card-body doesn't work. This PR fixes that by using event.currentTarget instead of event.target.

Hope I have not broken any existing use case! If so, will be happy to fix it.

@thednp
Copy link
Owner

thednp commented Nov 3, 2018

Will have a look at this after the week-end.

@Vuurvlieg
Copy link

@chaitanyagupta thanks for this PR, Looks like its the same 'bug' as in #242?

@chaitanyagupta
Copy link
Contributor Author

@Vuurvlieg yes the issue does look very similar.

@thednp
Copy link
Owner

thednp commented Nov 5, 2018

Hold on folks, just for a second. Yes this PR right here proves to be useful in this case, I just finished some testing with modern and some legacy browsers (I even found some unsolved and unrelated issues there), but this doesn't mean we change all handlers to work this way, as each handler has it's own quirks and whistles.

@thednp
Copy link
Owner

thednp commented Nov 5, 2018

@chaitanyagupta do you have a demo to showcase my code is faulty? I want to see it before anything else.

@chaitanyagupta
Copy link
Contributor Author

@thednp I have three versions of the same page for you. Click on any of the links under the "Features" heading on that page (i.e. Organize, Search, Unread Tasks, etc.)

  1. First, take a look at http://deftask-design.deftask.com.s3-website.us-east-2.amazonaws.com/bootstrap/index.html#features
    This uses bootstrap.js. You will see that switching tabs works just fine.

  2. Next look at http://deftask-design.deftask.com.s3-website.us-east-2.amazonaws.com/bn/index.html#features
    This uses bootstrap-native v4, but without my fixes. You will notice the issue as soon as you try switching tabs. If you look in the console, you will see an error similar to this: Uncaught TypeError: Cannot read property 'classList' of null

  3. Lastly, look at http://deftask-design.deftask.com.s3-website.us-east-2.amazonaws.com/bn-tab-click-fix/index.html#features
    This uses bootstrap-native with my fixes. Now everything works perfectly.

Hope this demonstrates the issue effectively.

FWIW the bootstrap theme I am using is: https://themes.getbootstrap.com/product/wingman-landing-page-app-template/

@chaitanyagupta
Copy link
Contributor Author

Just so you know, there is one more issue that I had to fix before I could get the demo working with this PR. It relates to not setting the aria-selected attribute correctly when switching tabs. However, its a tangential issue so I will file it separately.

@thednp thednp merged commit 36e44f6 into thednp:master Dec 3, 2018
thednp added a commit that referenced this pull request Dec 3, 2018
* V4 fixed missing `aria-selected` from Tab #254
* V3 fixed missing `aria-expanded` from Tab #254
* further updates for #253
* documentation fixes and updates
thednp added a commit that referenced this pull request Dec 3, 2018
* V4 fixed missing `aria-selected` from Tab #254
* V3 fixed missing `aria-expanded` from Tab #254
* further updates for #253
* documentation fixes and updates
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

Successfully merging this pull request may close these issues.

3 participants