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

Update visible-label-in-accessible-name-2ee8b8.md #2101

Merged
merged 11 commits into from
Jan 11, 2024
Merged

Conversation

kengdoj
Copy link
Collaborator

@kengdoj kengdoj commented Aug 21, 2023

Update examples to include #1458 (comment)
Closes issue(s):

  • closes #XXX (ADD ISSUE NUMBER HERE)

Need for Call for Review:

This will require a 2 weeks Call for Review << new rule, or substantial changes affecting a large number of test cases, if in doubt, use this. >>


Pull Request Etiquette

When creating PR:

  • Make sure you're requesting to pull a branch (right side) to the develop branch (left side).
  • Make sure you do not remove the "How to Review and Approve" section in your pull request description

After creating PR:

  • Add yourself (and co-authors) as "Assignees" for PR.
  • Add label to indicate if it's a Rule, Definition or Chore.
  • Link the PR to any issue it solves. This will be done automatically by referencing the issue at the top of this comment in the indicated place.
  • Optionally request feedback from anyone in particular by assigning them as "Reviewers".

When merging a PR:

  • Close any issue that the PR resolves. This will happen automatically upon merging if the PR was correctly linked to the issue, e.g. by referencing the issue at the top of this comment.

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Call for Review period. In case of disagreement, the longer period wins.

@kengdoj kengdoj marked this pull request as draft August 21, 2023 23:40
Jym77
Jym77 previously approved these changes Sep 28, 2023
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.

How do the changes relate to #2075?


```html
<button aria-label="close">X</button>
<button aria-label="anything">X</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we keep the "Close" label, which makes this pass 1.1.1?
(assuming this is indeed a close button)

Copy link
Collaborator Author

@kengdoj kengdoj Oct 30, 2023

Choose a reason for hiding this comment

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

Because the Expectation says

except for characters in the [text nodes][] used to express [non-text content][]

there is no expectation, and this example passes. I used aria-label="anything" to make it clear that really any description would pass this rule. But, you are right that SC 1.1.1 would then be not satisfied. But I'd like to consider SC 1.1.1 being secondary and handle that in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comes up often around here: when passed examples fail something else.

I like it best when there is a note on the passed example which documents that "something else". In this case that note could be: "This example passes the rule and will fail SC 1.1.1".

We could call these "curve ball passed examples".

To have examples like these, and to document them as such, meets two goals, both important:

  • It emphasizes why the example passes the rule. (In this case: "to make it clear that really any description would pass this rule", as Kathy put it). I think that this aids understanding, a lot.
  • It tells everyone that even though this is a "passed example" for this rule, it's not a good example overall, so no one should copy and paste the code.

Jean-Yves did this a lot in the examples for his recent PR 2084:

  • "Using an explicit role of generic for any element is globally not recommended, but not an error by itself."
  • "Using an explicit role which is the same as the implicit role is not recommended, but not an error by itself."
  • "The btn token is not a valid role and is therefore ignored."
  • "Even though the alert role is not allowed, the first token is a valid role and is therefore used."

I think it's a good thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works, but perhaps a cleaner way to handle this would be to modify the second condition of the applicability to filter out not just non-visible text content, but also non-text content. That would cause this example to move to the non-applicable section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, Tom.

But, Applicability has to be objective at this time, so the except non-text content part is under the Expectation section for now.

_rules/visible-label-in-accessible-name-2ee8b8.md Outdated Show resolved Hide resolved

```html
<button aria-label="close">X</button>
<button aria-label="anything">X</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comes up often around here: when passed examples fail something else.

I like it best when there is a note on the passed example which documents that "something else". In this case that note could be: "This example passes the rule and will fail SC 1.1.1".

We could call these "curve ball passed examples".

To have examples like these, and to document them as such, meets two goals, both important:

  • It emphasizes why the example passes the rule. (In this case: "to make it clear that really any description would pass this rule", as Kathy put it). I think that this aids understanding, a lot.
  • It tells everyone that even though this is a "passed example" for this rule, it's not a good example overall, so no one should copy and paste the code.

