feat(rule): Add new rule - react-a11y-media-captions #850

noamyogev84 · 2019-03-15T21:43:32Z

PR checklist

Addresses an existing issue: fixes new rule: react-a11y-media #195
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Added new rule react-a11y-media-captions. Rule will verify that video and audio elements have captions and descriptions. (example below)

    <video>
        <source src="sitting_and_smiling_234.mp4" type="video/mp4"/>
        <track src="sitting_and_smiling_234.vtt" kind="captions" srclang="en" label="English"/>
        <track src="myvideo_en.vtt" kind="description" srclang="en" label="English"/>
    </video>;

    <audio>
        <source src="beck_loser.mp3" type="video/mp4"/>
        <track src="beck_loser.vtt" kind="captions" srclang="en" label="English"/>
    </audio>;

fix behavior of validateMediaType

…t-contrib into issue_195

noamyogev84 · 2019-03-16T16:01:32Z

@JoshuaKGoldberg @IllusionMH Hey guys, would like your review on this.
Again, I have no luck with those builds failing (local build + tests are fine!) 🤦‍♂️
Any clue what's going on there???

IllusionMH · 2019-03-16T16:40:24Z

@noamyogev84 thanks for PR, will try to review it this weekend.

CI fails on Node 6, where npm does support package-lock.json and installs versions of TSLint with new rule that fails. PR #846 will fix CI errors.

noamyogev84 · 2019-03-16T16:56:31Z

@IllusionMH great, thanks man!

IllusionMH

Rule code look good. Handles case that I've missed. 👍

However, two paths doesn't have failing and successful test, which will be really helpful.

Could you please rebase your branch, and then run npm install and npm test? There are some unexpected changes in tslint-warnings.csv and new entry should be added to configs/latest.json.

README.md

src/reactA11yMediaCaptionsRule.ts

IllusionMH · 2019-03-18T03:19:03Z

src/reactA11yMediaCaptionsRule.ts

