Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Many Select / Menu Fixes #6151

Closed
wants to merge 5 commits into from
Closed

Many Select / Menu Fixes #6151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
089e33b
fix(menubar): fix menus overflow hiding behind menubar
rschmukler Dec 8, 2015
d034e14
fix(menubar): fix keyboard controls
rschmukler Dec 8, 2015
2ea692b
amend(select): keep select in DOM but move to body on open
rschmukler Dec 7, 2015
cd75b22
fix(menu): fix menus inside toolbars and dialogs
rschmukler Dec 8, 2015
9fe3a57
fix(menubar): fix broken menubar accessability with checkbox and radi…
rschmukler Dec 8, 2015
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
8 changes: 5 additions & 3 deletions src/components/menu/js/menuController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ function MenuController($mdMenu, $attrs, $element, $scope, $mdUtil, $timeout, $r
menuContainer = setMenuContainer;
// Default element for ARIA attributes has the ngClick or ngMouseenter expression
triggerElement = $element[0].querySelector('[ng-click],[ng-mouseenter]');
triggerElement.setAttribute('aria-expanded', 'false');

this.isInMenuBar = opts.isInMenuBar;
this.nestedMenus = $mdUtil.nodesToArray(menuContainer[0].querySelectorAll('.md-nested-menu'));

menuContainer.on('$mdInterimElementRemove', function() {
Expand Down Expand Up @@ -109,6 +111,7 @@ function MenuController($mdMenu, $attrs, $element, $scope, $mdUtil, $timeout, $r
self.enableHoverListener();
self.isOpen = true;
triggerElement = triggerElement || (ev ? ev.target : $element[0]);
triggerElement.setAttribute('aria-expanded', 'true');
$scope.$emit('$mdMenuOpen', $element);
$mdMenu.show({
scope: $scope,
Expand All @@ -117,8 +120,9 @@ function MenuController($mdMenu, $attrs, $element, $scope, $mdUtil, $timeout, $r
element: menuContainer,
target: triggerElement,
preserveElement: true,
parent: $element
parent: 'body'
}).finally(function() {
triggerElement.setAttribute('aria-expanded', 'false');
self.disableHoverListener();
});
};
Expand All @@ -128,14 +132,12 @@ function MenuController($mdMenu, $attrs, $element, $scope, $mdUtil, $timeout, $r

$scope.$watch(function() { return self.isOpen; }, function(isOpen) {
if (isOpen) {
triggerElement.setAttribute('aria-expanded', 'true');
menuContainer.attr('aria-hidden', 'false');
$element[0].classList.add('md-open');
angular.forEach(self.nestedMenus, function(el) {
el.classList.remove('md-open');
});
} else {
triggerElement && triggerElement.setAttribute('aria-expanded', 'false');
menuContainer.attr('aria-hidden', 'true');
$element[0].classList.remove('md-open');
}
Expand Down
4 changes: 4 additions & 0 deletions src/components/menu/js/menuDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ function MenuDirective($mdUtil) {
}
menuContainer.append(menuContents);

element.on('$destroy', function() {
menuContainer.remove();
});

element.append(menuContainer);
menuContainer[0].style.display = 'none';
mdMenuCtrl.init(menuContainer, { isInMenuBar: isInMenuBar });
Expand Down
13 changes: 5 additions & 8 deletions src/components/menu/js/menuServiceProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,8 @@ function MenuProvider($$interimElementProvider) {
* Place the menu into the DOM and call positioning related functions
*/
function showMenu() {
if (!opts.preserveElement) {
opts.parent.append(element);
} else {
element[0].style.display = '';
}
opts.parent.append(element);
element[0].style.display = '';

return $q(function(resolve) {
var position = calculateMenuPosition(element, opts);
Expand Down Expand Up @@ -241,13 +238,13 @@ function MenuProvider($$interimElementProvider) {
handled = true;
break;
case $mdConstant.KEY_CODE.UP_ARROW:
if (!focusMenuItem(ev, opts.menuContentEl, opts, -1)) {
if (!focusMenuItem(ev, opts.menuContentEl, opts, -1) && !opts.nestLevel) {
opts.mdMenuCtrl.triggerContainerProxy(ev);
}
handled = true;
break;
case $mdConstant.KEY_CODE.DOWN_ARROW:
if (!focusMenuItem(ev, opts.menuContentEl, opts, 1)) {
if (!focusMenuItem(ev, opts.menuContentEl, opts, 1) && !opts.nestLevel) {
opts.mdMenuCtrl.triggerContainerProxy(ev);
}
handled = true;
Expand Down Expand Up @@ -294,7 +291,7 @@ function MenuProvider($$interimElementProvider) {
if ((hasAnyAttribute(target, ['ng-click', 'ng-href', 'ui-sref']) ||
target.nodeName == 'BUTTON' || target.nodeName == 'MD-BUTTON') && !hasAnyAttribute(target, ['md-prevent-menu-close'])) {
var closestMenu = $mdUtil.getClosest(target, 'MD-MENU');
if (!target.hasAttribute('disabled') && (closestMenu == opts.parent[0])) {
if (!target.hasAttribute('disabled') && (!closestMenu || closestMenu == opts.parent[0])) {
close();
}
break;
Expand Down
44 changes: 31 additions & 13 deletions src/components/menu/menu.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe('material.components.menu', function() {
$mdUtil = _$mdUtil_;
$mdMenu = _$mdMenu_;
$timeout = _$timeout_;
var abandonedMenus = $document[0].querySelectorAll('.md-menu-container');
var abandonedMenus = $document[0].querySelectorAll('.md-open-menu-container');
angular.element(abandonedMenus).remove();
}));
afterEach(function() {
Expand Down Expand Up @@ -41,6 +41,23 @@ describe('material.components.menu', function() {
expect(getOpenMenuContainer(menu).length).toBe(0);
});

it('cleans up an open menu when the element leaves the DOM', function() {
var menu = setup();
openMenu(menu);
menu.remove();
expect(getOpenMenuContainer(menu).length).toBe(0);
});

it('sets up proper aria-owns and aria-haspopup relations between the button and the container', function() {
var menu = setup();
var button = menu[0].querySelector('button');
expect(button.hasAttribute('aria-haspopup')).toBe(true);
expect(button.hasAttribute('aria-owns')).toBe(true);
var ownsId = button.getAttribute('aria-owns');
openMenu(menu);
expect(getOpenMenuContainer(menu).attr('id')).toBe(ownsId);
});

it('opens on click without $event', function() {
var noEvent = true;
var menu = setup('ng-click', noEvent);
Expand Down Expand Up @@ -98,7 +115,7 @@ describe('material.components.menu', function() {
openMenu(menu);
expect(getOpenMenuContainer(menu).length).toBe(1);

var openMenuEl = menu[0].querySelector('md-menu-content');
var openMenuEl = $document[0].querySelector('md-menu-content');

pressKey(openMenuEl, $mdConstant.KEY_CODE.ESCAPE);
waitForMenuClose();
Expand Down Expand Up @@ -197,21 +214,22 @@ describe('material.components.menu', function() {
// ********************************************

function getOpenMenuContainer(el) {
var res;
el = (el instanceof angular.element) ? el[0] : el;
var container = el.querySelector('.md-open-menu-container');
if (container.style.display == 'none') {
return angular.element([]);
} else {
return angular.element(container);
}
inject(function($document) {
var container = $document[0].querySelector('.md-open-menu-container');
if (container && container.style.display == 'none') {
res = [];
} else {
res = angular.element(container);
}
});
return res;
}

function openMenu(el, triggerType) {
inject(function($document) {
el.children().eq(0).triggerHandler(triggerType || 'click');
$document[0].body.appendChild(el[0]);
waitForMenuOpen();
});
el.children().eq(0).triggerHandler(triggerType || 'click');
waitForMenuOpen();
}

function closeMenu() {
Expand Down
22 changes: 10 additions & 12 deletions src/components/menuBar/js/menuBarController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,16 @@ MenuBarController.prototype.init = function() {
el[0].classList.remove('md-open');
}

if (opts.closeAll) {
if ($element[0].contains(el[0])) {
var parentMenu = el[0];
while (parentMenu && rootMenus.indexOf(parentMenu) == -1) {
parentMenu = $mdUtil.getClosest(parentMenu, 'MD-MENU', true);
}
if (parentMenu) {
if (!opts.skipFocus) parentMenu.querySelector('button:not([disabled])').focus();
self.currentlyOpenMenu = undefined;
self.disableOpenOnHover();
self.setKeyboardMode(true);
}
if ($element[0].contains(el[0])) {
var parentMenu = el[0];
while (parentMenu && rootMenus.indexOf(parentMenu) == -1) {
parentMenu = $mdUtil.getClosest(parentMenu, 'MD-MENU', true);
}
if (parentMenu) {
if (!opts.skipFocus) parentMenu.querySelector('button:not([disabled])').focus();
self.currentlyOpenMenu = undefined;
self.disableOpenOnHover();
self.setKeyboardMode(true);
}
}
}));
Expand Down
21 changes: 17 additions & 4 deletions src/components/menuBar/js/menuItemController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,23 @@ MenuItemController.prototype.init = function(ngModel) {
this.mode = $attrs.type;
this.iconEl = $element[0].children[0];
this.buttonEl = $element[0].children[1];
if (ngModel) this.initClickListeners();
if (ngModel) {
// Clear ngAria set attributes
this.initClickListeners();
}
}
};

MenuItemController.prototype.clearNgAria = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comment here explaining what ngAria does and why it's a problem. (which is my understanding of what's happening here)

var el = this.$element[0];
var clearAttrs = ['role', 'tabindex', 'aria-invalid', 'aria-checked'];
angular.forEach(clearAttrs, function(attr) {
el.removeAttribute(attr);
});
};

MenuItemController.prototype.initClickListeners = function() {
var self = this;
var ngModel = this.ngModel;
var $scope = this.$scope;
var $attrs = this.$attrs;
Expand All @@ -35,20 +47,21 @@ MenuItemController.prototype.initClickListeners = function() {

this.handleClick = angular.bind(this, this.handleClick);

var icon = this.iconEl
var icon = this.iconEl;
var button = angular.element(this.buttonEl);
var handleClick = this.handleClick;

$attrs.$observe('disabled', setDisabled);
setDisabled($attrs.disabled);

ngModel.$render = function render() {
self.clearNgAria();
if (isSelected()) {
icon.style.display = '';
$element.attr('aria-checked', 'true');
button.attr('aria-checked', 'true');
} else {
icon.style.display = 'none';
$element.attr('aria-checked', 'false');
button.attr('aria-checked', 'false');
}
};

Expand Down
1 change: 1 addition & 0 deletions src/components/menuBar/js/menuItemDirective.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ angular
function MenuItemDirective() {
return {
require: ['mdMenuItem', '?ngModel'],
priority: 210,
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comment explaining the non-default priority

compile: function(templateEl, templateAttrs) {
if (templateAttrs.type == 'checkbox' || templateAttrs.type == 'radio') {
var text = templateEl[0].textContent;
Expand Down
10 changes: 6 additions & 4 deletions src/components/menuBar/menu-bar.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,13 @@ describe('material.components.menuBar', function() {
});
it('reflects the ng-model value', inject(function($rootScope) {
var menuItem = setup('ng-model="test"')[0];
expect(menuItem.getAttribute('aria-checked')).toBe('false');
var button = menuItem.querySelector('md-button');
expect(button.getAttribute('aria-checked')).toBe('false');
expect(menuItem.children[0].style.display).toBe('none');
$rootScope.test = true;
$rootScope.$digest();
expect(menuItem.children[0].style.display).toBe('');
expect(menuItem.getAttribute('aria-checked')).toBe('true');
expect(button.getAttribute('aria-checked')).toBe('true');
}));

function setup(attrs) {
Expand Down Expand Up @@ -283,12 +284,13 @@ describe('material.components.menuBar', function() {
it('reflects the ng-model value', inject(function($rootScope) {
$rootScope.test = 'apple';
var menuItem = setup('ng-model="test" value="hello"')[0];
expect(menuItem.getAttribute('aria-checked')).toBe('false');
var button = menuItem.querySelector('md-button');
expect(button.getAttribute('aria-checked')).toBe('false');
expect(menuItem.children[0].style.display).toBe('none');
$rootScope.test = 'hello';
$rootScope.$digest();
expect(menuItem.children[0].style.display).toBeFalsy();
expect(menuItem.getAttribute('aria-checked')).toBe('true');
expect(button.getAttribute('aria-checked')).toBe('true');
}));

function setup(attrs) {
Expand Down
18 changes: 13 additions & 5 deletions src/components/select/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
inputCheckValue();
};


attr.$observe('placeholder', ngModelCtrl.$render);


Expand Down Expand Up @@ -386,6 +387,10 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
if (!element[0].hasAttribute('id')) {
ariaAttrs.id = 'select_' + $mdUtil.nextUid();
}

var containerId = 'select_container_' + $mdUtil.nextUid();
selectContainer.attr('id', containerId);
ariaAttrs['aria-owns'] = containerId;
element.attr(ariaAttrs);

scope.$on('$destroy', function() {
Expand Down Expand Up @@ -419,6 +424,9 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
}
selectMenuCtrl = selectContainer.find('md-select-menu').controller('mdSelectMenu');
selectMenuCtrl.init(ngModelCtrl, attr.ngModel);
element.on('$destroy', function() {
selectContainer.remove();
});
}

function handleKeypress(e) {
Expand All @@ -442,7 +450,7 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
}
}

function openSelect(e) {
function openSelect() {
selectScope.isOpen = true;
element.attr('aria-expanded', 'true');

Expand All @@ -452,8 +460,8 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
skipCompile: true,
element: selectContainer,
target: element[0],
selectCtrl: mdSelectCtrl,
preserveElement: true,
parent: element,
hasBackdrop: true,
loadingAsync: attr.mdOnOpen ? scope.$eval(attr.mdOnOpen) || true : false
}).finally(function() {
Expand Down Expand Up @@ -1247,7 +1255,7 @@ function SelectProvider($$interimElementProvider) {
* trigger the [optional] user-defined expression
*/
function announceClosed(opts) {
var mdSelect = opts.selectEl.controller('mdSelect');
var mdSelect = opts.selectCtrl;
if (mdSelect) {
var menuController = opts.selectEl.controller('mdSelectMenu');
mdSelect.setLabelText(menuController.selectedLabels());
Expand All @@ -1262,7 +1270,7 @@ function SelectProvider($$interimElementProvider) {
function calculateMenuPositions(scope, element, opts) {
var
containerNode = element[0],
targetNode = opts.target[0].children[1], // target the label
targetNode = opts.target[0].children[0], // target the label
parentNode = $document[0].body,
selectNode = opts.selectEl[0],
contentNode = opts.contentEl[0],
Expand Down Expand Up @@ -1370,7 +1378,7 @@ function SelectProvider($$interimElementProvider) {
} else {
left = (targetRect.left + centeredRect.left - centeredRect.paddingLeft) + 2;
top = Math.floor(targetRect.top + targetRect.height / 2 - centeredRect.height / 2 -
centeredRect.top + contentNode.scrollTop) + 5;
centeredRect.top + contentNode.scrollTop) + 4;

transformOrigin = (centeredRect.left + targetRect.width / 2) + 'px ' +
(centeredRect.top + centeredRect.height / 2 - contentNode.scrollTop) + 'px 0px';
Expand Down
Loading