Dispose + reinitialize keyboard navigation issues

[sappm01]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

Check that it's possible to dispose the keyboard navigation plugin + reinitialize it on the same page.

From code.org:

Bug in console: Uncaught (in promise) Error: Unhandled Promise Rejection: Shortcut named "edit_or_confirm" collides with shortcuts "commitMove"

Additional comments: This is likely related to dispose() not quite cleaning up all the shortcuts properly yet. We run into it periodically and with different shortcut failures.

https://docs.google.com/document/d/1XlriyUKDc10iNYcgDvlMTlDOXXUqjulZq70KjKgZFHM/edit?tab=t.0

Reproduction steps

Stack trace

Screenshots

No response

Browsers

Chrome desktop, Safari desktop

[maribethb]

I can't reproduce this locally. We uninstall both the move shortcuts and the edit_or_confirm shortcut on dispose and we install them in the correct order in init (edit or confirm must be registered before move). So I don't have any problems with these particular shortcuts. @hannah is it possible that cdo is registering these shortcuts in a different order?

that being said, we can make this a bit more resilient by letting the edit shortcut also allowCollisions so that it doesn't necessarily have to be registered before move, and then adding a check that move mode isn't active. because currently if you did successfully register the edit shortcut after the move mode shortcut, I think it would break as the edit one would be run first, since shortcuts are tried in the order of most-recently-registered to last.

I also found one area related to clipboard actions we can clean up. otherwise I don't have any problems with dispose and then re-initializing.

[hannahbergam]

@maribethb We aren't registering either shortcut manually. It looks like the shortcut for commitMove is added dynamically when you initialize a move, and then somehow it isn't cleaned up when we load a new workspace. I see that it is unregistered dynamically in a few cases after, but would it be possible to also unregister on dispose?

[maribethb]

Thank you! I was looking at the "finishMove" instead of the "commitMove" shortcut, my bad.

[maribethb]

@hannahbergam I've put up #687, any chance you can test with that? I was never able to reproduce the specific error you were getting about "commitMove" but I did add some code unregistering it on dispose if for some reason it was still registered. I did catch some other problems and fixed them, and wrote some tests to check that what you're doing should be possible.

You can clean up your code here because you shouldn't need to unregister/reregister any of undo, redo, copy, paste, or cut anymore. and I don't think you need the patchShortcuts method at all anymore, but if you do, you can just directly copy the keyCodes property as the related bugs have all been fixed as well. let me know if any of that isn't true please :)