feat(viewer): Add focus trap for fullscreen #1439
Conversation
| } else { | ||
| this.focusLastElement(); | ||
| } | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Is it necessary to stop propagation of the tab keydown event here?
There was a problem hiding this comment.
Seems like if we're controlling the focus because of the tab key we should stop propagation. This is also how it's done in box-ui-elements
| @@ -534,6 +540,28 @@ describe('lib/viewers/BaseViewer', () => { | |||
| expect(base.disableAnnotationControls).toBeCalled(); | |||
| expect(base.processAnnotationModeChange).toBeCalledWith(AnnotationMode.NONE); | |||
| }); | |||
|
|
|||
| test('should enable the focus trap', () => { | |||
| jest.spyOn(FocusTrap.prototype, 'constructor'); | |||
There was a problem hiding this comment.
It's a little unusual to spy on the prototype. Can we instead make sure that base.focusTrap is defined, then use jest.spyOn(base.focusTrap, 'enable')? That way we're testing the instance rather than the prototype.
There was a problem hiding this comment.
Yeah, I don't prefer spying the prototype but I went this route because I wanted to test the case where we actually create the FocusTrap vs reuse it. I went with the approach you suggested below when reusing the existing focus trap
Adds
FocusTrapclass which takes an element and allows you toenableordisablefocus trapping.This class is used for when Preview enters fullscreen to constrain tab navigation to within the visible previewed content. This can also be applied to the print modal.
TODO: