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

fix: img with missing alt should have role img #430

Merged
merged 4 commits into from
Sep 21, 2020

Conversation

ckundo
Copy link
Contributor

@ckundo ckundo commented Sep 20, 2020

  • browsers will treat an image with missing alt as an image.
  • null alt is different than empty string: empty string removes the node from the AOM, where missing (null) alt does not.
  • this change may impact tests that incorrectly rely on missing alt and alt='' being equivalent.
  • recommend a patch level release.

[fixes #429]

- browsers will treat an image with missing alt as an image.
- null alt is different than empty string: empty string removes the node from the AOM, where missing (null) alt does not.
@changeset-bot
Copy link

changeset-bot bot commented Sep 20, 2020

🦋 Changeset is good to go

Latest commit: 4bc3983

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -89,6 +89,7 @@ const cases = [
["html", null, createElementFactory("html", {})],
["iframe", null, createElementFactory("iframe", {})],
["img with alt=\"some text\"", "img", createElementFactory("img", {alt: "text"})],
["img with missing alt", "img", createElementFactory("img", {})],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

per discussion #429 (comment) I think we agree on this behavior @eps1lon?

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect, thanks!

Copy link
Owner

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

You need to wrap the case in a block statement to avoid misleading block scoping:

case "img": { /* your code */ }

@@ -89,6 +89,7 @@ const cases = [
["html", null, createElementFactory("html", {})],
["iframe", null, createElementFactory("iframe", {})],
["img with alt=\"some text\"", "img", createElementFactory("img", {alt: "text"})],
["img with missing alt", "img", createElementFactory("img", {})],
Copy link
Owner

Choose a reason for hiding this comment

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

Perfect, thanks!

@eps1lon eps1lon changed the title img with missing alt should have role img fix: img with missing alt should have role img Sep 21, 2020
@eps1lon eps1lon merged commit 76e8f93 into eps1lon:main Sep 21, 2020
@eps1lon
Copy link
Owner

eps1lon commented Sep 21, 2020

@ckundo Much appreciated, thanks!

@ckundo ckundo deleted the ckundo/fix-img-missing-alt branch September 21, 2020 18:19
@github-actions github-actions bot mentioned this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

img elements with no alt attribute should maintain a role of img
2 participants