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

Shift + Spacebar should move back to the previous slide #710

Open
maxwell-k opened this issue Dec 30, 2018 · 3 comments
Open

Shift + Spacebar should move back to the previous slide #710

maxwell-k opened this issue Dec 30, 2018 · 3 comments

Comments

@maxwell-k
Copy link

maxwell-k commented Dec 30, 2018

Shift-Space doesn't move back to the previous step on:

Google Chrome	70.0.3538.110 (Official Build) (64-bit)

Shift-Tab works fine and I think that the problem is with the heavily commented logic inside the isNavigationEvent function.

The second if statement returns true for tab only and the third returns false if shift is pressed. event.shiftKey and event.keyCode === 32 // Shift+space is then never handled by the code added in b1e5186 . PR #706 was to make Shift-Space move back a step and close issue #248.

I think this could be a one line change:

-            if ( event.keyCode === 9 ) {
+            if ( event.keyCode === 9 || event.keyCode === 32 ) {

But that would leave difficult to follow comments, for example "sole exception" on the line above or:

        //   as another way to moving to next step... And yes, I know that for the sake of
        //   consistency I should add [shift+tab] as opposite action...

When I think Shift-Tab was added in 0dc8b43.

It might be better to refactor the logic in this function or rewrite the comments. I'd be happy to have a go at a PR for this but I didn't get the test suite running on my first attempt. I use puppeteer for other projects on this setup and merging #637 as it is now should make writing and running tests easier for me.

@henrikingo
Copy link
Member

Confirming and just to be clear, shift+space of course doesn't work on firefox either, as this is purely a logic issue.

I merged #637. Please let me know if you have more issues with tests, I'm very interested in getting that aspect working smoothly now. Note that I recently merged an up to date https://github.com/impress/impress.js/blob/master/test/HOWTO.md. Also, as a first step, you should be able to run all tests by simply opening qunit_test_runner.html in firefox. (Chrome refuses to run it due to íframe security policy.)

@maxwell-k
Copy link
Author

OK good we both see the same thing - a logic bug.

I will try to get the tests suite running somehow, thanks for the HOWTO doc. I normally use a Chromebook so I haven't run Firefox recently. I'll either run Firefox somehow or try and get #637 ready to merge.

@janishutz
Copy link
Contributor

I will be looking into this with V3

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

No branches or pull requests

3 participants