Orphan or last steps freezing on key pressing (keyboard) too fast. #188

Closed
pedroleo opened this Issue Jan 10, 2014 · 13 comments

Projects

None yet

5 participants

@pedroleo

We have found two bugs, although they are probably related to each other since both have to do with using the keyboard to navigate through steps.

1º - If you press the keys associated to "next" and "previous" quickly enough and stop at an orphan step, often both the orphan and next or previous step will be displayed on the screen. Furthermore, if you end tour while both steps are displayed, the next or previous step will stick to the page and keep on displaying while the backdrop closes.
2º - The key associated to "next" on the last step is automatically moved to "end tour", so when you are at the step before the last and press the key associated to "next" twice before the last step shows up, the last step sticks to the screen and will keep on displaying.

The only way to clear your screen from these steps is to either reload the page or go through the tour one more time. Again, this only happens when you navigate with the keyboard, I couldn't reproduce these with the mouse since the "next" and "previous" button disappear and reappear with each step.

image

@pedroleo

This happens too on Bootstrap-Tour website.

Last step:
image

Orphan step:
image

@LostCrew LostCrew was assigned Jan 12, 2014
@LostCrew
Collaborator
LostCrew commented Feb 1, 2014

hello @ccriativo
thank you for the report. the bug is confirmed and i will dig into this as soon as possible.

@LostCrew
Collaborator
LostCrew commented Feb 9, 2014

@sorich87 the bug is: when the user calls next() for the second time, hideStep() cannot remove the popover because it's not been created yet, so $element.data("bs.popover") or $element.data("popover") evaluates as undefined. in order to solve it, you could either:

  • insert a flag indicating whether any transitional operation is ongoing. on second call, the flag is true and the execution is stopped
  • rewrite/refactors all the core methods in a way that the expired popover is removed after the new popover is initialized

i would go for the first. we assume the user needs to actually read the step content, not jump from one to another as fast as possible. thoughts?

@LostCrew LostCrew modified the milestone: 1.0.0, 0.9.0 Feb 9, 2014
@sorich87
Owner
sorich87 commented Feb 9, 2014

Why not cancel the transitional operation and show the next step directly?

@LostCrew
Collaborator
LostCrew commented Feb 9, 2014

because it is already ongoing, unless we use another flag to cancel it.

@sorich87
Owner
sorich87 commented Feb 9, 2014

Ok, go with the solution you prefer.

@sorich87
Owner

@LostCrew, using a queue could also work (http://api.jquery.com/queue/).

@fczbkk
Contributor
fczbkk commented Mar 9, 2014

@LostCrew: we assume the user needs to actually read the step content, not jump from one to another as fast as possible. thoughts?

I have created a fiddle with a real-life example of a Tour that we actually use in our project. It has event hooks, associated with various elements on the page. When that element is interacted with, we display corresponding step from the Tour:

http://jsfiddle.net/fczbkk/Jhfrc/2/

Imagine a Tour for a complex web form. We can take user step by step through each form field. But if the user decides to skip between the fields, we want to keep up the pace. We don't want to force him to click on next/prev every time he presses TAB key.

I aggree that we want users to read the content of the popovers. But I think we should be prepared for rapid navigation between the steps from a technical perspective.

@nmstoker
nmstoker commented Apr 6, 2014

I think @fczbkk put it perfectly in his final sentence.

In my experience there are plenty of people who whiz through things like this, and to an extent I feel "On their own head be it" but it's better if we can protect even 'bad' users from getting an app into a an undesirable state (within limits, clearly!)

I stumbled upon this issue myself pretty much as soon as I started implementing - normally I wouldn't have used it that way (bashing arrow keys...), but when checking something I'd changed part way through a long-ish tour it came up. Took me a little while to rule out I wasn't doing something completely inept on the code side.

Anyway, really love Bootstrap Tour, it's beautifully done.

@fczbkk
Contributor
fczbkk commented Apr 21, 2014

I have tried to solve this problem by removing all pre-existing popovers associated with the tour when new popover is being displayed. The solution is quite simple. But it was pretty tricky to write a test for it.

Could you guys (@sorich87, @LostCrew) do the code review, before we mark this as closed, please? Thanks.

@LostCrew
Collaborator

@fczbkk yes, i'll do it as soon as i can

@sorich87
Owner

It looks good to me.

@fczbkk
Contributor
fczbkk commented Apr 22, 2014

Ok, closing. Looking forward to the next release.

@fczbkk fczbkk closed this Apr 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment