Skip to content

Remove multiple inheritance from menuitemcheckbox and menuitemradio #1278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 25, 2020
Merged

Remove multiple inheritance from menuitemcheckbox and menuitemradio #1278

merged 6 commits into from
Jun 25, 2020

Conversation

@carmacleod
Copy link
Contributor

@carmacleod carmacleod commented May 22, 2020 •
edited
Loading

NOTE: June 13, 2020
For some reason pr-preview did not run, so the Preview and Diff are old (tooltip says "Last updated on May 26, 2020"). Please just look at the code changes in Files changed - there are only 3 changes (2 deletions and an addition).

Closes #1277

  • removes checkbox from menuitemcheckbox superclasses (so it only inherits from menuitem)
  • removes radio from menuitemradio superclasses (so it only inherits from menuitemcheckbox)
  • requires aria-checked on menuitemcheckbox (which menuitemradio inherits)

Preview | Diff

@carmacleod carmacleod requested review from jnurthen and joanmarie May 22, 2020 23:22
@carmacleod carmacleod added this to the ARIA 1.2 milestone May 22, 2020
@jnurthen
Copy link
Member

jnurthen commented May 23, 2020

I'd prefer to solve this by changing the inheritance of menuitemcheckbox.
To be honest the other states and properties menuitemcheckbox (and eventually menuitemradio) inherit make no sense either:
aria-errormessage, aria-invalid, aria-readonly and aria-required

@jnurthen
Copy link
Member

jnurthen commented May 23, 2020

I'm going to tag the underlying issue for the agenda and if we don't have agreement through github we can discuss on Thursday.

@carmacleod carmacleod linked an issue May 23, 2020 that may be closed by this pull request
menuitemradio/menuitemcheckbox shouldn't support aria-expanded. Maybe radio should? #1277
Closed
@carmacleod
Copy link
Contributor Author

carmacleod commented May 23, 2020 •
edited
Loading

@jnurthen

I'd prefer to solve this by changing the inheritance of menuitemcheckbox.
To be honest the other states and properties menuitemcheckbox (and eventually menuitemradio) inherit make no sense either:
aria-errormessage, aria-invalid, aria-readonly and aria-required

That's brilliant, and I agree completely, and I will remove checkbox from menuitemcheckbox's superclasses in this PR (and add aria-checked to the supported required attrs). However, I think menuitemcheckbox should still inherit from menuitem, so it doesn't solve the aria-expanded problem.

(Also, hmmm... maybe aria-haspopup should be prohibited on menuitemcheckbox/menuitemradio...).

@carmacleod carmacleod changed the title Prohibit aria-expanded on menuitemcheckbox and menuitemradio, Prohibit aria-expanded on menuitemcheckbox and menuitemradio, support aria-expanded on radio May 23, 2020
@carmacleod
menuitemcheckbox: remove checkbox from superclasses,
a703ce8 
add aria-checked to required attributes
@carmacleod
Copy link
Contributor Author

carmacleod commented May 25, 2020

I guess radio should be removed from menuitemradio's superclasses to correspond to removing checkbox as a superclass of menuitemcheckbox.

@carmacleod carmacleod added the WIP label May 26, 2020
@carmacleod
Copy link
Contributor Author

carmacleod commented May 26, 2020 •
edited
Loading

This PR:

  • Removes checkbox from menuitemcheckbox superclasses (so it only inherits from menuitem)
  • Removes radio from menuitemradio superclasses (so it only inherits from menuitemcheckbox)
  • Adds aria-checked as required for menuitemcheckbox (which is then inherited by menuitemradio)
  • Prohibits aria-expanded and aria-haspopup on menuitemcheckbox and menuitemradio
  • Fixes up the javascript so that prohibited attributes (i.e. aria-expanded and aria-haspopup) don't say they are used in or inherit into roles they are prohibited on (i.e. menuitemcheckbox and menuitemradio)
  • Adds support for aria-expanded to radio

Since menuitemcheckbox and menuitemradio no longer inherit from an input (i.e. checkbox or radio),
they no longer support aria-errormessage, aria-invalid (2 of the 4 deprecated globals), aria-readonly or aria-required.

Ready for discussion and review.

@carmacleod
Copy link
Contributor Author

carmacleod commented May 28, 2020 •
edited
Loading

From the call:

  • don't support aria-expanded on radio
  • do support aria-expanded on menuitemradio and menuitemcheckbox
  • do remove the multiple inheritance (i.e. remove checkbox from menuitemcheckbox superclasses, and remove radio from menuitemradio superclasses)

Also, I will move the javascript fixes for prohibited attributes to a PR in aria-common so that the code is not lost.

@carmacleod carmacleod mentioned this pull request Jun 13, 2020
Closed
@carmacleod
Revert "remove prohibited attributes from "used in roles" and "inheri…
3ac2a32 
…ts into roles" lists"

This reverts commit 3f845d5
@carmacleod
don't allow aria-expanded on radio
10a1ba5 
allow aria-expanded and aria-haspopup on menuitemcheckbox and menuitemradio
@carmacleod carmacleod changed the title Prohibit aria-expanded on menuitemcheckbox and menuitemradio, support aria-expanded on radio Remove multiple inheritance from menuitemcheckbox and menuitemradio Jun 13, 2020
@carmacleod
Copy link
Contributor Author

carmacleod commented Jun 13, 2020

Ok, I've made the changes that were discussed on the call, removed the WIP label, and renamed this PR to reflect what it contains now:

  • removes the multiple inheritance from menuitemcheckbox and menuitemradio, i.e.
    • removes checkbox from menuitemcheckbox superclasses (so it only inherits from menuitem)
    • removes radio from menuitemradio superclasses (so it only inherits from menuitemcheckbox)
    • requires aria-checked on menuitemcheckbox (which menuitemradio inherits)

This has the desired side effect of removing support for aria-errormessage, aria-invalid, aria-readonly and aria-required from menuitemcheckbox and menuitemradio.

I also moved the javascript to support prohibited non-global attributes to w3c/aria-common#46 to be used later if we need it.

This PR is ready for review.

@carmacleod carmacleod removed the WIP label Jun 13, 2020
@jnurthen jnurthen removed the request for review from joanmarie June 18, 2020 17:20
@scottaohara scottaohara self-requested a review June 18, 2020 17:20
@jnurthen jnurthen requested review from jongund and sinabahram June 18, 2020 17:21
@jnurthen jnurthen merged commit 1d856c4 into master Jun 25, 2020
@jnurthen jnurthen mentioned this pull request Jul 6, 2020
Merged
@WilcoFiers WilcoFiers mentioned this pull request Jul 7, 2020
Closed
@jnurthen jnurthen deleted the issue1277 branch July 23, 2020 22:18
jnurthen pushed a commit that referenced this pull request Sep 9, 2020
@carmacleod @jnurthen
Remove multiple inheritance from menuitemcheckbox and menuitemradio (#…
284a71d 
…1278)

* menuitemcheckbox: remove checkbox from superclasses,
add aria-checked to required attributes
* remove radio from menuitemradio superclasses,
@36degrees 36degrees mentioned this pull request Sep 9, 2020
Closed
@carmacleod carmacleod mentioned this pull request Oct 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cookiecrook cookiecrook cookiecrook approved these changes

@jongund jongund jongund approved these changes

@sinabahram sinabahram sinabahram approved these changes

@scottaohara scottaohara scottaohara approved these changes

@jnurthen jnurthen Awaiting requested review from jnurthen

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ARIA 1.2

Development

Successfully merging this pull request may close these issues.

menuitemradio/menuitemcheckbox shouldn't support aria-expanded. Maybe radio should?

7 participants

@carmacleod @jnurthen @cookiecrook @jongund @sinabahram @scottaohara