Skip to content

Commit

Permalink
fix(spatial-navigation): focus lost in error modal (#8817)
Browse files Browse the repository at this point in the history
## Description
The spatial-navigation is unable to focus certain elements of the error
modal when this appears, this PR will fix that

## Specific Changes proposed
Allow the spatial-navigation to focus certain non-component elements in
the error modal

## Requirements Checklist
- [x] Feature implemented / Bug fixed
- [ ] If necessary, more likely in a feature request than a bug fix
- [ ] Change has been verified in an actual browser (Chrome, Firefox,
IE)
  - [ ] Unit Tests updated or fixed
  - [ ] Docs/guides updated
- [ ] Example created ([starter template on
JSBin](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0))
- [ ] Has no DOM changes which impact accessiblilty or trigger warnings
(e.g. Chrome issues tab)
  - [ ] Has no changes to JSDoc which cause `npm run docs:api` to error
- [ ] Reviewed by Two Core Contributors
  • Loading branch information
CarlosVillasenor authored Jul 29, 2024
1 parent 76e99b7 commit daf40bd
Show file tree
Hide file tree
Showing 3 changed files with 183 additions and 4 deletions.
8 changes: 8 additions & 0 deletions src/js/modal-dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,14 @@ class ModalDialog extends Component {
if (closeButton) {
parentEl.appendChild(closeButton.el_);
}

/**
* Fired after `ModalDialog` is re-filled with content & close button is appended.
*
* @event ModalDialog#aftermodalfill
* @type {Event}
*/
this.trigger('aftermodalfill');
}

/**
Expand Down
67 changes: 64 additions & 3 deletions src/js/spatial-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,17 @@ class SpatialNavigation extends EventTarget {
this.player_.on('modalclose', () => {
this.refocusComponent();
});
this.player_.on('error', () => {
this.focus(this.updateFocusableComponents()[0]);
});
this.player_.on('focusin', this.handlePlayerFocus_.bind(this));
this.player_.on('focusout', this.handlePlayerBlur_.bind(this));
this.isListening_ = true;
this.player_.errorDisplay.on('aftermodalfill', () => {
this.updateFocusableComponents();

// Focus the buttons of the modal
if (this.focusableComponents.length > 1) {
this.focusableComponents[1].focus();
}
});
}

/**
Expand Down Expand Up @@ -270,9 +275,65 @@ class SpatialNavigation extends EventTarget {
focusableComponents.push(value);
}
}

// TODO - Refactor the following logic after refactor of videojs-errors elements to be components is done.
if (value.name_ === 'ErrorDisplay' && value.opened_) {
const buttonContainer = value.el_.querySelector('.vjs-errors-ok-button-container');

if (buttonContainer) {
const modalButtons = buttonContainer.querySelectorAll('button');

modalButtons.forEach((element, index) => {
// Add elements as objects to be handled by the spatial navigation
focusableComponents.push({
name: () => {
return 'ModalButton' + (index + 1);
},
el: () => element,
getPositions: () => {
const rect = element.getBoundingClientRect();

// Creating objects that mirror DOMRectReadOnly for boundingClientRect and center
const boundingClientRect = {
x: rect.x,
y: rect.y,
width: rect.width,
height: rect.height,
top: rect.top,
right: rect.right,
bottom: rect.bottom,
left: rect.left
};

// Calculating the center position
const center = {
x: rect.left + rect.width / 2,
y: rect.top + rect.height / 2,
width: 0,
height: 0,
top: rect.top + rect.height / 2,
right: rect.left + rect.width / 2,
bottom: rect.top + rect.height / 2,
left: rect.left + rect.width / 2
};

return {
boundingClientRect,
center
};
},
// Asume that the following are always focusable
getIsAvailableToBeFocused: () => true,
getIsFocusable: (el) => true,
focus: () => element.focus()
});
});
}
}
});

this.focusableComponents = focusableComponents;

return this.focusableComponents;
}

Expand Down
112 changes: 111 additions & 1 deletion test/unit/spatial-navigation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import TestHelpers from './test-helpers.js';
import sinon from 'sinon';
import document from 'global/document';
import TextTrackSelect from '../../src/js/tracks/text-track-select';
import * as Dom from '../../src/js/utils/dom.js';

QUnit.module('SpatialNavigation', {
beforeEach() {
Expand Down Expand Up @@ -44,7 +45,6 @@ QUnit.test('start method initializes event listeners', function(assert) {
assert.ok(onSpy.calledWith('loadedmetadata'), 'loadedmetadata event listener added');
assert.ok(onSpy.calledWith('modalKeydown'), 'modalKeydown event listener added');
assert.ok(onSpy.calledWith('modalclose'), 'modalclose event listener added');
assert.ok(onSpy.calledWith('error'), 'error event listener added');

// Additionally, check if isListening_ flag is set
assert.ok(this.spatialNav.isListening_, 'isListening_ flag is set');
Expand Down Expand Up @@ -502,3 +502,113 @@ QUnit.test('error on player calls updateFocusableComponents', function(assert) {

assert.ok(updateFocusableComponentsSpy.calledOnce, 'on error event spatial navigation should call "updateFocusableComponents"');
});

QUnit.test('error on player focus the second focusable element of error modal', function(assert) {
this.spatialNav.start();

const firstComponent = {
name: () => 'firstComponent',
el: () => document.createElement('div'),
focus: sinon.spy(),
getIsAvailableToBeFocused: () => true
};

const secondComponent = {
name: () => 'secondComponent',
el: () => document.createElement('div'),
focus: sinon.spy(),
getIsAvailableToBeFocused: () => true
};

this.spatialNav.focusableComponents = [firstComponent, secondComponent];
this.spatialNav.getCurrentComponent = () => firstComponent;
this.spatialNav.updateFocusableComponents = () => [firstComponent, secondComponent];

this.player.error({
code: 1,
dismiss: true
});

assert.ok(secondComponent.focus.calledOnce, 'Focus should move to the second component');
assert.notOk(firstComponent.focus.called, 'Focus should not remain on the first component');
});

QUnit.test('on error, modalButtons should get the buttons if those are available', function(assert) {
this.spatialNav.start();

const buttonContainer = Dom.createEl('div', {}, {class: 'vjs-errors-ok-button-container'});

const testEl1 = Dom.createEl('button', {}, {class: 'c1'});

// Add first element to error modal
buttonContainer.appendChild(testEl1);

const testEl2 = Dom.createEl('button', {}, {class: 'c1'});

// Add second element to error modal
buttonContainer.appendChild(testEl2);
this.player.errorDisplay.el().appendChild(buttonContainer);

this.player.error({
code: 1,
dismiss: true
});

this.spatialNav.getCurrentComponent = () => this.spatialNav.focusableComponents[0];

const getPositionsEl1Spy = sinon.spy(this.spatialNav.focusableComponents[0], 'getPositions');
const getPositionsEl2Spy = sinon.spy(this.spatialNav.focusableComponents[1], 'getPositions');

this.spatialNav.move('left');

assert.strictEqual(this.spatialNav.focusableComponents.length, 2, 'button elements are now part of the array of focusableComponents');
assert.ok(getPositionsEl1Spy.calledOnce, 'getPositions method called on button');
assert.ok(getPositionsEl2Spy.calledOnce, 'getPositions method called on button');
assert.strictEqual(this.spatialNav.focusableComponents[0].name(), 'ModalButton1', 'testEl1 name should be ModalButton1');
assert.strictEqual(this.spatialNav.focusableComponents[1].name(), 'ModalButton2', 'testEl2 name should be ModalButton2');

getPositionsEl1Spy.restore();
getPositionsEl2Spy.restore();
});

QUnit.test('on error, modalButtons added functions should work properly', function(assert) {
this.spatialNav.start();

const buttonContainer = Dom.createEl('div', {}, {class: 'vjs-errors-ok-button-container'});

const testEl1 = Dom.createEl('button', {}, {class: 'c1'});

// Add first element to error modal
buttonContainer.appendChild(testEl1);

this.player.errorDisplay.el().appendChild(buttonContainer);

this.player.error({ code: 1, dismiss: true });

assert.strictEqual(this.spatialNav.focusableComponents[0].el() instanceof Element, true, 'el function from modal buttons should return a DOM element'); // eslint-disable-line no-undef
assert.strictEqual(this.spatialNav.focusableComponents[0].getIsFocusable(), true, 'getIsFocusable function from modal buttons is always true');
assert.strictEqual(this.spatialNav.focusableComponents[0].getIsAvailableToBeFocused(), true, 'getIsAvailableToBeFocused function from modal buttons is always true');
assert.strictEqual(this.spatialNav.focusableComponents[0].getIsAvailableToBeFocused(), true, 'getIsAvailableToBeFocused function from modal buttons is always true');
assert.strictEqual(typeof this.spatialNav.focusableComponents[0].getPositions(), 'object', 'focusableComponents function from modal buttons should return an object');
});

QUnit.test('If component passes the required functions it should be added to focusableComponents', function(assert) {
this.spatialNav.start();

const firstComponent = {
name_: 'firstComponent',
name: () => this.name,
el_: document.createElement('div'),
el: () => this.el_,
focus: sinon.spy(),
getIsAvailableToBeFocused: () => true,
getIsFocusable: () => true
};

this.player.children_.push(firstComponent);
this.spatialNav.getCurrentComponent = () => firstComponent;
this.spatialNav.updateFocusableComponents();

assert.strictEqual(this.spatialNav.focusableComponents.length, 1, 'focusableComponents array should have 1 component');
assert.strictEqual(this.spatialNav.focusableComponents[0].name_, 'firstComponent', 'the name of the component in focusableComponents array should be "firstComponent"');
});

0 comments on commit daf40bd

Please sign in to comment.