From 3f23fd91666cf3bf25d5f90e0faf430b70fd883e Mon Sep 17 00:00:00 2001 From: Eirik Blakstad Date: Fri, 16 Nov 2018 14:01:32 +0100 Subject: [PATCH 1/3] fix(aria/ngClick): check if element is `contenteditable` before blocking spacebar `ngAria`'s `ngClick` blocks spacebar keypresses on non-blacklisted elements, which is an issue when the element is `contenteditable`. --- src/ngAria/aria.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngAria/aria.js b/src/ngAria/aria.js index cc7e481c5588..e3bfebd1c8b6 100644 --- a/src/ngAria/aria.js +++ b/src/ngAria/aria.js @@ -390,7 +390,7 @@ ngAriaModule.directive('ngShow', ['$aria', function($aria) { if (keyCode === 13 || keyCode === 32) { // If the event is triggered on a non-interactive element ... - if (nodeBlackList.indexOf(event.target.nodeName) === -1) { + if (nodeBlackList.indexOf(event.target.nodeName) === -1 && !event.target.isContentEditable) { // ... prevent the default browser behavior (e.g. scrolling when pressing spacebar) // See https://github.com/angular/angular.js/issues/16664 event.preventDefault(); From 628bd597e144f42f995c83f7c73eb94b669f1f47 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 26 Nov 2018 15:16:27 +0200 Subject: [PATCH 2/3] fixup! fix(aria/ngClick): check if element is `contenteditable` before blocking spacebar --- test/ngAria/ariaSpec.js | 73 ++++++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 20 deletions(-) diff --git a/test/ngAria/ariaSpec.js b/test/ngAria/ariaSpec.js index e36fb9cc2778..76dee0906675 100644 --- a/test/ngAria/ariaSpec.js +++ b/test/ngAria/ariaSpec.js @@ -954,6 +954,59 @@ describe('$aria', function() { expect(clickEvents).toEqual(['div(true)', 'li(true)', 'div(true)', 'li(true)']); }); + it('should trigger a click in browsers that provide `event.which` instead of `event.keyCode`', + function() { + compileElement( + '
' + + '
' + + '
' + + '
'); + + var divElement = element.find('div'); + var liElement = element.find('li'); + + divElement.triggerHandler({type: 'keydown', which: 13}); + liElement.triggerHandler({type: 'keydown', which: 13}); + divElement.triggerHandler({type: 'keydown', which: 32}); + liElement.triggerHandler({type: 'keydown', which: 32}); + + expect(clickEvents).toEqual(['div(true)', 'li(true)', 'div(true)', 'li(true)']); + } + ); + + it('should not prevent default keyboard action if the target element has editable content', + function() { + compileElement( + '
' + + '
' + + '
' + + '
'); + + var divElement = element.find('div'); + var ulElement = element.find('ul'); + var liElement = element.find('li'); + + // Using `browserTrigger()`, because it supports event bubbling. + + // Element are not editable yet. + browserTrigger(divElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); + browserTrigger(ulElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); + browserTrigger(liElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); + + expect(clickEvents).toEqual(['div(true)', 'ul(true)', 'li(true)']); + + clickEvents = []; + scope.$apply('edit = true'); + + // Element are now editable. + browserTrigger(divElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); + browserTrigger(ulElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); + browserTrigger(liElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); + + expect(clickEvents).toEqual(['div(false)', 'ul(true)', 'li(false)']); + } + ); + they('should not prevent default keyboard action if an interactive $type element' + 'is nested inside ng-click', nodeBlackList, function(elementType) { function createHTML(type) { @@ -981,26 +1034,6 @@ describe('$aria', function() { } ); - it('should trigger a click in browsers that provide `event.which` instead of `event.keyCode`', - function() { - compileElement( - '
' + - '
' + - '
' + - '
'); - - var divElement = element.find('div'); - var liElement = element.find('li'); - - divElement.triggerHandler({type: 'keydown', which: 13}); - liElement.triggerHandler({type: 'keydown', which: 13}); - divElement.triggerHandler({type: 'keydown', which: 32}); - liElement.triggerHandler({type: 'keydown', which: 32}); - - expect(clickEvents).toEqual(['div(true)', 'li(true)', 'div(true)', 'li(true)']); - } - ); - they('should not bind to key events if there is existing `ng-$prop`', ['keydown', 'keypress', 'keyup'], function(eventName) { scope.onKeyEvent = jasmine.createSpy('onKeyEvent'); From 2ed73c8f8f00e636243776f769403f15868826e1 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 28 Nov 2018 18:24:43 +0200 Subject: [PATCH 3/3] fixup! fix(aria/ngClick): check if element is `contenteditable` before blocking spacebar --- test/ngAria/ariaSpec.js | 88 +++++++++++++++++++++++++++++++++-------- 1 file changed, 72 insertions(+), 16 deletions(-) diff --git a/test/ngAria/ariaSpec.js b/test/ngAria/ariaSpec.js index 76dee0906675..2f96cb2f0a0a 100644 --- a/test/ngAria/ariaSpec.js +++ b/test/ngAria/ariaSpec.js @@ -975,36 +975,92 @@ describe('$aria', function() { ); it('should not prevent default keyboard action if the target element has editable content', - function() { + inject(function($document) { + // Note: + // `contenteditable` is an enumarated (not a boolean) attribute (see + // https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable). + // We need to check the following conditions: + // - No attribute. + // - Value: "" + // - Value: "true" + // - Value: "false" + + function eventFor(keyCode) { + return {bubbles: true, cancelable: true, keyCode: keyCode}; + } + compileElement( '
' + - '
' + - '
' + + // No attribute. + '
' + + '
' + + '
' + + '
' + + + // Value: "" + '
' + + '
' + + '
' + + '
' + + + // Value: "true" + '
' + + '
' + + '
' + + '
' + + + // Value: "false" + '
' + + '
' + + '
' + + '
' + '
'); - var divElement = element.find('div'); - var ulElement = element.find('ul'); - var liElement = element.find('li'); + // Support: Safari 11-12+ + // Attach to DOM, because otherwise Safari will not update the `isContentEditable` property + // based on the `contenteditable` attribute. + $document.find('body').append(element); + + var containers = element.children(); + var container; // Using `browserTrigger()`, because it supports event bubbling. - // Element are not editable yet. - browserTrigger(divElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); - browserTrigger(ulElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); - browserTrigger(liElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); + // No attribute | Elements are not editable. + container = containers.eq(0); + browserTrigger(container.find('div'), 'keydown', eventFor(13)); + browserTrigger(container.find('ul'), 'keydown', eventFor(32)); + browserTrigger(container.find('li'), 'keydown', eventFor(13)); expect(clickEvents).toEqual(['div(true)', 'ul(true)', 'li(true)']); + // Value: "" | Elements are editable. clickEvents = []; - scope.$apply('edit = true'); + container = containers.eq(1); + browserTrigger(container.find('div'), 'keydown', eventFor(32)); + browserTrigger(container.find('ul'), 'keydown', eventFor(13)); + browserTrigger(container.find('li'), 'keydown', eventFor(32)); + + expect(clickEvents).toEqual(['div(false)', 'ul(true)', 'li(false)']); - // Element are now editable. - browserTrigger(divElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); - browserTrigger(ulElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 13}); - browserTrigger(liElement, 'keydown', {bubbles: true, cancelable: true, keyCode: 32}); + // Value: "true" | Elements are editable. + clickEvents = []; + container = containers.eq(2); + browserTrigger(container.find('div'), 'keydown', eventFor(13)); + browserTrigger(container.find('ul'), 'keydown', eventFor(32)); + browserTrigger(container.find('li'), 'keydown', eventFor(13)); expect(clickEvents).toEqual(['div(false)', 'ul(true)', 'li(false)']); - } + + // Value: "false" | Elements are not editable. + clickEvents = []; + container = containers.eq(3); + browserTrigger(container.find('div'), 'keydown', eventFor(32)); + browserTrigger(container.find('ul'), 'keydown', eventFor(13)); + browserTrigger(container.find('li'), 'keydown', eventFor(32)); + + expect(clickEvents).toEqual(['div(true)', 'ul(true)', 'li(true)']); + }) ); they('should not prevent default keyboard action if an interactive $type element' +