From c52c785c50462f374c1ed81395adf2fc3f492a61 Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Mon, 15 Apr 2019 22:06:48 -0300 Subject: [PATCH 1/2] refactor: simplify and rename mayContainChildComponent function --- src/util/isChildNodeOf.ts | 11 +++++++++++ src/util/mayContainChildComponent.ts | 22 ---------------------- 2 files changed, 11 insertions(+), 22 deletions(-) create mode 100644 src/util/isChildNodeOf.ts delete mode 100644 src/util/mayContainChildComponent.ts diff --git a/src/util/isChildNodeOf.ts b/src/util/isChildNodeOf.ts new file mode 100644 index 000000000..a0eeaced0 --- /dev/null +++ b/src/util/isChildNodeOf.ts @@ -0,0 +1,11 @@ +import { ElementAst } from '@angular/compiler'; + +export const isChildNodeOf = (root: ElementAst, childNodeName: string): boolean => { + const traverseChildNodes = (node: ElementAst): boolean => { + return node.children.some(childNode => { + return childNode instanceof ElementAst && (childNode.name === childNodeName || traverseChildNodes(childNode)); + }); + }; + + return traverseChildNodes(root); +}; diff --git a/src/util/mayContainChildComponent.ts b/src/util/mayContainChildComponent.ts deleted file mode 100644 index d7f1be99e..000000000 --- a/src/util/mayContainChildComponent.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { ElementAst } from '@angular/compiler'; - -export function mayContainChildComponent(root: ElementAst, componentName: string): boolean { - function traverseChildren(node: ElementAst): boolean { - if (!node.children) { - return false; - } - if (node.children) { - for (let i = 0; i < node.children.length; i += 1) { - const childNode: ElementAst = node.children[i]; - if (childNode.name === componentName) { - return true; - } - if (traverseChildren(childNode)) { - return true; - } - } - } - return false; - } - return traverseChildren(root); -} From e134865de53b1e9f4be2804af1a71460cca54370 Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Mon, 15 Apr 2019 22:08:51 -0300 Subject: [PATCH 2/2] fix(rule): template-accessibility-label-for not recognizing options and interpolated values --- src/templateAccessibilityLabelForRule.ts | 186 ++++++++++++------ .../templateAccessibilityLabelForRule.spec.ts | 125 ++++++++---- 2 files changed, 213 insertions(+), 98 deletions(-) diff --git a/src/templateAccessibilityLabelForRule.ts b/src/templateAccessibilityLabelForRule.ts index db6c6d779..3ac689888 100644 --- a/src/templateAccessibilityLabelForRule.ts +++ b/src/templateAccessibilityLabelForRule.ts @@ -1,57 +1,93 @@ import { ElementAst } from '@angular/compiler'; -import { IRuleMetadata, RuleFailure, Rules, Utils } from 'tslint/lib'; +import { IOptions, IRuleMetadata, RuleFailure } from 'tslint/lib'; +import { AbstractRule } from 'tslint/lib/rules'; +import { dedent } from 'tslint/lib/utils'; import { SourceFile } from 'typescript/lib/typescript'; +import { ComponentMetadata } from './angular/metadata'; import { NgWalker, NgWalkerConfig } from './angular/ngWalker'; import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor'; -import { mayContainChildComponent } from './util/mayContainChildComponent'; +import { isChildNodeOf } from './util/isChildNodeOf'; +import { objectKeys } from './util/objectKeys'; -interface ILabelForOptions { - labelComponents: string[]; - labelAttributes: string[]; - controlComponents: string[]; -} -export class Rule extends Rules.AbstractRule { +const OPTION_CONTROL_COMPONENTS = 'controlComponents'; +const OPTION_LABEL_ATTRIBUTES = 'labelAttributes'; +const OPTION_LABEL_COMPONENTS = 'labelComponents'; + +const OPTION_SCHEMA_VALUE = { + properties: { + items: { + type: 'string' + }, + type: 'array', + uniqueItems: true + }, + type: 'object' +}; + +const DEFAULT_CONTROL_COMPONENTS = ['button', 'input', 'meter', 'output', 'progress', 'select', 'textarea']; +const DEFAULT_LABEL_ATTRIBUTES = ['for', 'htmlFor']; +const DEFAULT_LABEL_COMPONENTS = ['label']; + +type OptionKeys = typeof OPTION_CONTROL_COMPONENTS | typeof OPTION_LABEL_ATTRIBUTES | typeof OPTION_LABEL_COMPONENTS; + +type OptionDictionary = Readonly>>; + +const getReadableItems = (items: ReadonlyArray): string => { + const { length: itemsLength } = items; + + if (itemsLength === 1) return `"${items[0]}"`; + + return `${items + .map(x => `"${x}"`) + .slice(0, itemsLength - 1) + .join(', ')} and "${[...items].pop()}"`; +}; + +export class Rule extends AbstractRule { static readonly metadata: IRuleMetadata = { - description: 'Checks if the label has associated for attribute or a form element', - optionExamples: [[true, { labelComponents: ['app-label'], labelAttributes: ['id'], controlComponents: ['app-input', 'app-select'] }]], - options: { - items: { - type: 'object', - properties: { - labelComponents: { - type: 'array', - items: { - type: 'string' - } - }, - labelAttributes: { - type: 'array', - items: { - type: 'string' - } - }, - controlComponents: { - type: 'array', - items: { - type: 'string' - } - } + description: 'Checks if a label component is associated with a form element', + optionExamples: [ + true, + [ + true, + { + [OPTION_CONTROL_COMPONENTS]: ['app-input'] } + ], + [ + true, + { + [OPTION_CONTROL_COMPONENTS]: ['app-input', 'app-select'], + [OPTION_LABEL_ATTRIBUTES]: ['id'], + [OPTION_LABEL_COMPONENTS]: ['app-label'] + } + ] + ], + options: { + additionalProperties: false, + properties: { + [OPTION_CONTROL_COMPONENTS]: OPTION_SCHEMA_VALUE, + [OPTION_LABEL_ATTRIBUTES]: OPTION_SCHEMA_VALUE, + [OPTION_LABEL_COMPONENTS]: OPTION_SCHEMA_VALUE }, - type: 'array' + type: 'object' }, - optionsDescription: 'Add custom label, label attribute and controls', - rationale: Utils.dedent` - The label tag should either have a for attribute or should have associated control. - This rule supports two ways, either the label component should explicitly have a for attribute or a control nested inside the label component - It also supports adding custom control component and custom label component support.`, + optionsDescription: dedent` + An optional object with optional \`${OPTION_CONTROL_COMPONENTS}\`, \`${OPTION_LABEL_ATTRIBUTES}\` and \`${OPTION_LABEL_COMPONENTS}\` properties. + + * \`${OPTION_CONTROL_COMPONENTS}\` - components that must be inside a label component. Default and non overridable values are + ${getReadableItems(DEFAULT_CONTROL_COMPONENTS)}. + * \`${OPTION_LABEL_ATTRIBUTES}\` - attributes that must be set on label components. Default and non overridable values are + ${getReadableItems(DEFAULT_LABEL_ATTRIBUTES)}. + * \`${OPTION_LABEL_COMPONENTS}\` - components that act like a label. Default and non overridable values are + ${getReadableItems(DEFAULT_LABEL_COMPONENTS)}. + `, ruleName: 'template-accessibility-label-for', type: 'functionality', typescriptOnly: true }; - static readonly FAILURE_STRING = 'A form label must be associated with a control'; - static readonly FORM_ELEMENTS = ['input', 'select', 'textarea']; + static readonly FAILURE_STRING = 'A label component must be associated with a form element'; apply(sourceFile: SourceFile): RuleFailure[] { const walkerConfig: NgWalkerConfig = { templateVisitorCtrl: TemplateVisitorCtrl }; @@ -59,33 +95,73 @@ export class Rule extends Rules.AbstractRule { return this.applyWithWalker(walker); } + + isEnabled(): boolean { + return super.isEnabled() && this.areOptionsValid(); + } + + private areOptionsValid(): boolean { + const { length: ruleArgumentsLength } = this.ruleArguments; + + if (ruleArgumentsLength === 0) return true; + + if (ruleArgumentsLength > 1) return false; + + const { + metadata: { options: ruleOptions } + } = Rule; + const [ruleArgument] = this.ruleArguments as ReadonlyArray; + const ruleArgumentsKeys = objectKeys(ruleArgument); + const propertiesKeys = objectKeys(ruleOptions.properties as OptionDictionary); + + return ( + ruleArgumentsKeys.every(argumentKey => propertiesKeys.includes(argumentKey)) && + ruleArgumentsKeys + .map(argumentKey => ruleArgument[argumentKey]) + .every(argumentValue => Array.isArray(argumentValue) && argumentValue.length > 0) + ); + } } class TemplateVisitorCtrl extends BasicTemplateAstVisitor { - visitElement(element: ElementAst, context: any) { + private readonly controlComponents: ReadonlySet; + private readonly labelAttributes: ReadonlySet; + private readonly labelComponents: ReadonlySet; + + constructor(sourceFile: SourceFile, options: IOptions, context: ComponentMetadata, templateStart: number) { + super(sourceFile, options, context, templateStart); + + const { controlComponents, labelAttributes, labelComponents } = (options.ruleArguments[0] || {}) as OptionDictionary; + + this.controlComponents = new Set([...DEFAULT_CONTROL_COMPONENTS, ...controlComponents]); + this.labelAttributes = new Set([...DEFAULT_LABEL_ATTRIBUTES, ...labelAttributes]); + this.labelComponents = new Set([...DEFAULT_LABEL_COMPONENTS, ...labelComponents]); + } + + visitElement(element: ElementAst, context: any): any { this.validateElement(element); super.visitElement(element, context); } - private validateElement(element: ElementAst) { - let { labelAttributes, labelComponents, controlComponents }: ILabelForOptions = this.getOptions() || {}; - controlComponents = Rule.FORM_ELEMENTS.concat(controlComponents || []); - labelComponents = ['label'].concat(labelComponents || []); - labelAttributes = ['for'].concat(labelAttributes || []); + private hasControlComponentInsideElement(element: ElementAst): boolean { + return Array.from(this.controlComponents).some(controlComponentName => isChildNodeOf(element, controlComponentName)); + } - if (labelComponents.indexOf(element.name) === -1) { - return; - } - const hasForAttr = element.attrs.some(attr => labelAttributes.indexOf(attr.name) !== -1); - const hasForInput = element.inputs.some(input => { - return labelAttributes.indexOf(input.name) !== -1; - }); + private hasValidAttrOrInput(element: ElementAst): boolean { + return [...element.attrs, ...element.inputs] + .map(attrOrInput => attrOrInput.name) + .some(attrOrInputName => this.labelAttributes.has(attrOrInputName)); + } - const hasImplicitFormElement = controlComponents.some(component => mayContainChildComponent(element, component)); + private isLabelComponent(element: ElementAst): boolean { + return this.labelComponents.has(element.name); + } - if (hasForAttr || hasForInput || hasImplicitFormElement) { + private validateElement(element: ElementAst): void { + if (!this.isLabelComponent(element) || this.hasValidAttrOrInput(element) || this.hasControlComponentInsideElement(element)) { return; } + const { sourceSpan: { end: { offset: endOffset }, diff --git a/test/templateAccessibilityLabelForRule.spec.ts b/test/templateAccessibilityLabelForRule.spec.ts index 5c6f42e3d..8084f8a9b 100644 --- a/test/templateAccessibilityLabelForRule.spec.ts +++ b/test/templateAccessibilityLabelForRule.spec.ts @@ -8,7 +8,7 @@ const { describe(ruleName, () => { describe('failure', () => { - it("should fail when label doesn't have for attribute", () => { + it('should fail if a label does not have a "for" attribute', () => { const source = ` @Component({ template: \` @@ -16,8 +16,9 @@ describe(ruleName, () => { ~~~~~~~ \` }) - class Bar {} + class Test {} `; + assertAnnotated({ message: FAILURE_STRING, ruleName, @@ -25,7 +26,7 @@ describe(ruleName, () => { }); }); - it("should fail when custom label doesn't have label attribute", () => { + it('should fail if a label component does not have a label attribute', () => { const source = ` @Component({ template: \` @@ -33,46 +34,98 @@ describe(ruleName, () => { ~~~~~~~~~~~ \` }) - class Bar {} + class Test {} `; + const options = [ + { + labelAttributes: ['id'], + labelComponents: ['app-label'] + } + ]; + assertAnnotated({ message: FAILURE_STRING, + options, ruleName, - source, - options: { - labelComponents: ['app-label'], - labelAttributes: ['id'] + source + }); + }); + + it('should fail if a label component does not have a control component inside it', () => { + const source = ` + @Component({ + template: \` + + ~~~~~~~~~~~ + \` + }) + class Test {} + `; + const options = [ + { + controlComponents: ['app-input'], + labelComponents: ['app-label'] } + ]; + + assertAnnotated({ + message: FAILURE_STRING, + options, + ruleName, + source }); }); }); describe('success', () => { - it('should work when label has for attribute', () => { + it('should succeed if a label has "for" attribute', () => { const source = ` @Component({ template: \` + \` }) - class Bar {} + class Test {} `; + assertSuccess(ruleName, source); }); - it('should work when label are associated implicitly', () => { + it('should succeed if a label component has a label attribute', () => { + const source = ` + @Component({ + template: \` + + + + \` + }) + class Test {} + `; + const options = [ + { + labelAttributes: ['id'], + labelComponents: ['app-label'] + } + ]; + + assertSuccess(ruleName, source, options); + }); + + it('should succeed if a label component has a control component inside it', () => { const source = ` @Component({ template: \` @@ -82,46 +135,32 @@ describe(ruleName, () => { \` }) - class Bar {} + class Test {} `; - assertSuccess(ruleName, source, { - labelComponents: ['app-label'], - controlComponents: ['app-input'] - }); - }); + const options = [ + { + controlComponents: ['app-input'], + labelComponents: ['app-label'] + } + ]; - it("should fail when label doesn't have for attribute", () => { - const source = ` - @Component({ - template: \` - - \` - }) - class Bar {} - `; - assertSuccess(ruleName, source); + assertSuccess(ruleName, source, options); }); - it('should work when custom label has label attribute', () => { + it('should succeed if a label component is associated with a form element', () => { const source = ` @Component({ template: \` - - + + + + \` }) - class Bar {} + class Test {} `; - assertSuccess(ruleName, source, { - labelComponents: ['app-label'], - labelAttributes: ['id'] - }); + + assertSuccess(ruleName, source); }); }); });