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

Do not generate default alt text for images #30213

Merged
merged 1 commit into from Aug 23, 2017

Conversation

@ckundo
Copy link
Contributor

@ckundo ckundo commented Aug 12, 2017

Summary

  • Deprecate image_alt helper.
  • Remove the call to image_alt from image_tag.

[Fixes #30096]

Impact

  • After this change, screen readers will fallback to default, expected, and user configurable behavior for missing alt text.
  • Also with this change, automated linting and testing tools will correctly generate warnings.

Motivation

  • Auto-generating content from the filename of an image is not suitable alternative text.
  • Alt text that isn't fully considered can be distracting and fatiguing for screen readers users (blind, low vision, dyslexic people).
  • Setting a filename fallback short circuits screen reader default behavior and configuration for blank descriptions, and may be frustrating to the assistive technology user.
  • Setting poor defaults also creates false negatives for linting tools, that makes it harder to improve application accessibility.
@rails-bot
Copy link

@rails-bot rails-bot commented Aug 12, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Loading

@ckundo
Copy link
Contributor Author

@ckundo ckundo commented Aug 12, 2017

I'm working on fixing up test failures now, but the gist of the PR is the same.

Loading

@ckundo
Copy link
Contributor Author

@ckundo ckundo commented Aug 12, 2017

Something to reiterate from the PR comment: this change will create warnings in some accessibility testing tools. These would be correct failures, but new failures nonetheless. In some cases these warnings could potentially be part of CI suites, and, in an even more narrow subset of applications, may create build failures.

Is it worth introducing a configuration option to disable the correct behavior for folks that may need to fix failures when upgrading Rails? The alternative for those that want to delay fixes would be to monkey-patch the image_tag helper in their apps until they remediate the issue. @mgifford how did y'all handle this in Drupal?

Loading

Copy link
Member

@rafaelfranca rafaelfranca left a comment

It looks good to me. Nice work. Can you add a CHANGELOG entry?

Loading

@@ -286,6 +282,8 @@ def image_tag(source, options = {})
# image_alt('underscored_file_name.png')
# # => Underscored file name
def image_alt(src)
::ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.")
Copy link
Member

@rafaelfranca rafaelfranca Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the first :: here.

Loading

@@ -286,6 +282,8 @@ def image_tag(source, options = {})
# image_alt('underscored_file_name.png')
# # => Underscored file name
def image_alt(src)
ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.")
Copy link
Contributor

@georgeclaghorn georgeclaghorn Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/AssesTagHelper/AssetsTagHelper, s/exlicitly/explicitly

Loading

Copy link
Member

@rafaelfranca rafaelfranca Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to talk about the module name too. Only image_alt is deprecated is good. And it is going to be removed from Rails 6.0.

Loading

@@ -268,6 +262,8 @@ def image_tag(source, options = {})
tag("img", options)
end

# This method will be deprecated in future versions of Rails.
Copy link
Contributor

@georgeclaghorn georgeclaghorn Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is deprecated now. It’ll be removed in Rails 5.2.

Loading

Copy link
Member

@kaspth kaspth Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's deprecated in 5.2 and will be removed in 6.0.

Loading

Copy link
Member

@kaspth kaspth Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though we let the deprecation warning itself suffice, so need for a comment here.

Loading

Copy link
Contributor

@georgeclaghorn georgeclaghorn Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the comment says this method will eventually be deprecated when it’s actually deprecated in this PR.

The deprecation warning says this method will be removed in 5.2; it should say 6.0.

Loading

@@ -286,6 +282,8 @@ def image_tag(source, options = {})
# image_alt('underscored_file_name.png')
# # => Underscored file name
def image_alt(src)
ActiveSupport::Deprecation.warn("ActionView::Helpers::AssesTagHelper.image_alt is deprecated and will be removed from Rails 5.2. You must exlicitly set alt text on images.")
Copy link
Member

@rafaelfranca rafaelfranca Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to talk about the module name too. Only image_alt is deprecated is good. And it is going to be removed from Rails 6.0.

Loading

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Looks good to me. Can you squash your commits?

Loading

@ckundo ckundo force-pushed the ccundiff-alt-text-default branch 2 times, most recently from 75153dc to c469879 Aug 16, 2017
unless src.start_with?("cid:") || src.start_with?("data:") || src.blank?
options[:alt] = options.fetch(:alt) { image_alt(src) }
end
options[:src] = path_to_image(source, skip_pipeline: skip_pipeline)
Copy link
Member

@maclover7 maclover7 Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing the method being used to generate src? Using path_to_image instead of resolve_image_source is causing CI to fail 😬

Loading

@@ -1,3 +1,9 @@
* Remove default `alt` text generation

Fixes #30096
Copy link
Member

@maclover7 maclover7 Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a period after the issue number

Loading

@ckundo
Copy link
Contributor Author

@ckundo ckundo commented Aug 17, 2017

Thanks @maclover7, fixed. Good to squash?

Loading

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 17, 2017

👍 squash again

Loading

- Auto-generating content from the filename of an image is not suitable
  alternative text; alt text that isn't fully considered can be
  distracting and fatiguing for screen readers users (blind, low vision,
  dyslexic people).
- Setting a filename fallback short circuits screen reader default
  behavior and configuration for blank descriptions.
- Setting poor defaults also creates false negatives for accessibility
  linting and testing software, that makes it harder to improve
  application accessibility.

***

- After this change, if authors leave images without alt text, screen
  readers will fallback to default behavior for missing alt text.
- Also with this change, Automated linting and testing tools will
  correctly generate warnings.

[Fixes rails#30096]
@ckundo ckundo force-pushed the ccundiff-alt-text-default branch from c66f5ab to 9a74c7b Aug 17, 2017
@ckundo
Copy link
Contributor Author

@ckundo ckundo commented Aug 21, 2017

Squashed and ready!

Loading

@ckundo
Copy link
Contributor Author

@ckundo ckundo commented Aug 23, 2017

@rafaelfranca anything else I can add?

Loading

@rafaelfranca rafaelfranca merged commit 9a74c7b into rails:master Aug 23, 2017
2 checks passed
Loading
rafaelfranca added a commit that referenced this issue Aug 23, 2017
Do not generate default alt text for images
y-yagi added a commit to y-yagi/rails that referenced this issue Aug 23, 2017
astronaut-wannabe added a commit to crimethinc/website that referenced this issue Apr 17, 2018
The `image_tag` helper no longer generates a default alt tag from the filename.

The reasoning is explained in [rails PR #30213][0]

Since we don't have custom `alt` tags written up for the book images, I just
removed the `alt` tags from the spec expectations

[0]:rails/rails#30213
jmuheim added a commit to jmuheim/base that referenced this issue Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants