From 756d58327615da27c1a8a4a5a85abd462a8f329a Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 25 Jun 2016 14:56:46 +0200 Subject: [PATCH] fix(menu-item): properly compile when used with ng-repeat * 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. Referencing #8117. Fixes #8697. --- src/components/menuBar/js/menuBarDirective.js | 7 ++ .../menuBar/js/menuItemDirective.js | 21 +++-- src/components/menuBar/menu-bar.spec.js | 79 +++++++++---------- 3 files changed, 56 insertions(+), 51 deletions(-) diff --git a/src/components/menuBar/js/menuBarDirective.js b/src/components/menuBar/js/menuBarDirective.js index 93057ffea24..ae5e2d9f3b9 100644 --- a/src/components/menuBar/js/menuBarDirective.js +++ b/src/components/menuBar/js/menuBarDirective.js @@ -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').attr('md-in-menu-bar', ''); + return function postLink(scope, el, attr, ctrl) { el.addClass('_md'); // private md component indicator for styling $mdTheming(scope, el); diff --git a/src/components/menuBar/js/menuItemDirective.js b/src/components/menuBar/js/menuItemDirective.js index ccc8fc4a7e9..9e5aab1dcef 100644 --- a/src/components/menuBar/js/menuItemDirective.js +++ b/src/components/menuBar/js/menuItemDirective.js @@ -4,14 +4,17 @@ angular .directive('mdMenuItem', MenuItemDirective); /* @ngInject */ -function MenuItemDirective($mdUtil) { +function MenuItemDirective() { return { + controller: 'MenuItemController', require: ['mdMenuItem', '?ngModel'], priority: 210, // ensure that our post link runs after ngAria compile: function(templateEl, templateAttrs) { + var type = templateAttrs.type; // Note: This allows us to show the `check` icon for the md-menu-bar items. - if (isInsideMenuBar() && (templateAttrs.type == 'checkbox' || templateAttrs.type == 'radio')) { + // The mdInMenuBar attribute is set by the mdMenuBar directive. + if (templateAttrs.hasOwnProperty('mdInMenuBar') && (type == 'checkbox' || type == 'radio')) { var text = templateEl[0].textContent; var buttonEl = angular.element(''); buttonEl.html(text); @@ -20,10 +23,11 @@ function MenuItemDirective($mdUtil) { templateEl.html(''); templateEl.append(angular.element('')); templateEl.append(buttonEl); - templateEl[0].classList.add('md-indent'); + templateEl.addClass('md-indent'); + templateAttrs.$set('mdInMenuBar', null); - 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')); @@ -53,11 +57,6 @@ function MenuItemDirective($mdUtil) { templateEl[0].removeAttribute(attr); } } - - function isInsideMenuBar() { - return !!$mdUtil.getClosest(templateEl, 'md-menu-bar', true); - } - }, - controller: 'MenuItemController' + } }; } diff --git a/src/components/menuBar/menu-bar.spec.js b/src/components/menuBar/menu-bar.spec.js index f38ca008045..7f07f84a8a8 100644 --- a/src/components/menuBar/menu-bar.spec.js +++ b/src/components/menuBar/menu-bar.spec.js @@ -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( '' + @@ -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 { @@ -317,6 +317,10 @@ 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); @@ -324,6 +328,13 @@ describe('material.components.menuBar', function() { 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'); @@ -354,29 +365,13 @@ 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 = 'Test 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(''); - 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); @@ -384,6 +379,13 @@ describe('material.components.menuBar', function() { 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'); @@ -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 = 'Test 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(''); - 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 = '' + + '' + + 'Test Item' + + ''; - 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() {