Skip to content

Commit

Permalink
fix(menu-item): properly compile when used with ng-repeat
Browse files Browse the repository at this point in the history 
* Fixes the menuItem directive not compiling properly when used together with ngRepeat. This is due to the fact that ngRepeat passes around a documentFragment, instead of a DOM node, which means that it doesn't have a parent and doesn't allow us to properly determine whether the menuItem is inside of a menuBar. This change switches the behavior by making the menuBar directive mark it's children instead.
* Minor cleanup in the menuItem directive.
* Avoided some repetition in the menuBar unit tests.
* Switches to using the prefixer in the menuItem directive.

Referencing angular#8117.

Fixes angular#8697.
  • Loading branch information
@crisbeto
crisbeto committed Jun 25, 2016
1 parent 15da974 commit 8ec3fd5
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 55 deletions.
7 changes: 7 additions & 0 deletions src/components/menuBar/js/menuBarDirective.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ function MenuBarDirective($mdUtil, $mdTheming) {
}
});

// Mark the child menu items that they're inside a menu bar. This is necessary,
// because mnMenuItem has special behaviour during compilation, depending on
// whether it is inside a mdMenuBar. We can usually figure this out via the DOM,
// however if a directive that uses documentFragment is applied to the child (e.g. ngRepeat),
// the element won't have a parent and won't compile properly.
templateEl.find('md-menu-item').addClass('md-in-menu-bar');

return function postLink(scope, el, attr, ctrl) {
el.addClass('_md'); // private md component indicator for styling
$mdTheming(scope, el);
Expand Down
32 changes: 17 additions & 15 deletions src/components/menuBar/js/menuItemDirective.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ angular
/* @ngInject */
function MenuItemDirective($mdUtil) {
return {
controller: 'MenuItemController',
require: ['mdMenuItem', '?ngModel'],
priority: 210, // ensure that our post link runs after ngAria
compile: function(templateEl, templateAttrs) {
var type = templateAttrs.type;
var inMenuBarClass = 'md-in-menu-bar';

// Note: This allows us to show the `check` icon for the md-menu-bar items.
if (isInsideMenuBar() && (templateAttrs.type == 'checkbox' || templateAttrs.type == 'radio')) {
// The `md-in-menu-bar` class is set by the mdMenuBar directive.
if (templateEl.hasClass(inMenuBarClass) && (type == 'checkbox' || type == 'radio')) {
var text = templateEl[0].textContent;
var buttonEl = angular.element('<md-button type="button"></md-button>');
buttonEl.html(text);
Expand All @@ -20,10 +24,10 @@ function MenuItemDirective($mdUtil) {
templateEl.html('');
templateEl.append(angular.element('<md-icon md-svg-icon="check"></md-icon>'));
templateEl.append(buttonEl);
templateEl[0].classList.add('md-indent');
templateEl.addClass('md-indent').removeClass(inMenuBarClass);

setDefault('role', (templateAttrs.type == 'checkbox') ? 'menuitemcheckbox' : 'menuitemradio', buttonEl);
angular.forEach(['ng-disabled'], moveAttrToButton);
setDefault('role', type == 'checkbox' ? 'menuitemcheckbox' : 'menuitemradio', buttonEl);
moveAttrToButton('ng-disabled');

} else {
setDefault('role', 'menuitem', templateEl[0].querySelector('md-button, button, a'));
Expand All @@ -46,18 +50,16 @@ function MenuItemDirective($mdUtil) {
}
}

function moveAttrToButton(attr) {
if (templateEl[0].hasAttribute(attr)) {
var val = templateEl[0].getAttribute(attr);
buttonEl[0].setAttribute(attr, val);
templateEl[0].removeAttribute(attr);
}
}
function moveAttrToButton(attribute) {
var attributes = $mdUtil.prefixer(attribute);

function isInsideMenuBar() {
return !!$mdUtil.getClosest(templateEl, 'md-menu-bar', true);
angular.forEach(attributes, function(attr) {
if (templateEl[0].hasAttribute(attr)) {
button[0].setAttribute(attr, val);
templateEl[0].removeAttribute(attr);
}
});
}
},
controller: 'MenuItemController'
}
};
}
79 changes: 39 additions & 40 deletions src/components/menuBar/menu-bar.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ describe('material.components.menuBar', function() {
});

describe('ARIA', function() {

it('sets role="menubar" on the menubar', function() {
var menuBar = setup();
var ariaRole = menuBar[0].getAttribute('role');
expect(ariaRole).toBe('menubar');
});

it('should set the role on the menu trigger correctly', inject(function($compile, $rootScope) {
var el = $compile(
'<md-menu-bar>' +
Expand Down Expand Up @@ -235,7 +235,7 @@ describe('material.components.menuBar', function() {
it('clicks the focused menu', function() {
var opened = false;
spyOn(ctrl, 'getFocusedMenu').and.returnValue({
querySelector: function() { return true }
querySelector: function() { return true; }
});
spyOn(angular, 'element').and.returnValue({
controller: function() { return {
Expand Down Expand Up @@ -317,13 +317,24 @@ describe('material.components.menuBar', function() {

describe('md-menu-item directive', function() {
describe('type="checkbox"', function() {
function setup(attrs) {
return setupMenuItem(attrs + ' type="checkbox"');
}

it('compiles', function() {
var menuItem = setup('ng-model="test"')[0];
expect(menuItem.classList.contains('md-indent')).toBe(true);
var children = menuItem.children;
expect(children[0].nodeName).toBe('MD-ICON');
expect(children[1].nodeName).toBe('MD-BUTTON');
});
it('compiles with ng-repeat', function() {
var menuItem = setup('ng-repeat="i in [1, 2, 3]"')[0];
expect(menuItem.classList.contains('md-indent')).toBe(true);
var children = menuItem.children;
expect(children[0].nodeName).toBe('MD-ICON');
expect(children[1].nodeName).toBe('MD-BUTTON');
});
it('sets aria role', function() {
var menuItem = setup()[0].querySelector('md-button');
expect(menuItem.getAttribute('role')).toBe('menuitemcheckbox');
Expand Down Expand Up @@ -354,36 +365,27 @@ describe('material.components.menuBar', function() {
expect(menuItem.children[0].style.display).toBe('');
expect(button.getAttribute('aria-checked')).toBe('true');
}));
});

describe('type="radio"', function() {
function setup(attrs) {
attrs = attrs || '';

var template = '<md-menu-item type="checkbox" ' + attrs + '>Test Item</md-menu-item>';

var checkboxMenuItem;
inject(function($compile, $rootScope) {
// We need to have a `md-menu-bar` as a parent of our menu item, because the menu-item
// is only wrapping and indenting the content if it's inside of a menu bar.
var menuBarMock = angular.element('<md-menu-bar>');
var itemEl = angular.element(template);

menuBarMock.append(itemEl);
checkboxMenuItem = $compile(itemEl)($rootScope);

$rootScope.$digest();
});
return checkboxMenuItem;
return setupMenuItem(attrs + ' type="radio"');
}
});

describe('type="radio"', function() {
it('compiles', function() {
var menuItem = setup('ng-model="test"')[0];
expect(menuItem.classList.contains('md-indent')).toBe(true);
var children = menuItem.children;
expect(children[0].nodeName).toBe('MD-ICON');
expect(children[1].nodeName).toBe('MD-BUTTON');
});
it('compiles with ng-repeat', function() {
var menuItem = setup('ng-repeat="i in [1, 2, 3]"')[0];
expect(menuItem.classList.contains('md-indent')).toBe(true);
var children = menuItem.children;
expect(children[0].nodeName).toBe('MD-ICON');
expect(children[1].nodeName).toBe('MD-BUTTON');
});
it('sets aria role', function() {
var menuItem = setup()[0].querySelector('md-button');
expect(menuItem.getAttribute('role')).toBe('menuitemradio');
Expand Down Expand Up @@ -417,27 +419,24 @@ describe('material.components.menuBar', function() {
expect(menuItem.children[0].style.display).toBeFalsy();
expect(button.getAttribute('aria-checked')).toBe('true');
}));
});

function setup(attrs) {
attrs = attrs || '';

var template = '<md-menu-item type="radio" ' + attrs + '>Test Item</md-menu-item>';

var radioMenuItem;
inject(function($compile, $rootScope) {
// We need to have a `md-menu-bar` as a parent of our menu item, because the menu-item
// is only wrapping and indenting the content if it's inside of a menu bar.
var menuBarMock = angular.element('<md-menu-bar>');
var itemEl = angular.element(template);
function setupMenuItem(attrs) {
// We need to have a `md-menu-bar` as a parent of our menu item, because the menu-item
// is only wrapping and indenting the content if it's inside of a menu bar.
var menuBar;
var template =
'<md-menu-bar>' +
'<md-menu-item ' + (attrs = attrs || '') + '>Test Item</md-menu-item>' +
'</md-menu-bar>';

menuBarMock.append(itemEl);
radioMenuItem = $compile(itemEl)($rootScope);
inject(function($compile, $rootScope) {
menuBar = $compile(template)($rootScope);
$rootScope.$digest();
});

$rootScope.$digest();
});
return radioMenuItem;
}
});
return menuBar.find('md-menu-item');
}
});

function waitForMenuOpen() {
Expand Down

0 comments on commit 8ec3fd5

Please sign in to comment.