Jean-Yves did this a lot in the examples for his recent PR 2084:

  • "Using an explicit role of generic for any element is globally not recommended, but not an error by itself."
  • "Using an explicit role which is the same as the implicit role is not recommended, but not an error by itself."
  • "The btn token is not a valid role and is therefore ignored."
  • "Even though the alert role is not allowed, the first token is a valid role and is therefore used."

I think it's a good thing

@kengdoj kengdoj marked this pull request as ready for review November 28, 2023 23:34
Copy link
Collaborator

@dd8 dd8 left a comment

Choose a reason for hiding this comment

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

The updated rules references the #matching-characters glossary definition which says:

A sequence of characters is considered to match another if, after removing leading and trailing space characters and replacing remaining occurrences of one or more space characters with a single space, the two sequences of characters are equal character-by-character, ignoring any differences in letter casing

The link to https://html.spec.whatwg.org/#white_space in the #matching-characters definition is broken so it's not clear which definition of space characters is intended. The various WHATWG specs have moved to a common definition of ASCII whitespace defined here:
https://infra.spec.whatwg.org/#ascii-whitespace

This is different to the Unicode whitespace definition the rule currently uses:
https://www.w3.org/WAI/standards-guidelines/act/rules/2ee8b8/proposed/#whitespace

Notably ASCII whitespace doesn't include &nbsp; which would be problematic for this example from @dan-tripp-siteimprove

<a aria-label="compose email" href="#">Compose &nbsp;&nbsp;<br> email</a>

Desired behaviour: pass this rule

@kengdoj
Copy link
Collaborator Author

kengdoj commented Dec 1, 2023

Thank you @dd8 for catching that.

I've opened #2144 to update the matching characters definition.

@Jym77 Jym77 added the Review call 2 weeks Call for review for new rules and big changes label Dec 7, 2023
@Jym77
Copy link
Collaborator

Jym77 commented Dec 7, 2023

Call for Review ends on December 21st.

@Jym77 Jym77 removed the Review call 2 weeks Call for review for new rules and big changes label Dec 7, 2023
@Jym77 Jym77 dismissed their stale review December 7, 2023 14:22

Same org already approved it.

@Jym77
Copy link
Collaborator

Jym77 commented Dec 7, 2023

Removing from Call for Review as two reviewers were from the same organisation…

This link has [visible][] text does not match the [accessible name][] because there are extra spaces in the accessible name.

```html
<a aria-label="Call 1 2 3. 4 5 6. 7 8 9 0." href="tel:1234567890">123.456.7890</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the inclusion of a full stop at the end of the aria-label attribute value deliberate? If so, do we need to provide clarification regarding its potential impact on the outcome? In the issue description only extra spaces are mentioned.

Copy link
Collaborator Author

@kengdoj kengdoj Dec 12, 2023

Choose a reason for hiding this comment

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

thanks! I edited the aria-label.

@kengdoj kengdoj requested review from giacomo-petri and dd8 December 12, 2023 19:17
@tombrunet
Copy link
Collaborator

Added a comment regarding Pass 5. It feels to me like this scenario should be considered inapplicable, but that would require a modification to the applicability. The rule is consistent as-is, but it seems more accurate to say that the non-text content scenario is not applicable to this rule rather than to say that it passes it...

@Jym77 Jym77 merged commit 1db4aaf into develop Jan 11, 2024
2 checks passed
@Jym77 Jym77 deleted the kengdoj-labelinname branch January 11, 2024 15:10
dan-tripp-siteimprove added a commit to dan-tripp-siteimprove/act-rules.github.io that referenced this pull request Feb 7, 2024
- being more specific with regards to whitespace and &nbsp;.  as per dd8's comment at act-rules#2101 (review)
- misc. other edits.
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.