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

"Links with identical accessible names have equivalent purpose" (b20e66): Prepare for TF review #1463

Merged
merged 9 commits into from Nov 2, 2020

Conversation

HelenBurge
Copy link
Collaborator

I would like my addition to passed example 11 to be confirmed, as was not clear to me... Despite reading the definitions (I tried to make it less technical for others like me!)

I might have over described what may seem obvious, to clarify the reason for passes/fails as per #1248

<< Describe the changes >>

Closes issue(s):

  • closes #XXX (ADD ISSUE NUMBER HERE)

Need for Final Call:
<< choose one of the following and remove the rest >>
<< check https://act-rules.github.io/pages/design/process/#final-call-aka-call-for-consensus-cfc >>
This can be merged with 1 approval << choose reason: editorial changes to website/test code, adding new contributor, other (explain). >>
This will not require a Final Call << choose reason(s): editorial changes, changes to assumptions, background, accessibility support, change to website/test code (not rule), other (explain). >>
This will require a 1 week Final Call << small changes affecting a small number of test cases, if in doubt do not use this. >>
This will require a 2 weeks Final Call << 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).

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 Final Call period. In case of disagreement, the longer period wins.

I would like my addition to passed example 11 to be confirmed, as was not clear to me... Despite reading the definitions (I tried to make it less technical for others like me!)

I might have over described what may seem obvious, to clarify the reason for passes/fails as per act-rules#1248
@CLAassistant
Copy link

CLAassistant commented Sep 28, 2020

CLA assistant check
All committers have signed the CLA.

@Jym77 Jym77 changed the title #1248-updates "Links with identical accessible names have equivalent purpose" (b20e66): Prepare for TF review Sep 29, 2020
Jym77
Jym77 previously requested changes Sep 29, 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 work 🎉 . A few "polishing" comments.

_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
@@ -332,7 +332,7 @@ Both links resolve to [same resource][] after redirect, but the redirect is not

#### Inapplicable Example 1

These `a` and `area` elements have no `href` attribute.
These `a` and `area` elements have no `href` attribute. These are not valid links.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
These `a` and `area` elements have no `href` attribute. These are not valid links.
These `a` and `area` elements have no `href` attribute. Thus, they do not have a role of `link`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure on this as the 'a' and 'area' are the role of link, but the 'href' is where it goes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the mappings: https://www.w3.org/TR/html-aam-1.0/#html-element-role-mappings (direct links to the elements don't work well… you need to scroll… 😞 ), the a and area elements have no role when there is no href attribute.
So, in that case, there is no element with a role of link (or inheriting from link), hence the rule is not applicable. I say, "role of link" is the technical term for "valid link" 😀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case I think we should amend it again, as the role side is still a bit technical, so maybe:
"These a and area elements have no href attribute. Thus, they do not have a role of link making the links not work as expected."

Copy link
Collaborator

Choose a reason for hiding this comment

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

"not work as expected" is a fairly strong statement, given that there are valid cases for a without href: https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element (see the second paragraph after the green box + the first example). So, it might be a deliberate choice of the author to omit the href attribute, and the element would work exactly as expected.

(+ see the example at https://html.spec.whatwg.org/multipage/image-maps.html#image-maps for an area without href where the lack of href is exactly what is intended, namely to make the "hole" in the red square not clickable)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So in plain text how would you define a link without a role?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, that's the point. It's not a link.
It's an a/area element without an href attribute and therefore not a link (role has little to do here, it's not a link for ARIA (no role), but it's not a link for HTML in the first place (not clickable, …)

Maybe "Thus they are not links and do not have a role of link"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes - that would work 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Jym77, can I update the last suggestion to the change to commit? Or is it something for you to change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do it.
I normally prefer to let one person handle all changes on a PR, so that there is clear responsibility (not in term of "who to blame?", but rather in avoiding the "who should do that?" and streamlining the process).

@Jym77
Copy link
Collaborator

Jym77 commented Sep 29, 2020

@HelenBurge : good work on your first PR 🥇

Some (meta)-tips about Github and how we use it for ACT rules (don't be scared, it is a lot of info I'm dumping here and it will take time to absorb all of it…)

  • I've updated the title of your PR to give full information: "[rule name]" ([rule id]): [what the PR does]. This helps figuring out the topic when browsing through the list of PRs.

  • I've added you as "Assignee" for it. This can help you at a later point filtering "your" PRs to follow work you have to do: https://github.com/act-rules/act-rules.github.io/pulls/assigned/HelenBurge (or, from the overview of all PRs, select the "Assignee" menu and check your name).

  • I've added some labels for the PRs ("Rule update" and "Reviewers wanted") to help classify it. Notably, the "Reviewer wanted" label is a general sign to anybody browsing PRs list that they can go get a look at it (versus something that is still work in progress).

  • I've requested reviews from some people (in the top-right menu). That way, these people will get an actual notification (mail, …) and you are more likely to get reviews. If in doubt, just ask from the people you recognise from the calls 😁

  • Can you update the PR description (the first message) to 1/ clean the "Need for Final Call" section, keeping "Will need a 1 week Final Call"; and 2/ replace the "closes XXX" part by "closes 2 links different destination same acc name is passing, should discuss [b20e66] #1248" (this will automatically close the issue once the rule is merged and ease our administrative work…)

  • After making changes, following review (or if you disagree with some comments), can you 1/ "Dismiss" the review (from the bottom box, the "change requested" menu will expand these reviews); and 2/ re-request the review (from the top-right list of reviewers, click the "recycle" arrow. This will help us (reviewers) track that work has been done and that we have to act again.

Sorry again for that much info in one go 🤓 👨‍🏫 It's good work. Don't worry about the administrative details…

Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Copy link
Collaborator Author

@HelenBurge HelenBurge left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback (I have not been able to do as much as suggested as only have basic permissions!) I like most except for the one talking about an href being the link role.

@@ -332,7 +332,7 @@ Both links resolve to [same resource][] after redirect, but the redirect is not

#### Inapplicable Example 1

These `a` and `area` elements have no `href` attribute.
These `a` and `area` elements have no `href` attribute. These are not valid links.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure on this as the 'a' and 'area' are the role of link, but the 'href' is where it goes.

Amended the rule "Inapplicable Example 1" from feedback
Jym77
Jym77 previously requested changes Oct 19, 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.

👍
Polishing the look.

_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
HelenBurge and others added 2 commits October 23, 2020 10:50
Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Copy link
Member

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

One more tiny change.

_rules/links-identical-name-equivalent-purpose-b20e66.md Outdated Show resolved Hide resolved
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
@WilcoFiers WilcoFiers merged commit 881953a into act-rules:develop Nov 2, 2020
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.

None yet

6 participants