+function getAttributeText(attribute: ts.JsxAttribute): string | undefined {
+    if (attribute && attribute.initializer) {
+        if (tsutils.isJsxExpression(attribute.initializer)) {
+            return attribute.initializer.expression ? attribute.initializer.expression.getText() : undefined;


Looks like there are no tests for this line.

src/reactA11yMediaCaptionsRule.ts

IllusionMH · 2019-03-18T03:24:02Z

tslint-warnings.csv

@@ -156,6 +156,7 @@ no-redundant-jsdoc,Forbids JSDoc which duplicates TypeScript functionality.,TSLI
 no-reference-import,Don't `<reference types="foo" />` if you import `foo` anyway.,TSLINT178RR3K,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation"
 no-regex-spaces,Do not use multiple spaces in a regular expression literal. Similar to the ESLint no-regex-spaces rule,TSLINT1T5TJ80,tslint,Non-SDL,Error,Critical,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
 no-reserved-keywords,"Do not use reserved keywords as names of local variables, fields, functions, or other identifiers.",TSLINT14J77I2,tslint,SDL,Error,Critical,Mandatory,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,398,"CWE 398 - Indicator of Poor Code Quality"
+no-restricted-globals,Disallow specific global variables.,TSLINTGKU2KB,tslint,,,,,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,


Unexpected change, if correct version of packages is installed for this branch.

pulled latest master version + run npm install,
still this entry is there. 😮

Thanks for pinging. Will check what's with this line today evening.

You are correct. Looks like these strings were not added during update of tslint package.

IllusionMH · 2019-03-24T21:29:21Z

Fixes for lint erros in Node 6 are in master.

JoshuaKGoldberg · 2019-04-04T11:46:49Z

Ping @noamyogev84, do you still think you'll have time to get to these changes?

noamyogev84 · 2019-04-04T15:10:45Z

@JoshuaKGoldberg I'll continue today. 🍻

…t-contrib into issue_195

add unit tests

noamyogev84 · 2019-04-05T14:44:14Z

@IllusionMH @JoshuaKGoldberg
Did some work here, i'll appreciate your review 🍺

IllusionMH

Sorry for delay with review, it was tought weekend.
I think that better to ckeck strings values instead of variable names inside JsxAttribute's.

Also would be nice to avoid erros if there are spread attributes, however this one is optional and up to you.

IllusionMH · 2019-04-08T07:27:03Z

tests/react-a11y-media-captions/test.tsx.lint

+const SomeComponent = () =>
+<video width="320" height="240">
+    <source src="sitting_and_smiling_232.mp4" type="video/mp4"/>
+    <track src="subtitles_en.vtt" kind={subtitles} srclang="en" label="English"/>


Hmm... Looks like I've missed what will be checked inside JsxExpression.
I would expect simple check for string kind={'subtitles'} but not for variable name.
Without type info (I don't think it is needed for initial implementation) it would be great to check only strings inside JsxExpression.

Also would be nice to skip errors if there are spread attributes on track elements (see reasoning here)

Hi @IllusionMH
I'm sorry for being away for so long... 🙏

changed expression check according to your comments.
also, added Spread Attribute detection on track elements, so the following test should pass:

<video width="320" height="240"> <source src="sitting_and_smiling_260.mp4" type="video/mp4"/> <track {...props} /> <track src="subtitles_en.vtt" kind="subtitles" srclang="en" label="English"/> </video>;```

text to look only for string literals

add spread attributes logic to track elements

IllusionMH · 2019-07-09T22:22:18Z

@noamyogev84 thanks for following up on this PR!
Test cases look good, however as I understand - passing variable instead of string literal to kind attribute expression will trigger an error. Not sure if potential error here aligns with similar rules, but need to check first.
I will get to PC and finish reviewie tomorrow.

IllusionMH · 2019-07-10T23:32:16Z

tslint-warnings.csv

@@ -149,7 +149,6 @@ CWE 710 - Coding Standards Violation"
 no-misused-new,Warns on apparent attempts to define constructors for interfaces or `new` for classes.,TSLINTL96MA6,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
 no-multiline-string,Do not declare multiline strings,TSLINT10K5P9U,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation"
 no-non-null-assertion,Disallows non-null assertions using the `!` postfix operator.,TSLINTNO75FN,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation"
-no-null-undefined-union,Disallows explicitly declared or implicitly returned union types with both `null` and `undefined` as members.         ,TSLINT1JAMNL7,tslint,,,,,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,


Not sure why this line was deleted - it is added back on my PC.
Could please run npm install (with npm 5+) and then npm test again?

IllusionMH

Overall it looks good and failures are lint failures are false positives from TSLint rule only on Node v6. 🎉

Looks like other rules are using checks with getStringLiteral that will "fail" if variable is used in JSX expression. Example
However this case should be in tests so there would be no questions if it is intended or not.

@JoshuaKGoldberg do you think we should be consistent with existing rules and report errors if non-string value is used (variable or other expression) + ignore checks if spread is present?

IllusionMH · 2019-07-10T23:35:28Z

src/reactA11yMediaCaptionsRule.ts

+    return elementName === AUDIO_ELEMENT_NAME;
+}
+
+function getAttributeText(attribute: ts.JsxAttribute): string | undefined {


Looks like this function can be replaced with existing utility getStringLiteral from src/utils/JsxAttribute.ts

Which has similar signature and results I think

noamyogev84 · 2019-07-19T19:35:56Z

However this case should be in tests so there would be no questions if it is intended or not.

Not sure if I understand.

IllusionMH · 2019-07-19T19:57:33Z

@noamyogev84 I mean test that will clearly state that expressions that contain variable with correct data will be still counted as error. Something like this:

const captionsKind = 'captions';
const SomeComponent = () => 
    <audio>
        <source src="beck_loser.mp3" type="video/mp4"/>
        <track src="beck_loser.vtt" kind={ captionsKind } srclang="en" label="English"/>
    </audio>;

noamyogev84 · 2019-07-20T12:25:37Z

@IllusionMH @JoshuaKGoldberg
Hey guys, anything else is needed here?

IllusionMH

Everything looks good now! The only problem as usually - npm which messed up again with package-lock.json.

It would be great if you could revert only that file. If not - I will merge this PR and regenerate it.

package-lock.json

IllusionMH · 2019-07-29T09:33:15Z

@noamyogev84 everything looks great! 👍
Thank you for contribution and patience during review!

noamyogev84 added 4 commits March 3, 2019 18:06

add new reactA11yMediaCaptionsRule

4d980c9

add react-a11y-media-caption to readme

be2f106

add unit tests

4b944cc

fix behavior of validateMediaType

Merge branch 'master' of https://github.com/Microsoft/tslint-microsof…

2a61738

…t-contrib into issue_195

IllusionMH suggested changes Mar 18, 2019

View reviewed changes

JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Mar 21, 2019

noamyogev84 added 2 commits April 5, 2019 16:41

Merge branch 'master' of https://github.com/Microsoft/tslint-microsof…

042242f

…t-contrib into issue_195

fix according to comments

bbadbb1

add unit tests

JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Apr 5, 2019

IllusionMH suggested changes Apr 8, 2019

View reviewed changes

JoshuaKGoldberg added PR: Waiting for Author Changes have been requested that the pull request author should address. and removed PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! labels Apr 15, 2019

noamyogev84 added 3 commits July 9, 2019 10:28

merge master -> feature_195

15b92c8

modify get JsxExpression

85aad2a

text to look only for string literals

modify to check string literal inside JsxExpression

6948edf

add spread attributes logic to track elements

IllusionMH added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Jul 9, 2019

IllusionMH reviewed Jul 10, 2019

View reviewed changes

IllusionMH suggested changes Jul 10, 2019

View reviewed changes

replace getAttributeText with getStringLiteral

8b46abc

add uni test to indicate no variable as expression

e9b1afb

IllusionMH approved these changes Jul 21, 2019

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

IllusionMH changed the title ~~Add new rule - react-a11y-media-captions~~ feat(rule): Add new rule - react-a11y-media-captions Jul 21, 2019

git l

9088e0b

IllusionMH removed the PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! label Jul 29, 2019

IllusionMH merged commit 8f86920 into microsoft:master Jul 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rule): Add new rule - react-a11y-media-captions #850

feat(rule): Add new rule - react-a11y-media-captions #850

noamyogev84 commented Mar 15, 2019 •

edited

Loading

noamyogev84 commented Mar 16, 2019

IllusionMH commented Mar 16, 2019

noamyogev84 commented Mar 16, 2019

IllusionMH left a comment

IllusionMH Mar 18, 2019

IllusionMH Mar 18, 2019

noamyogev84 Apr 5, 2019

IllusionMH Apr 5, 2019

IllusionMH Apr 8, 2019

IllusionMH commented Mar 24, 2019

JoshuaKGoldberg commented Apr 4, 2019

noamyogev84 commented Apr 4, 2019

noamyogev84 commented Apr 5, 2019

IllusionMH left a comment

IllusionMH Apr 8, 2019

noamyogev84 Jul 9, 2019

IllusionMH commented Jul 9, 2019

IllusionMH Jul 10, 2019

IllusionMH left a comment

IllusionMH Jul 10, 2019

noamyogev84 commented Jul 19, 2019 •

edited

Loading

IllusionMH commented Jul 19, 2019

noamyogev84 commented Jul 20, 2019

IllusionMH left a comment

IllusionMH commented Jul 29, 2019

feat(rule): Add new rule - react-a11y-media-captions #850

feat(rule): Add new rule - react-a11y-media-captions #850

Conversation

noamyogev84 commented Mar 15, 2019 • edited Loading

PR checklist

Overview of change:

noamyogev84 commented Mar 16, 2019

IllusionMH commented Mar 16, 2019

noamyogev84 commented Mar 16, 2019

IllusionMH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IllusionMH commented Mar 24, 2019

JoshuaKGoldberg commented Apr 4, 2019

noamyogev84 commented Apr 4, 2019

noamyogev84 commented Apr 5, 2019

IllusionMH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IllusionMH commented Jul 9, 2019

Choose a reason for hiding this comment

IllusionMH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamyogev84 commented Jul 19, 2019 • edited Loading

IllusionMH commented Jul 19, 2019

noamyogev84 commented Jul 20, 2019

IllusionMH left a comment

Choose a reason for hiding this comment

IllusionMH commented Jul 29, 2019

noamyogev84 commented Mar 15, 2019 •

edited

Loading

noamyogev84 commented Jul 19, 2019 •

edited

Loading