Skip to content

Conversation

dd8
Copy link
Collaborator

@dd8 dd8 commented Mar 5, 2020

Closes issue(s):
#1124
#1125

Need for Final Call:
This will require a 2 weeks Final Call due to substantial changes affecting most test cases

This PR:

  • restricts the applicability to elements that display an image (to make <img/> inapplicable because it has no visual rendering, and is not voiced)
  • removes `img alt=':-)' as a passed example because it fails 1.1.1
  • changes role=img to role=figure in Inapplicable Example 1 because the example description is "The element does not have the semantic role of img"
  • adds img src or background-image to most examples
  • adds <img/> as an inapplicable example
  • adds additional failed examples like <img srcset='...'/>

@dd8
Copy link
Collaborator Author

dd8 commented Mar 5, 2020

I don't have write access to the repo, so I can't:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.

@Jym77 Jym77 assigned dd8 Mar 5, 2020
Copy link
Collaborator

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

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

Good job.

## Applicability

The rule applies to HTML `img` elements or any HTML element with the [semantic role][] of `img` that is [included in the accessibility tree][].
The rule applies to HTML `img` elements or any HTML element with the [semantic role][] of `img` that is [included in the accessibility tree][] and displays an image.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "displays an image" need to be clarified.
For example <img src="this.url.does.not.exists" /> arguably does not display an image, but I think this is not what you intend to rule out.

Copy link
Collaborator Author

@dd8 dd8 Mar 5, 2020

Choose a reason for hiding this comment

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

How about changing:

and displays an image

to

and renders an image or an omitted image indicator

based on the rendering description here https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element:attr-img-src-5

Is that description precise enough?

The definition above requires a name for a missing URL specified by HTML, but not for a missing CSS background-image (which seems right because users don't know the CSS image is missing, unless they look at DevTools console)

A couple of options that won't work:

  1. Using the represents nothing definition from HTML. I don't think that works if there's a CSS background-image and no src.

  2. Using the valid non-empty URL potentially surrounded by spaces definition from HTML. That doesn't work for inline SVG.

Copy link
Member

@WilcoFiers WilcoFiers Mar 6, 2020

Choose a reason for hiding this comment

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

How about this?

it's [current request][]'s [state][image request state] is not [completely available][]; or

I used it here: https://github.com/act-rules/act-rules.github.io/pull/1213/files#diff-f7f0c7b0159f5cafa41a1983a6d9389aR30

@Jym77 Jym77 added reviewers wanted Rule Update Use this label for an existing rule that is being updated labels Mar 5, 2020
Copy link
Collaborator

@adilsofficial adilsofficial left a comment

Choose a reason for hiding this comment

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

Good work 👍 just a minor changes.


```html
<img />
<img src="/test-assets/shared/w3c-logo.png" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Althought this code is considered img element with an empty accessibile name but I guess we can improve this example by adding alt attribute. What do you think?

Suggested change
<img src="/test-assets/shared/w3c-logo.png" />
<img src="/test-assets/shared/w3c-logo.png" alt=""/>

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that would break the example, as alt="" would make the image decorative, which we don't want for this.

Copy link
Collaborator

@daniel-montalvo daniel-montalvo left a comment

Choose a reason for hiding this comment

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

Good job! I do support changes proposed by @WilcoFiers here.

@dd8
Copy link
Collaborator Author

dd8 commented Mar 27, 2020

I'll take a look at the changes requested when time allows - probably in a couple of weeks. Other stuff taking priority at the moment due to Covid-19

@kasperisager kasperisager removed their request for review April 24, 2020 09:53
@dd8
Copy link
Collaborator Author

dd8 commented Jul 7, 2020

Getting back to this - this rule has changed a lot since the original PR in March (including the rule being renamed, and some of the changes from this PR were picked up in #1300).

I think it might be clearer to open a new PR and refer back to this one (for requested changes). Anyone disagree?

@Jym77
Copy link
Collaborator

Jym77 commented Jul 8, 2020

I think it might be clearer to open a new PR and refer back to this one (for requested changes). Anyone disagree?

Do it. It will be much easier for everybody (especially for you).

@dd8 dd8 mentioned this pull request Jul 8, 2020
6 tasks
@dd8
Copy link
Collaborator Author

dd8 commented Jul 8, 2020

Yep, makes sense, new PR is #1382

I don't have rights to assign reviewers, so can someone do that?

@WilcoFiers
Copy link
Member

Handled in #1382, closing this PR.

@WilcoFiers WilcoFiers closed this Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewers wanted Rule Update Use this label for an existing rule that is being updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants