Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(modal): shift+tab without focused element within modal #5294

Closed
wants to merge 1 commit into from
Closed

fix(modal): shift+tab without focused element within modal #5294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
$modalStack.loadFocusElementList(modal);
var focusChanged = false;
if (evt.shiftKey) {
if ($modalStack.isFocusInFirstItem(evt)) {
if ($modalStack.isFocusInFirstItem(evt) || $modalStack.isModalFocused(evt, modal)) {
focusChanged = $modalStack.focusLastFocusableElement();
}
} else {
Expand Down Expand Up @@ -547,6 +547,16 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
return false;
};

$modalStack.isModalFocused = function(evt, modalWindow) {
if (evt && modalWindow) {
var modalDomEl = modalWindow.value.modalDomEl;
if (modalDomEl && modalDomEl.length) {
return (evt.target || evt.srcElement) === modalDomEl[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to check whether it contains the element using contains.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But by using contains, method will return true for each tabbable element within modalWindow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the function is not quite right if that is the case then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about $modalStack.isModalFocused?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable

}
}
return false;
};

$modalStack.isFocusInFirstItem = function(evt) {
if (focusableElementList.length > 0) {
return (evt.target || evt.srcElement) === focusableElementList[0];
Expand Down
12 changes: 10 additions & 2 deletions src/modal/test/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,11 +606,15 @@ describe('$uibModal', function() {
template:'<a href="#" id="tab-focus-link"><input type="text" id="tab-focus-input1"/><input type="text" id="tab-focus-input2"/>' +
'<button id="tab-focus-button">Open me!</button>'
});
$rootScope.$digest();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this extra $digest necessary? This seems a little suspicious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's necessary to update focused element after modal opening

expect($document).toHaveModalsOpen(1);

triggerKeyDown(angular.element(document.activeElement), 9, true);
expect(document.activeElement.getAttribute('id')).toBe('tab-focus-button');

var lastElement = angular.element(document.getElementById('tab-focus-link'));
lastElement.focus();
triggerKeyDown(lastElement, 9, true);
triggerKeyDown(angular.element(document.activeElement), 9, true);
expect(document.activeElement.getAttribute('id')).toBe('tab-focus-button');

initialPage.remove();
Expand Down Expand Up @@ -646,11 +650,15 @@ describe('$uibModal', function() {
'<button id="tab-focus-button">Open me!</button>',
keyboard: false
});
$rootScope.$digest();
expect($document).toHaveModalsOpen(1);

triggerKeyDown(angular.element(document.activeElement), 9, true);
expect(document.activeElement.getAttribute('id')).toBe('tab-focus-button');

var lastElement = angular.element(document.getElementById('tab-focus-link'));
lastElement.focus();
triggerKeyDown(lastElement, 9, true);
triggerKeyDown(angular.element(document.activeElement), 9, true);
expect(document.activeElement.getAttribute('id')).toBe('tab-focus-button');

initialPage.remove();
Expand Down