Implement react-a11y-iframes rule #692

noamyogev84 · 2018-12-21T23:03:13Z

PR checklist

Addresses an existing issue: fixes new rule: react-a11y-iframe #204
enhancement
Includes tests
Documentation update

Overview of change:

Validate that title attribute exists, and is a non-empty expression or text.
Validate that src attribute exists, and is a non-empty text.
Validate that hidden attribute doesn't exist.
Validate that title attribute text is unique when there are 2 or more iframe elements in scope.

add unit tests

noamyogev84 · 2018-12-21T23:04:31Z

Hi @JoshuaKGoldberg.
I'd love your review on this one. Please let me know if you think more scenarios (React Components, Functions) should be covered as well.

…rosoft-contrib into issue_204

IllusionMH · 2018-12-23T03:50:13Z

Hi @noamyogev84 ! Thanks for rule implementation!

We are in process of migration to TSLint test format now (see #489).
Could you convert tests to the TSLint format? If not, it's OK and we will convert it later.

noamyogev84 · 2018-12-23T08:45:25Z

Hi @IllusionMH no problem, my pleasure!
Trying to play a little bit with the new test format. Running node --inspect-brk ./node_modules/tslint/bin/tslint --test -r dist/src "tests/**" and no breakpoints are being hit in chrome devTools. Am i missing something?

IllusionMH · 2018-12-23T14:12:27Z

There are prepared debug configs if you use VS Code.

I had problem with adding folder to DevTools (in Opera) so
I've added at the beginning of the file

// tslint:disable-next-line
debugger;

to get to file, and then was able to place breakpoints where I like (there should be original mapped TS code).

If you are on Windows, it might be useful to replace "sourceMap": true, with "inlineSourceMap": true,' in tsconfig.json if approach above doesn't work for you

noamyogev84 · 2018-12-23T15:03:08Z

No success on both approaches.
It seems like the vscode debugger just can't find anything to debug with the tslint configuration. :(

IllusionMH · 2018-12-23T15:22:30Z

It doesn't work for your rule, or for existing too?
For example I've tried no-relative-imports and added src/noRelativeImportsRule.ts respectively.

Also noticed extra /**" in command that you pasted. Not sure if it is only paste error.

JoshuaKGoldberg

Waiting on the super comment, with a couple of nitpicks on the description. Will leave testing changes to IllusionMH :)

README.md

src/reactA11yIframesRule.ts

JoshuaKGoldberg

Waiting on the super comment, with a couple of nitpicks on the description. Will leave testing changes to IllusionMH :)

(hit the wrong button first...)

IllusionMH · 2018-12-26T17:31:05Z

Will leave testing changes to IllusionMH :)

Will do!

noamyogev84 if you still have problems with debug (can't find last message here) - could you please create issue node version and system info. Will investigate it further.

JoshuaKGoldberg · 2018-12-26T19:02:44Z

Oh @IllusionMH I just noticed the ambiguity in my statement - I meant leaving reviewing the changes to the rule's tests to you, not leaving testing out the rule. 😄

README.md

noamyogev84 · 2018-12-28T10:26:05Z

Will leave testing changes to IllusionMH :)

Will do!

noamyogev84 if you still have problems with debug (can't find last message here) - could you please create issue node version and system info. Will investigate it further.

@IllusionMH Will do. (issue #703)

noamyogev84 · 2019-01-04T14:34:24Z

@JoshuaKGoldberg @IllusionMH maybe some additional input: Tests for React related code must have a tsx extension to set the languageVariant properly. the following code explains it best:

return sourceFile.languageVariant === ts.LanguageVariant.JSX
            ? this.applyWithWalker(new ReactA11yIframesRuleWalker(sourceFile, this.getOptions()))
            : [];

IllusionMH · 2019-01-04T14:48:49Z

@noamyogev84 sorry, I don't actually understand your last comment.

If you ask about how file with JSX tags should be named in new tests format then it should be test.tsx.lint (you can also check tests in tslint-react repo)

If something else - clarify it, please.

fix build error (String -> string)

noamyogev84 · 2019-01-04T14:56:52Z

@IllusionMH yeah well it seems pretty obvious i guess, i wasn't familiar with that. 😞
Anyway, tests are now included for you to review. 🥂

Outdated

IllusionMH · 2019-01-04T14:59:49Z

@noamyogev84 thanks! I will review it tonight.

IllusionMH

Migrated tests looks good 👍. Other review comments about existing code are mostly nit's.

All auto generated files can be easily re-created after packages update and it shoul remove unnecessary changes in them.

I also found that there are no test cases for class component and functional components (as function declaration).
example code

class Sidebar extends React.Component {
    render() {
        let thirdBlock = <p>No video at this moment</p>;

        if (this.props.hasVideo) {
            thirdBlock = <iframe src="http://someSource.com"></iframe>;
        }

        return (
            <aside>
                <h3>Calendar</h3>
                <div className="responsive-calendar-wrapper">
                    <iframe src="http://someSource.com" />
                </div>
                <hr />
                <h3>Ads</h3>
                <iframe title="Our sponsors" src="" />
                { thirdBlock }
            </aside>
        )
    }
}

function FunctionComponent() {
    return (
        <aside>
            <h3>Calendar</h3>
            <div className="responsive-calendar-wrapper">
                <iframe src="http://someSource.com" />
            </div>
            <hr />
            <h3>Ads</h3>
            <iframe title="Our sponsors" src="" />
        </aside>
    )
}

And looks like current implementation won't trigger errors for any of these iframe elements.

I can suggest that instead of looking for elements that might contain JSX elements, look directly for JsxElement, JsxSelfClosingElement, and JsxFragment elements. In this way you'll be able start your validation function from top-most JSX elements (or their parent for simplicity).

Unfortunately Lint.RuleWalker doesn't have visitJsxFragment method, however it can be implemented with walker function (you can check jsx-ban-elements rule for example).

Do not hesitate to ping me if you have any questions.

IllusionMH · 2019-01-05T01:23:51Z

src/reactA11yIframesRule.ts

+    public static metadata: ExtendedMetadata = {
+        ruleName: 'react-a11y-iframes',
+        type: 'functionality',
+        description: 'Enforce that iframe elements are not empty, have title and are unique.',


Comma was added to README.md, but not here.

IllusionMH · 2019-01-05T01:26:09Z

src/tests/reactA11yIframesRuleTests.ts

+const IFRAME_EMPTY_OR_HIDDEN_ERROR_STRING: string = 'An iframe element should not be hidden or empty.';
+const IFRAME_UNIQUE_TITLE_ERROR_STRING: string = 'An iframe element must have a unique title.';
+
+describe('reactA11yIframesRuleTests', (): void => {


This file isn't needed anymore and should be removed.
All tests are now in new format (thanks for converting them! 🙌) that are even better - with checks where error range ends.

IllusionMH · 2019-01-05T01:37:39Z

tests/react-a11y-iframes/test.tsx.lint

@@ -0,0 +1,24 @@
+
+const someComponent = () => <iframe title="I'm a non empty title" src="http://someSource.com"/>;


nit. Would expect component function to start from upper case letter.

IllusionMH · 2019-01-05T01:41:04Z

tests/react-a11y-iframes/test.tsx.lint

+const someComponent = () => <>
+            <iframe title="hi there" src="http://someSource.com"></iframe>
+            <iframe title="hello there" src="http://someSource.com"></iframe>
+        </>


nit. indentation looks off in these two blocks (same as in mocha test file where these strings were deeply nested).

IllusionMH · 2019-01-05T01:43:03Z

tslint-warnings.csv

@@ -15,8 +15,6 @@ class-name,Enforces PascalCased class and interface names.,TSLINT65UF71,tslint,N
 CWE 710 - Coding Standards Violation"
 comment-format,Enforces formatting rules for single-line comments.,TSLINT1T6OE84,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,"398, 710","CWE 398 - Indicator of Poor Code Quality
 CWE 710 - Coding Standards Violation"
-comment-type,Allows a limited set of comment types,TSLINT78JBS7,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,"398, 710","CWE 398 - Indicator of Poor Code Quality


This is not expected change here.

IllusionMH · 2019-01-05T01:51:38Z

tslint-warnings.csv

@@ -245,7 +236,7 @@ CWE 705 - Incorrect Control Flow Scoping
 CWE 710 - Coding Standards Violation"
 prefer-for-of,Recommends a 'for-of' loop over a standard 'for' loop if the index is only used to access the array being iterated.,TSLINT51MHG7,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
 prefer-method-signature,Prefer `foo(): void` over `foo: () => void` in interfaces and types.,TSLINT1LVIQFA,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,,
-prefer-object-spread,Enforces the use of the ES2018 object spread operator over `Object.assign()` where appropriate.,TSLINT10K16KT,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"
+prefer-object-spread,Enforces the use of the ES2015 object spread operator over `Object.assign()` where appropriate.,TSLINT10K16KT,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"


ES2015 was corrected to ES2018 in TSLint repo. This feels like a rollback.
I assume that this wile was generated after merge, but without npm install that should have updated TSLint to 5.12.0 that has metadata updates and new rules (that were removed above).

Please run npm install, npm test, and commit updated file (this file should only contain diff with your rule).

add comma to rule description fix function names in test.tsx.lint

noamyogev84 · 2019-01-08T17:48:21Z

I'll start by re implementing the rule with the walk function.
@JoshuaKGoldberg @IllusionMH would you care to explain what's the reason we're changing all the rules to this pattern? pretty interesting to understand 😄

add unit tests for class and function rule enforcement

IllusionMH · 2019-01-09T11:03:21Z

TL;DR; walk function approach easier to control (e.g. reset counters/maps after children iteration) and should be faster (especially if early stop walking through nodes that definitely won't have an error earlier).

There is an issue in TSLint to mark RuleWalker as deprecated (you can also check linked discussion).

I think it is similar to tsUtils.forEachToken, but with greater control on each step, instead of simple callbacks (as I see there is no way to stop further iteration there).

IllusionMH

Thank you for updates! Reworked code looks clearer to me 👍
The only question to discuss at the moment if titles list should be reset when JsxFragment is encountered.

Tests looks good, and I will check if there any other cases that may not be handled later.

src/reactA11yIframesRule.ts

Dismissing previous "Changes required" review because code is updated. However thaere are points to check/discuss before final approve

IllusionMH

Current implementation and tests is good 👍

Detecting duplicate titles can be tricky for any code, but I think in this state it will do a good enough job. If there will be need - rule can be improved later.

@JoshuaKGoldberg do you have any on the notes/suggestion?

JoshuaKGoldberg

Sorry for the delay @noamyogev84! This looks wonderful and I'm excited to get it in for 6.1.0.

Just the one missing test case - looks like it should already pass, so if you don't have time to get to it in the next few days, I can always add it in.

tests/react-a11y-iframes/test.tsx.lint

JoshuaKGoldberg

Sorry for the delay @noamyogev84! This looks wonderful and I'm excited to get it in for 6.1.0.

Just the one missing test case - looks like it should already pass, so if you don't have time to get to it in the next few days, I can always add it in.

(hit the wrong button, agh)

JoshuaKGoldberg · 2019-01-19T20:52:24Z

Whoo, thanks @noamyogev84 and @IllusionMH! 🙌

* implement reactA11yIFramesRule add unit tests * generate README and tslint.json * generate README and tslint.json * Rename reactA11yIFramesRule.ts to reactA11yIframesRule.ts * Rename reactA11yIFramesRuleTests.ts to reactA11yIframesRuleTests.ts * add call to super, fix README.md, lowercase tsUtils * add tslint tests fix build error (String -> string) * update tslint-warnings after npm install * remove mocha test add comma to rule description fix function names in test.tsx.lint * refactor reactA11yIframsRule to use walk function add unit tests for class and function rule enforcement * remove resetting previousTiltles when JsxFragment is enountered * add some test cases

noamyogev84 added 2 commits December 22, 2018 00:26

implement reactA11yIFramesRule

f2bcaa9

add unit tests

generate README and tslint.json

d42998e

noamyogev84 changed the title ~~Issue 204~~ Implement react-a11y-iframes rule Dec 21, 2018

noamyogev84 added 4 commits December 22, 2018 23:57

generate README and tslint.json

2020414

Merge branch 'issue_204' of https://github.com/noamyogev84/tslint-mic…

296e445

…rosoft-contrib into issue_204

Rename reactA11yIFramesRule.ts to reactA11yIframesRule.ts

ba639ca

Rename reactA11yIFramesRuleTests.ts to reactA11yIframesRuleTests.ts

b847929

JoshuaKGoldberg approved these changes Dec 26, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/reactA11yIframesRule.ts Outdated Show resolved Hide resolved

src/reactA11yIframesRule.ts Outdated Show resolved Hide resolved

src/reactA11yIframesRule.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg previously requested changes Dec 26, 2018

View reviewed changes

JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Dec 26, 2018

IllusionMH reviewed Dec 26, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

add call to super, fix README.md, lowercase tsUtils

ab800bd

Merge branch 'master' into issue_204

77faed9

add tslint tests

d4e4311

fix build error (String -> string)

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 Jan 4, 2019

IllusionMH previously requested changes Jan 5, 2019

View reviewed changes

noamyogev84 added 2 commits January 5, 2019 10:44

update tslint-warnings after npm install

924649f

remove mocha test

443bbdb

add comma to rule description fix function names in test.tsx.lint

IllusionMH 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 Jan 6, 2019

refactor reactA11yIframsRule to use walk function

3fccff9

add unit tests for class and function rule enforcement

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 Jan 9, 2019

IllusionMH reviewed Jan 11, 2019

View reviewed changes

src/reactA11yIframesRule.ts Outdated Show resolved Hide resolved

remove resetting previousTiltles when JsxFragment is enountered

ce26229

IllusionMH approved these changes Jan 14, 2019

View reviewed changes

IllusionMH mentioned this pull request Jan 19, 2019

Planning for 6.1.0 #788

Closed

JoshuaKGoldberg approved these changes Jan 19, 2019

View reviewed changes

tests/react-a11y-iframes/test.tsx.lint Show resolved Hide resolved

JoshuaKGoldberg suggested changes Jan 19, 2019

View reviewed changes

add some test cases

c598136

JoshuaKGoldberg approved these changes Jan 19, 2019

View reviewed changes

JoshuaKGoldberg merged commit f67d898 into microsoft:master Jan 19, 2019

drexler mentioned this pull request Jan 20, 2019

add react-a11y-iframes to recommended ruleset #794

Closed

IllusionMH added this to the 6.1.0-beta milestone Feb 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement react-a11y-iframes rule #692

Implement react-a11y-iframes rule #692

noamyogev84 commented Dec 21, 2018

noamyogev84 commented Dec 21, 2018

IllusionMH commented Dec 23, 2018

noamyogev84 commented Dec 23, 2018 •

edited

Loading

IllusionMH commented Dec 23, 2018

noamyogev84 commented Dec 23, 2018

IllusionMH commented Dec 23, 2018

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

IllusionMH commented Dec 26, 2018

JoshuaKGoldberg commented Dec 26, 2018

noamyogev84 commented Dec 28, 2018 •

edited

Loading

noamyogev84 commented Jan 4, 2019 •

edited

Loading

IllusionMH commented Jan 4, 2019

noamyogev84 commented Jan 4, 2019

IllusionMH commented Jan 4, 2019

IllusionMH left a comment

IllusionMH Jan 5, 2019

IllusionMH Jan 5, 2019

IllusionMH Jan 5, 2019

IllusionMH Jan 5, 2019

IllusionMH Jan 5, 2019

IllusionMH Jan 5, 2019

noamyogev84 commented Jan 8, 2019

IllusionMH commented Jan 9, 2019

IllusionMH left a comment

IllusionMH left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Jan 19, 2019

		@@ -0,0 +1,24 @@

		const someComponent = () => <iframe title="I'm a non empty title" src="http://someSource.com"/>;

Implement react-a11y-iframes rule #692

Implement react-a11y-iframes rule #692

Conversation

noamyogev84 commented Dec 21, 2018

PR checklist

Overview of change:

noamyogev84 commented Dec 21, 2018

IllusionMH commented Dec 23, 2018

noamyogev84 commented Dec 23, 2018 • edited Loading

IllusionMH commented Dec 23, 2018

noamyogev84 commented Dec 23, 2018

IllusionMH commented Dec 23, 2018

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

IllusionMH commented Dec 26, 2018

JoshuaKGoldberg commented Dec 26, 2018

noamyogev84 commented Dec 28, 2018 • edited Loading

noamyogev84 commented Jan 4, 2019 • edited Loading

IllusionMH commented Jan 4, 2019

noamyogev84 commented Jan 4, 2019

IllusionMH commented Jan 4, 2019

IllusionMH left a comment

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

IllusionMH Jan 5, 2019

Choose a reason for hiding this comment

noamyogev84 commented Jan 8, 2019

IllusionMH commented Jan 9, 2019

IllusionMH left a comment

Choose a reason for hiding this comment

IllusionMH left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jan 19, 2019

noamyogev84 commented Dec 23, 2018 •

edited

Loading

noamyogev84 commented Dec 28, 2018 •

edited

Loading

noamyogev84 commented Jan 4, 2019 •

edited

Loading