Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

img-uses-alt does not allow empty string or possible empty string #6

Closed
lencioni opened this issue Apr 5, 2016 · 13 comments
Closed
Assignees

Comments

@lencioni
Copy link
Contributor

lencioni commented Apr 5, 2016

This rule considers the following JSX bad:

function Foo() {
  return <img alt={foo || ''} />;
}

this is also considered bad:

function Foo() {
  return <img alt="" />;
}

but this is okay:

function Foo() {
  return <img alt={foo} />;
}

However, I think that all three should be okay. Empty strings can be used appropriately for alt text on images that are decorative.

@beefancohen
Copy link
Contributor

I agree on the first one, since we can't determine the value of foo until runtime. However, not sure about the second - You can place a space in between the quotation marks (<img alt=" " />) and it should pass and still semantically represent the same thing to a screen reader. Also, I can implement case where alt="" (or any other form of undefined value) passes lint rule if role=presentation is present.

@lencioni
Copy link
Contributor Author

lencioni commented Apr 5, 2016

I think the role=presentation bit makes sense. According to the spec, it should probably enforce alt="" if it has a presentation role.

Authors SHOULD NOT provide meaningful alternative text (for example, use alt="" in HTML4) when the presentation role is applied to an image.

https://www.w3.org/TR/wai-aria/roles#presentation

@beefancohen
Copy link
Contributor

Fixing first example in 0.5.3 - will upgrade minor on role=presentation enhancement. 0.5.3 should be done within the hour.

@lencioni
Copy link
Contributor Author

lencioni commented Apr 5, 2016

Awesome! Thanks again!

beefancohen pushed a commit that referenced this issue Apr 5, 2016
Fixes #8
Fixes #9
Fixes first example in #6
@beefancohen
Copy link
Contributor

0.5.3 published - should fix first use case + other bugs that are closed!

@lencioni
Copy link
Contributor Author

lencioni commented Apr 6, 2016

I think this is still broken for cases like

function Foo() {
  return <img alt={foo.bar || ''} />;
}

and

function Foo() {
  return <img alt={bar() || ''} />;
}

and

function Foo() {
  return <img alt={foo.bar() || ''} />;
}

beefancohen pushed a commit that referenced this issue Apr 6, 2016
@beefancohen
Copy link
Contributor

Added test cases for those and fixed in v0.5.4 - still may be other edge cases, working on resolving cases to handle each type specified in spec

@lencioni
Copy link
Contributor Author

lencioni commented Apr 6, 2016

Wonderful!

@lencioni
Copy link
Contributor Author

lencioni commented Apr 6, 2016

@evcohen do you have an ETA on the role="presentation" change? No rush--I'm just wondering if I should roll with alt=" " or wait it out.

lencioni added a commit to lencioni/eslint-plugin-jsx-a11y that referenced this issue Apr 6, 2016
This rule was giving the same error message for two different types of
errors. This could lead to confusion, so I decided to make the error
messages more descriptive.

When jsx-eslint#6 is resolved, the empty alt value message should probably be
expanded to provide guidance about using `role="presentation"`.
@beefancohen
Copy link
Contributor

@lencioni waiting for ci build to pass and then publishing v0.6.0. Error message updated and this strictly allows only the following scenario <img alt="" role="presentation" />

bad:

<img alt={``} role="presentation" /> etc. as we only want to deal with literals for this case.

@lencioni
Copy link
Contributor Author

lencioni commented Apr 7, 2016

I noticed that the Chrome audit rules allows alt="" without role="presentation" and role="presentation" without alt="", FYI: https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules#ax_text_02

@beefancohen
Copy link
Contributor

Use the attributes alt="", role="presentation" or include the image as a CSS background-image to identify it as being used purely for stylistic or decorative purposes and that it should be ignored by people using assistive technologies.
Source: http://fae20.cita.illinois.edu/rule/ARIA_STRICT/IMAGE_2/

Not sure what the real rule is in this case, but as a linter, I think it's better to be opinionated in a case like this or give users configuration options to manage the rigidity of the rule. My thinking is, the only time alt can be undefined (not just ="", but literally non-existent) is when role="presentation". In this sense, we can drop the check for alt altogether if role="presentation" is present. Thoughts?

@lencioni
Copy link
Contributor Author

lencioni commented Apr 7, 2016

My reading of the text you posted agrees with the Chrome text I lined to, and it also fits my intuitive understanding. I think it makes sense to enforce the existence of alt unless role="presentation", and if role="presentation" enforce either non-existent or empty alt.

lencioni referenced this issue in lencioni/javascript Apr 8, 2016
After digging into this rule a little more with @evcohen, I believe that
it is okay for images to have an empty string for alt text. I expect the
next release of the linter rule to allow for this as well.

More context:

  https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/6
lencioni added a commit to lencioni/eslint-plugin-jsx-a11y that referenced this issue Apr 8, 2016
As we discussed on jsx-eslint#6, it is okay to use `alt=""` without
`role="presentation"`. It is possible that we want to enforce no alt
prop if `role="presentation"` but I'll leave that for another time and
possibly a different rule.
lencioni added a commit to lencioni/eslint-plugin-jsx-a11y that referenced this issue Apr 8, 2016
As we discussed on jsx-eslint#6, it is okay to use `alt=""` without
`role="presentation"`. It is possible that we want to enforce no alt
prop if `role="presentation"` but I'll leave that for another time and
possibly a different rule.
beefancohen pushed a commit that referenced this issue Apr 8, 2016
* Allow alt="" or role="presentation"

As we discussed on #6, it is okay to use `alt=""` without
`role="presentation"`. It is possible that we want to enforce no alt
prop if `role="presentation"` but I'll leave that for another time and
possibly a different rule.

* Slightly simplify img-uses-alt error tests

We don't really get much value out of having two ways to check for
errors. Let's just use the one function everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants