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

Rule: ARIA required context role (ff89c9) #255

Merged
merged 74 commits into from
Nov 25, 2019

Conversation

annethyme
Copy link
Collaborator

@annethyme annethyme commented Sep 14, 2018

Closes #194, closes #302

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”.

@annethyme annethyme added the Rule Use this label for a new rule that does not exist already label Sep 14, 2018
@annethyme annethyme self-assigned this Sep 14, 2018
@kensgists kensgists self-requested a review September 20, 2018 15:12
@WilcoFiers WilcoFiers requested review from kensgists and removed request for kensgists September 20, 2018 15:12
_rules/SC1-3-1-aria-required-context-role.md Outdated Show resolved Hide resolved
_rules/SC1-3-1-aria-required-context-role.md Outdated Show resolved Hide resolved
_rules/SC1-3-1-aria-required-context-role.md Outdated Show resolved Hide resolved
_rules/SC1-3-1-aria-required-context-role.md Outdated Show resolved Hide resolved
_rules/SC1-3-1-aria-required-context-role.md Outdated Show resolved Hide resolved
nitedog
nitedog previously requested changes Oct 2, 2018
Copy link
Collaborator

@nitedog nitedog left a comment

Choose a reason for hiding this comment

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

Mostly there but agree with Wilco on trying to simplify some of the language

@annethyme annethyme dismissed nitedog’s stale review October 3, 2018 19:10

Updated. Please review again.

@annethyme annethyme dismissed WilcoFiers’s stale review October 3, 2018 19:12

See replies to comments.

@WilcoFiers WilcoFiers changed the title [WIP] Rule: ARIA required context role (ff89c9) rule: ARIA required context role (ff89c9) Nov 1, 2019
@WilcoFiers WilcoFiers dismissed stale reviews from carlosapaduarte, ShadowBB, and jeeyyy November 1, 2019 12:01

Review has been updated

@Jym77 Jym77 assigned WilcoFiers and unassigned Jym77 and jeeyyy Nov 7, 2019
pages/glossary/owned-by.md Show resolved Hide resolved
@WilcoFiers WilcoFiers added Review call 2 weeks Call for review for new rules and big changes and removed reviewers wanted labels Nov 11, 2019
Jym77
Jym77 previously requested changes Nov 12, 2019
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.

  • Many editorial changes in polishing the description of examples.
  • A maybe naive question about the applicability to implicit roles (I must miss some point here…)
  • A real concern about the limitation of "owned by" to children rather than descendants.

_rules/aria-required-context-role-ff89c9.md Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Outdated Show resolved Hide resolved

#### Passed Example 1

Element with [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, expressed as an [explicit semantic role](#explicit-role).
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
Element with [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, expressed as an [explicit semantic role](#explicit-role).
The `div` element with an [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, expressed as an [explicit semantic role](#explicit-role).

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we at least go for

Suggested change
Element with [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, expressed as an [explicit semantic role](#explicit-role).
The element with an [explicit semantic role](#explicit-role) of `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) of `list`, expressed as an [explicit semantic role](#explicit-role).

in order to make it a proper English sentence.


#### Passed Example 2

Element with [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, through the [implicit semantic role](#implicit-role) of `ul`.
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
Element with [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, through the [implicit semantic role](#implicit-role) of `ul`.
The `div` element with an [explicit semantic role](#explicit-role) `listitem` is contained within its [required context role](https://www.w3.org/TR/wai-aria-1.1/#scope) `list`, through the [implicit semantic role](#implicit-role) of the `ul` element.


#### Inapplicable Example 4

The `listitem` has an [explicit semantic role](#explicit-role), but it is identical to the [implicit semantic role](#implicit-role), making the element inapplicable.
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
The `listitem` has an [explicit semantic role](#explicit-role), but it is identical to the [implicit semantic role](#implicit-role), making the element inapplicable.
The `li` element has an [explicit semantic role](#explicit-role), but it is identical to its [implicit semantic role](#implicit-role), making the element inapplicable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd still use

Suggested change
The `listitem` has an [explicit semantic role](#explicit-role), but it is identical to the [implicit semantic role](#implicit-role), making the element inapplicable.
The `listitem` has an [explicit semantic role](#explicit-role), but it is identical to its [implicit semantic role](#implicit-role), making the element inapplicable.

_rules/aria-required-context-role-ff89c9.md Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Show resolved Hide resolved
_rules/aria-required-context-role-ff89c9.md Show resolved Hide resolved
pages/glossary/owned-by.md Show resolved Hide resolved
@jeeyyy jeeyyy changed the title rule: ARIA required context role (ff89c9) Rule: ARIA required context role (ff89c9) Nov 12, 2019
@WilcoFiers WilcoFiers dismissed Jym77’s stale review November 13, 2019 10:40

Resolved everything I think needs resolving, left comments for the rest of it.

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.

Mostly happy with changes. I'm still a bit in the dark about this:
#255 (comment)

@WilcoFiers WilcoFiers merged commit 5d33cbd into develop Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review call 2 weeks Call for review for new rules and big changes Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

owned elements definition SC1-3-1-aria-required-context-role