From bebc1859feabaaf6cced70cc5056b54b59db6acd Mon Sep 17 00:00:00 2001 From: Kyle Date: Tue, 7 Nov 2017 19:17:36 -0600 Subject: [PATCH 1/2] Default to using tagName div with role of region and updating assertion and change to warning to reflect intended use in comments --- addon/components/a11y-landmark.js | 22 +++++++++++------ tests/unit/components/a11y-landmark-test.js | 27 +++++++++++++++------ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/addon/components/a11y-landmark.js b/addon/components/a11y-landmark.js index 0574070..c216250 100644 --- a/addon/components/a11y-landmark.js +++ b/addon/components/a11y-landmark.js @@ -8,7 +8,7 @@ const LANDMARK_NAVIGATION_ROLE = { main: 'main', footer: 'contentinfo', form: 'form', - div: 'div' + div: 'region' }; const VALID_LANDMARK_ROLES = [ @@ -46,10 +46,10 @@ export default Ember.Component.extend({ * nav (navigation) * div (application, document, region or any of the previous) */ - tagName: null, + tagName: 'div', /*should only be set if ('div' or 'form') is being used as a tagName, otherwise we don't need it. - * valid values: + * valid values: * banner * navigation * aside @@ -62,8 +62,8 @@ export default Ember.Component.extend({ */ landmarkRole: null, - /*we should set an aria-role when either a native element is not used, or the native element does not have the body element as its parent. - * since nothing is going to be the direct child of the body in an Ember app, we don't have to check for that. + /*we should set an aria-role when either a native element is not used, or the native element does not have the body element as its parent. + * since nothing is going to be the direct child of the body in an Ember app, we don't have to check for that. */ ariaRole: Ember.computed('tagName', 'landmarkRole', function() { const landmark = this.get('tagName'); @@ -74,8 +74,11 @@ export default Ember.Component.extend({ return 'search'; } else if (landmark != 'form' && landmarkRole === 'search') { Ember.assert('This is not a valid combination. Use the form element for a search.'); + } else if (landmark === 'div') { + this._validateLandmarkRole(landmarkRole); + return landmarkRole; } else { - Ember.assert('Cannot set both "tagName" and "landMarkRole." Use one or the other.'); + Ember.assert('Only "div" or "form" can be used with "landMarkRole." Use one or the other.'); } } else if (landmarkRole) { this._validateLandmarkRole(landmarkRole); @@ -84,7 +87,10 @@ export default Ember.Component.extend({ this._validateTagName(landmark); return LANDMARK_NAVIGATION_ROLE[landmark]; } else { - Ember.assert('Must specify either tagName or landmarkRole'); + Ember.warn('Should specify either tagName or landmarkRole', { + id: 'ember-a11y.no-tagName-or-landmarkRole' + }); + return 'region'; } }), @@ -108,4 +114,4 @@ export default Ember.Component.extend({ // add support for an aria-label, since a landmark element can have one defined. ariaLabel: null -}); \ No newline at end of file +}); diff --git a/tests/unit/components/a11y-landmark-test.js b/tests/unit/components/a11y-landmark-test.js index 9c34a7f..2317e76 100644 --- a/tests/unit/components/a11y-landmark-test.js +++ b/tests/unit/components/a11y-landmark-test.js @@ -76,7 +76,7 @@ test('it fails to determine ariaRole when landmarkRole is invalid', function(ass }); test('it fails to determine ariaRole when tagName and landmarkRole are both provided', function(assert) { - const expectedErrorMessage = 'Cannot set both \"tagName\" and \"landMarkRole.\" Use one or the other.'; + const expectedErrorMessage = 'Only "div" or "form" can be used with "landMarkRole." Use one or the other.'; assert.throws(() => { const component = this.subject({ @@ -87,11 +87,22 @@ test('it fails to determine ariaRole when tagName and landmarkRole are both prov }, buildEmberAssertion(expectedErrorMessage)); }); -test('it fails to determine ariaRole when neither tagName nor landmarkRole are provided', function(assert) { - const expectedErrorMessage = 'Must specify either tagName or landmarkRole'; +test('it defaults to div[role="region"] when neither tagName nor landmarkRole are provided', function(assert) { + const component = this.subject({}); - assert.throws(() => { - const component = this.subject(); - component.get('ariaRole'); - }, buildEmberAssertion(expectedErrorMessage)); -}); \ No newline at end of file + const ariaRole = component.get('ariaRole'); + assert.equal(ariaRole, 'region'); + + const tagName = component.get('tagName'); + assert.equal(tagName, 'div'); +}); + +test('it sets tagName correctly when tagName is "form" and landmarkRole is "search"', function(assert) { + const component = this.subject({ + tagName: 'form', + landmarkRole: 'search' + }); + const tagName = component.get('tagName'); + + assert.equal(tagName, 'form'); +}); From 2e226341075db7e334744ee65095e6b54444b2ef Mon Sep 17 00:00:00 2001 From: Kyle Date: Tue, 7 Nov 2017 19:25:52 -0600 Subject: [PATCH 2/2] Adding tests to make sure tagName works as expected --- tests/unit/components/a11y-landmark-test.js | 22 +++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/unit/components/a11y-landmark-test.js b/tests/unit/components/a11y-landmark-test.js index 2317e76..3cdc2f8 100644 --- a/tests/unit/components/a11y-landmark-test.js +++ b/tests/unit/components/a11y-landmark-test.js @@ -87,7 +87,7 @@ test('it fails to determine ariaRole when tagName and landmarkRole are both prov }, buildEmberAssertion(expectedErrorMessage)); }); -test('it defaults to div[role="region"] when neither tagName nor landmarkRole are provided', function(assert) { +test('it defaults to tagName "div" and role "region" when neither tagName nor landmarkRole are provided', function(assert) { const component = this.subject({}); const ariaRole = component.get('ariaRole'); @@ -97,7 +97,7 @@ test('it defaults to div[role="region"] when neither tagName nor landmarkRole ar assert.equal(tagName, 'div'); }); -test('it sets tagName correctly when tagName is "form" and landmarkRole is "search"', function(assert) { +test('it sets tagName to "form" when tagName is "form" and landmarkRole is "search"', function(assert) { const component = this.subject({ tagName: 'form', landmarkRole: 'search' @@ -106,3 +106,21 @@ test('it sets tagName correctly when tagName is "form" and landmarkRole is "sear assert.equal(tagName, 'form'); }); + +test('it sets tagName to "form" when tagName is "form" and no landmarkRole is provided', function(assert) { + const component = this.subject({ + tagName: 'form' + }); + const tagName = component.get('tagName'); + + assert.equal(tagName, 'form'); +}); + +test('it sets tagName to "div" when landmarkRole is provided', function(assert) { + const component = this.subject({ + landmarkRole: 'navigation' + }); + const tagName = component.get('tagName'); + + assert.equal(tagName, 'div'); +});