IGreatlyDislikeJavascript / bootstrap-tourist

Quick and easy way to build your product tours with Bootstrap Popovers for Bootstrap 3 and 4. Based on Bootstrap-Tour, but with many fixes and features added.
63 stars 36 forks source link

On the last step in the tour the next button is disabled, but can still be clicked #54

Open DancingDad opened 4 years ago

DancingDad commented 4 years ago

"bootstrap-tourist": "^0.3.2",

Error: Noticed that on the last step in the tour the next button is greyed out but can still be clicked resulting in the user being locked in a backdrop they cannot navigate from without refreshing the page.

Solution: The next button should be disabled and unclickable on the last step of the tour. The only clickable options should be "Prev" and "End Tour".

DancingDad commented 4 years ago

Think I found the issue.

Root Cause: The data-role="next" is still in place, even though the button is visibly disabled. Due to the data-role being in place a click on the button still performs an action.

Fix: In addition to adding the disabled class, added a line of code to remove the data-role attribute. This prevents the button from performing any action on click.

Code, recommended change shown with comment.

if (step.next < 0) {
    $next.addClass('disabled').prop('disabled', true).prop('tabindex', -1);
    $next.removeAttr('data-role');  // Added this line of code.
}

Testing: After making the change locally: All worked as expected:

Leaving this issue open as I have not done a PR for this. Would classify myself as newbie at JS and some of this code looks a bit scary. :)

IGreatlyDislikeJavascript commented 4 years ago

Thanks for finding and fixing this. I'm hoping to be in a position to start updating code again shortly, a full lockdown still exists in my country.

Due to the data-role being in place a click on the button still performs an action.

That's odd! I need to look into this, my expectation was that jq click event (and handler) wouldn't fire on a disabled element. Looks like you've tested your fix comprehensively too, thank you.

Would classify myself as newbie at JS and some of this code looks a bit scary. :)

Haha yes, you and me both. As I've said all along, I'm not a js coder! Some of the scary code is because the original developers wrote Tour in coffee script, which I've never heard of and since found out does some weird transpilation stuff, with no comments.

Thanks again.

vladrusu commented 4 years ago

The bug manifests itself on both last step, but on first step also (clicking on Previous on the first step closes the tour but leaves the backdrop/highlighted element visible!).

Elaborating on @DancingDad's solution, I came with this code:

if (step.prev <= 0) {
    $prev.addClass('disabled').prop('disabled', true).prop('tabindex', -1);
    $prev.removeAttr('data-role');
}
if (step.next < 0) {
    $next.addClass('disabled').prop('disabled', true).prop('tabindex', -1);
    $next.removeAttr('data-role');
}
igor-stojcic commented 4 years ago

Regards to all. This is a great correction, I just think that in the first if statement step.prev <= 0 should only be step.prev < 0 . Correct me if I'm wrong. Thanks.

AlexOulapourHCTX commented 2 years ago

Will killbobjr's fix for this be merged into master?