Check for missing alt attributes on img tags #70

ckundo · 2015-03-13T22:46:37Z

This introduces a check that aligns with the W3C WCAG 2.0 guideline 1.1.1, which states:

All non-text content that is presented to the user has a text alternative that serves the equivalent purpose

http://www.w3.org/TR/2008/REC-WCAG20-20081211/#text-equiv-all

[closes #69]

sds · 2015-03-13T23:44:23Z

lib/haml_lint/linter/alt_text.rb

+    include LinterRegistry
+
+    def visit_tag(node)
+      if node.tag_name == 'img' && !node.has_hash_attribute?(:alt)


Are img tags the only tags that are applicable here? That seems surprising.

This technique is specifically related to img tags (http://www.w3.org/TR/2014/NOTE-WCAG20-TECHS-20140916/H37). There are other requirements to meet the guideline I linked to in the PR, but this is an easy test that moves towards that goal. I can make the guideline name more specific, like ImgAltText if that's more clear.

The name is fine. I guess I was just expecting a list of different tags, so code more along the lines of:

TAGS_REQUIRING_ALT = %w[img ...] ... if TAGS_REQUIRING_ALT.include?(node.tag_name) && ... ... end

@sds the only tag that explicitly requires the attribute is img. area tags require them when using an href, and applet tags don't need them if they have fallback content in the tag body.

sds · 2015-03-13T23:48:53Z

Thanks for the pull request, @ckundo.

I've left some feedback inline. Main concern is around the use of eval, which will work only for the simplest of cases.

ckundo · 2015-03-15T16:08:41Z

lib/haml_lint/tree/tag_node.rb

+    # @return [true,false]
+    def has_hash_attribute?(attribute)
+      hash_attributes? &&
+        hash_attributes_source.scan(/(#{attribute})\W/).any?


@sds the parser gem does not give access to the hash keys as nodes, so I went with a regex. I'll refine this matcher, but is it an ok direction from your standpoint?

I don't think I understand what you mean by the parser gem not giving access to hash keys as nodes. If you look at the implementation at https://github.com/causes/haml-lint/blob/master/lib/haml_lint/linter/class_attribute_with_static_value.rb#L43 you can see we're checking to see if a key is a string or symbol.

Yep, you're right I misunderstood. I've updated the commit.

- Introduces a new method to check if an attribute exists in a tag nodes hash attributes - Required for followup commit to check for missing alt attributes [sds#69]

- alternative text is required for non text content - http://www.w3.org/TR/2008/REC-WCAG20-20081211/#text-equiv-all [sds#69]

@sds

- Per comment by @sds, users must explicitly enable accessibility rules. [Finishes sds#69]

ckundo · 2015-03-16T18:12:47Z

lib/haml_lint/tree/tag_node.rb

+    private
+
+    def parsed_attributes
+      HamlLint::RubyParser.new.parse(hash_attributes_source)


Seems like there can be some additional modeling/extraction done around the parser, but I wasn't sure how far to take it. Open to suggestions.

There is a parse_ruby helper defined on the Linter class which you can use instead.

@sds saw that, though it's a private method. I can promote it to public.

also, the parse_ruby method is an instance method on Linter. Should I create a corresponding class method? It seems like adding code to the Linter class for the sake of TagNode is a smell. Should they be interacting in that way? What about putting that helper in a module?

Ah, I forgot we were in the TagNode. Sounds like we should define a parse_ruby helper on the Node class, since I see this being something more heavily used in node helpers going forward.

How about a module/mixin? That would avoid code duplication.

Yup, that works.

Hi @sds, I spiked on creating a mixin but it felt a little forced. It's less code to reuse the class here, and it's easier to test the parser object as a class vs module.

What do you think about keeping this line as is? I think that's the only thing blocking this PR.

Here's the spike I was talking about. Do you think this a better approach? ckundo@f74eaf6

sds · 2015-03-28T19:19:37Z

Merged in fe37a4c with some minor adjustments in 275fcab. Thanks!

sds reviewed Mar 13, 2015
View reviewed changes

sds added the enhancement label Mar 13, 2015

ckundo force-pushed the cc-alt-attributes branch from 1466667 to 2f0e714 Compare March 15, 2015 16:01

ckundo reviewed Mar 15, 2015
View reviewed changes

ckundo force-pushed the cc-alt-attributes branch from 2f0e714 to ed0bdec Compare March 15, 2015 21:38

Cameron Cundiff added 3 commits March 16, 2015 13:59

Add method to check attribute presence.

9904ff4

- Introduces a new method to check if an attribute exists in a tag nodes hash attributes - Required for followup commit to check for missing alt attributes [sds#69]

Add linter for missing img alt attributes.

40e67f9

- alternative text is required for non text content - http://www.w3.org/TR/2008/REC-WCAG20-20081211/#text-equiv-all [sds#69]

Make alt text linter disabled by default.

e22d5a3

- Per comment by @sds, users must explicitly enable accessibility rules. [Finishes sds#69]

ckundo force-pushed the cc-alt-attributes branch from ed0bdec to e22d5a3 Compare March 16, 2015 18:00

ckundo reviewed Mar 16, 2015
View reviewed changes

sds closed this Mar 28, 2015

sds mentioned this pull request Mar 28, 2015

accessibility rules - check for missing alt attributes #69

Closed

ckundo deleted the cc-alt-attributes branch March 28, 2015 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for missing alt attributes on img tags #70

Check for missing alt attributes on img tags #70

ckundo commented Mar 13, 2015

sds Mar 13, 2015

ckundo Mar 15, 2015

sds Mar 16, 2015

ckundo Mar 16, 2015

sds commented Mar 13, 2015

ckundo Mar 15, 2015

sds Mar 16, 2015

ckundo Mar 16, 2015

ckundo Mar 16, 2015

sds Mar 16, 2015

ckundo Mar 16, 2015

ckundo Mar 16, 2015

sds Mar 16, 2015

ckundo Mar 17, 2015

sds Mar 17, 2015

ckundo Mar 27, 2015

ckundo Mar 27, 2015

sds commented Mar 28, 2015

Check for missing alt attributes on img tags #70

Check for missing alt attributes on img tags #70

Conversation

ckundo commented Mar 13, 2015

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

sds commented Mar 13, 2015

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

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

sds commented Mar 28, 2015