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

remove 4 Global States and properties (#999) #1023

Merged
merged 11 commits into from
Oct 22, 2019
Merged

Conversation

jnurthen
Copy link
Member

@jnurthen jnurthen commented Jul 31, 2019

closes #999


Preview | Diff

@jnurthen jnurthen added the WIP label Jul 31, 2019
@carmacleod
Copy link
Contributor

carmacleod commented Aug 1, 2019

Summary of changes:

Removes the following attributes from global attributes:

  • aria-disabled
  • aria-errormessage
  • aria-haspopup
  • aria-invalid

Adds the above as supported attributes on the following roles (and their subclasses):

  • aria-disabled

    • button
    • composite
    • gridcell
    • input
    • menuitem
    • row
    • scrollbar
    • separator
    • tab
  • aria-errormessage

    • checkbox
    • combobox
    • gridcell
    • listbox
    • radiogroup
    • slider
    • textbox
    • tree
  • aria-haspopup

    • button
    • combobox
    • gridcell
    • menuitem
    • slider
    • textbox
    • treeitem
  • aria-invalid

    • checkbox
    • combobox
    • gridcell
    • listbox
    • radiogroup
    • slider
    • textbox
    • tree

@jnurthen jnurthen marked this pull request as ready for review October 8, 2019 18:06
@jnurthen jnurthen removed the WIP label Oct 8, 2019
@jnurthen jnurthen changed the title [WIP] Initial version for #999 remove 4 Global States and properties (#999) Oct 9, 2019
Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

Here's the only things that jumped out at me:
menubar allows for aria-disabled. being a related concept to menubar, I think toolbar should for aria-disabled as well.

I question if menuitemcheckbox and menuitemradio should allow for aria-errormessage or aria-invalid.

@jnurthen
Copy link
Member Author

jnurthen commented Oct 9, 2019

Here's the only things that jumped out at me:
menubar allows for aria-disabled. being a related concept to menubar, I think toolbar should for aria-disabled as well.

Happy to add that

I question if menuitemcheckbox and menuitemradio should allow for aria-errormessage or aria-invalid.

I think I agree but I can't change that in this PR due to the inheritance model. This PR does not make things worse as they already allow these attributes. Are you ok making this change and potentially tackling this in 1.3?

@scottaohara
Copy link
Member

scottaohara commented Oct 9, 2019

Are you ok making this change and potentially tackling this in 1.3?

yes if you remove the word "potentially" from that statement :D

because making changes where we can is far better than making no changes at all

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

per indicated updates / concessions changing review status to "approve"

@carmacleod
Copy link
Contributor

I question if menuitemcheckbox and menuitemradio should allow for aria-errormessage or aria-invalid.

I think I agree but I can't change that in this PR due to the inheritance model.

Does it make sense to list aria-errormessage and aria-invalid in "Prohibited States and Properties:" section for menuitemcheckbox and menuitemradio? That way, the inheritance model doesn't have to change.

The one that felt weird to me is aria-haspopup on a slider? So I googled it, and discovered that whatsock recommends using aria-haspopup for a context menu. Not sure if this advice is old, or still valid? If valid, then aria-haspopup may need to stay global?

Also want to note that if a group is representing a fieldset, then it can be disabled. But in other contexts, it can't. Not quite sure how to handle that.

One final note... not sure why Level Access tests aria-haspopup on links and radios?
Hmmm... looking a bit further, it looks like we might have to allow aria-haspopup on links because people seem to be creating dropdown navs like this a11y-101 nested nav example and this ingresso-bootstrap dropdown example.

@scottaohara
Copy link
Member

@carmacleod thanks for making me think some more :)

Does it make sense to list aria-errormessage and aria-invalid in "Prohibited States and Properties:" section for menuitemcheckbox and menuitemradio? That way, the inheritance model doesn't have to change.

That might be a reasonable approach, yet at the same time I feel a bit of apprehension about doing it so as to avoid changing other things. If others think this is an ok thing to do, then sure... but I also think it's just as reasonable to leave as is and then tackle in 1.3.

The one that felt weird to me is aria-haspopup on a slider?

I also went back and forth on whether a slider should have an aria-haspopup but I also don't have a strong argument for or against, so imo better to stay with current allowances here.

So I googled it, and discovered that whatsock recommends using aria-haspopup for a context menu. Not sure if this advice is old, or still valid? If valid, then aria-haspopup may need to stay global?

ARIA 1.0 defines aria-haspopup as:

Indicates that the element has a popup context menu or sub-level menu...

"context menu" is absent from the definition as of ARIA 1.1. While that does not indicate it can't be one, the definition now includes:

For the popup element to be keyboard accessible, authors SHOULD ensure that the element that can trigger the popup is focusable, that there is a keyboard mechanism for opening the popup

Per the updated definition it seems to me that the focusable expectation precludes this attribute from being global.

One final note... not sure why Level Access tests aria-haspopup on links and radios?

Level Access has a bunch of tests for ARIA support. Being a global attribute it's reasonable for there to be tests for it. I'm sure I have some more buried somewhere as well :)

Hmmm... looking a bit further, it looks like we might have to allow aria-haspopup on links because people seem to be creating dropdown navs like this a11y-101 nested nav example and this ingresso-bootstrap dropdown example.

Regarding those examples, I'd submit that improper use of the attribute does not indicate that it should be continued to be allowed. If anything I'd point to those examples as evidence of author confusion about the attribute, and why it shouldn't be allowed on links to help mitigate misuse.

Also want to note that if a group is representing a fieldset, then it can be disabled. But in other contexts, it can't. Not quite sure how to handle that.

Yeh, I was also thinking about that... the flexible nature of the group role does make that a bit weird, but if we kept aria-disabled as allowed on group it would continue to keep parity with a group being used as a stand-in for fieldset, and again would be no worse off than it presently is for other potentially dubious usage. @jnurthen I think it does make sense to continue to allow aria-disabled on group if possible.

@carmacleod
Copy link
Contributor

@scottaohara Thanks for the thoughtful responses.

If others think this is an ok thing to do, then sure... but I also think it's just as reasonable to leave as is and then tackle in 1.3.

👍

I also went back and forth on whether a slider should have an aria-haspopup but I also don't have a strong argument for or against, so imo better to stay with current allowances here.

👍

"context menu" is absent from the definition as of ARIA 1.1.

I'd like to ask the team what the history of this is.

Per the updated definition it seems to me that the focusable expectation precludes this attribute from being global.

tabindex="0" on any element? Anyhow, first need to determine whether aria-haspopup is needed for context menu, or not.

If anything I'd point to those examples as evidence of author confusion about the attribute, and why it shouldn't be allowed on links to help mitigate misuse.

👍

if we kept aria-disabled as allowed on group it would continue to keep parity with a group being used as a stand-in for fieldset

👍

@accdc
Copy link
Contributor

accdc commented Oct 10, 2019

Personally, I'm opposed to removing aria-disabled, specifically from role=link as we discussed today on the call.

We have current clients using similar implementations as outlined below, which is a valid use of aria-disabled.

There is a web portal that includes multiple components, each of which has specific actions that can be applied to each region. These include "Edit", "Remove", "Move", and so on.

Each of these action elements are not standard active elements, but clickable icons made of span tags plus relevant visual styling.

To ensure keyboard accessibility and screen reader accessibility, they also include tabindex="0" and role="link", plus relevant scripting so that keyboard behavior matches mouse click functionality.

These use role="link" instead of role="button", because there is a submit button at the end of the form that needs to be easily navigable by pressing "b" using relevant desktop screen readers, or in the swipe order for such controls on mobile devices, and the use of role="button" for all of these action elements would interfere with the user’s ability to do so without having to go through every such control prior to that button. Thus role="link" is preferable for this purpose instead.

Moreover, based on certain conditions, not all of these action elements are enabled at the same time, so they are sometimes disabled and sometimes not. They are visually styled to convey this disabled state to sighted users, but there also needs to be a way for screen reader users to receive the same information. So, aria-disabled="true" is dynamically added to the relevant role="link" elements that are thus effected for this purpose.

If support for aria-disabled is removed from role="link", then it will be impossible for screen reader users to receive the same level of information as sighted users, which defeats the intended purpose of ARIA.

@scottaohara
Copy link
Member

scottaohara commented Oct 14, 2019

If support for aria-disabled is removed from role="link", then it will be impossible for screen reader users to receive the same level of information as sighted users, which defeats the intended purpose of ARIA.

This is not an accurate statement. Buttons, which can be disabled, could have been used here and would have allowed screen reader users to receive the same level of information as sighted users. However, as noted they were not used in an effort to simplify the UX for screen reader users navigating by button elements. The two issues (while both important) should not be conflated.

That said, given the choice to continue to allow the attribute on role=link for 1.2, or to impede other progress this PR would make, I'll go with progress. I suggest we have a grounded discussion about this again for attribute parity in 1.3, and for now note that while aria-disabled may be used on a role=link, it is not recommended to be used on <a href> for the reasons I stated on the call, and then some.

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

requesting a note be added to indicate that while allowed with role=link use on / disabling a host language equivalent may not be allowed.

@stes-acc
Copy link

stes-acc commented Oct 15, 2019

As a matter of fact, the discussion for the host language is pretty old:
see e.g. https://discourse.wicg.io/t/adding-a-disabled-attribute-to-a-s/1116/2

@accdc
Copy link
Contributor

accdc commented Oct 15, 2019

"That said, given the choice to continue to allow the attribute on role=link for 1.2, or to impede other progress this PR would make, I'll go with progress.
I suggest we have a grounded discussion about this again for attribute parity in 1.3, and for now note that while aria-disabled may be used on a role=link,
it is not recommended to be used on for the reasons I stated on the call, and then some."

What has been concerning me for a while now, is that there seems to be a strong push to make all ARIA usage synchronous with HTML, which is being justified by the role parity project. ARIA is not designed to be only used with HTML.

@scottaohara
Copy link
Member

scottaohara commented Oct 15, 2019

I understand that ARIA is not for use with just HTML. It is, however, designed to be in conformance with the allowances of the host language. Hence my request to note that "disabling a host language equivalent may not be allowed"

@carmacleod
Copy link
Contributor

Here is a new summary of the changes this PR would introduce as of October 11:

Removes the following 4 attributes from global attributes:

  • aria-disabled
  • aria-errormessage
  • aria-haspopup
  • aria-invalid

Adds the above attributes back as supported attributes on certain roles (and their subclasses), as follows:

  • aria-disabled

    • application
    • button
    • composite
    • gridcell
    • group
    • input
    • link
    • menuitem
    • row * see note, below
    • scrollbar
    • separator
    • tab
    • toolbar * see note, below
  • aria-errormessage

    • application
    • checkbox
    • combobox
    • gridcell
    • listbox
    • radiogroup
    • slider
    • textbox
    • tree
  • aria-haspopup

    • application
    • button
    • combobox
    • gridcell
    • menuitem
    • slider
    • tab
    • textbox
    • treeitem
  • aria-invalid

    • application
    • checkbox
    • combobox
    • gridcell
    • listbox
    • radiogroup
    • slider
    • textbox
    • tree

NOTE: Now that group supports aria-disabled, row, select, and toolbar don't need to explicitly support it because they are subclasses of group and will inherit aria-disabled.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Remove aria-disabled from "Supported States and Properties" for row, select, and toolbar, and ensure that it shows up in "Inherited States and Properties" list.

index.html Outdated Show resolved Hide resolved
@stes-acc
Copy link

We definitely should address things like

<a href="[href]" aria-disabled="true">

without explicit role="link" declaration with respect to what HTML browsers should map in these cases.

However, this is part of the larger discussion "using native roles + aria properties without explicit aria role declaration".

In my opinion, for the aforementioned example browsers should just map disabled attribute to accessibility API as there were a role defined. Explicitly disabling will happen by author JS/CSS code or /and native element href manipulation.

@carmacleod
Copy link
Contributor

@stes-acc

We definitely should address things like <a href="[href]" aria-disabled="true">...

Please move this comment to #1099 where we will discuss "disabled links" (and their related mappings, if any) in the ARIA 1.3 time frame.

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Looks good!

@accdc
Copy link
Contributor

accdc commented Oct 17, 2019

"What else is being removed here that you take issue with?"

There isn't, the link role was what concerned me since we have clients using this right now based on prior support.

Also, one of our clients has a software-based web UI development platform that has some major restrictions in what their developers are allowed to do within that framework. One of these restrictions is that they cannot change the href attributes on any links, and the platform does support disabling links, which are styled differently and do nothing when clicked. This platform does allow for the addition of supporting attributes however, so aria-disabled was added to make these links equally accessible for non-sighted screen reader users. Moreover, these changes were made as part of a lawsuit, and if this suddenly stopped working it would put the client back in legal jeopardy.

If this attribute is not removed from link, then I support the pull.

@stes-acc
Copy link

"What else is being removed here that you take issue with?"

There isn't, the link role was what concerned me since we have clients using this right now based on prior support.

Also, one of our clients has a software-based web UI development platform that has some major restrictions in what their developers are allowed to do within that framework. One of these restrictions is that they cannot change the href attributes on any links, and the platform does support disabling links, which are styled differently and do nothing when clicked. This platform does allow for the addition of supporting attributes however, so aria-disabled was added to make these links equally accessible for non-sighted screen reader users. Moreover, these changes were made as part of a lawsuit, and if this suddenly stopped working it would put the client back in legal jeopardy.

If this attribute is not removed from link, then I support the pull.

+1

@LJWatson
Copy link
Contributor

I realise that support for -disabled on elements with role="link" isn't going to be dropped for the moment, but it worries me that the rationale for not dropping it is apparently based on a single use case - and a use case that is, I think, not a particularly strong one.

@accdc wrote:

There is a web portal that includes multiple components, each of which has specific actions that can be applied to each region. These include "Edit", "Remove", "Move", and so on.>

These use role="link" instead of role="button", because there is a submit button at the end of the form that needs to be easily navigable by pressing "b" using relevant desktop screen readers, or in the swipe order for such controls on mobile devices, and the use of role="button" for all of these action elements would interfere with the user’s ability to do so without having to go through every such control prior to that button. Thus role="link" is preferable for this purpose instead.>

Arguably these should be buttons, given that they appear to invoke an action on the current interface, rather than fetch a resource. Making them links might ease screen reader navigation to the submit button if that's the intended action, but it equally makes navigation to these links harder if one of them is the intended action.

We push developers to code semantically and to use the appropriate HTML (code) for the purpose, but this approach suggests it's ok not to do that when it suits accessibility, and that makes it hard to keep insisting that it's important at all.

I think we need strong use cases for making changes, or deciding not to make changes, to standards, and that those use cases need to be backed up by a reasonable amount of evidence to support their validity in the wild.

@accdc
Copy link
Contributor

accdc commented Oct 18, 2019

"I realise that support for -disabled on elements with role="link" isn't going to be dropped for the moment, but it worries me that the rationale for not dropping it is apparently based on a single use case - and a use case that is, I think, not a particularly strong one."

I don't understand this, because people are already doing this.

E.G

Microsoft: Turn on or off links in email messages - Outlook
https://support.office.com/en-us/article/turn-on-or-off-links-in-email-messages-2d79b907-93b6-4774-82e6-1f0385cf20f8
"If the Junk Email Filter doesn’t consider a message to be spam but does consider it to be phishing, the message is left in the Inbox, but any links in the message are disabled and you can’t use the Reply and Reply All commands."

How to disable a link using only CSS? - GeeksforGeeks
https://www.geeksforgeeks.org/how-to-disable-a-link-using-only-css/

People have already been doing this in the wild for years.

I don't understand this reasoning that, because links can't be disabled in HTML, nobody is allowed to have disable links anywhere. The reason being, that people are already implementing disabled links, they are just not accessible because they do not convey their disabled state to non-sighted screen reader users.

So what I keep hearing here, is that blind people shouldn't have a need to know when a link is disabled, because it doesn't match HTML. We had the same discussion when it was proposed to remove support for aria-checked on role=option, because it doesn't match HTML.

I disagree with that. I have every right as a blind person to get the same meaning from a control as a sighted person does visually, and right now, for links, using aria-disabled is the only option available for doing this.

As I mentioned before, it is fine to add guidance to the APG to discourage people from doing this as a general practice. However, it is not okay to remove the ability to do this from all technologies everywhere just because some people don't agree with it.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

There are 3 things I think need to be addressed before merging:

  1. Make support for aria-disabled on row conditional on the row being in a grid or treegrid.
  2. Since aria-errormessage and aria-invalid are supported by slider, make them also supported on spinbutton.
  3. In the preview, aria-errormessage is not showing that it is inherrited into any roles. Shouldn't it be inherrited by searchbox? Interestingly, aria-invalid is showing as inherrited into 7 roles. It should be possible to use aria-errormessage on any element that supports aria-invalid. Maybe this is a script bug?

Less important: the abstract role select is listed as inherrited for aria-disabled. Is that a bug in the script propagating inherited states and properties?

@jnurthen
Copy link
Member Author

jnurthen commented Oct 22, 2019

Make support for aria-disabled on row conditional on the row being in a grid or treegrid.

Logged #1101 . As these were previously global this is not adding support here. This can be addressed in 1.3

@jnurthen
Copy link
Member Author

jnurthen commented Oct 22, 2019

Since aria-errormessage and aria-invalid are supported by slider, make them also supported on spinbutton.

resolved

@jnurthen
Copy link
Member Author

In the preview, aria-errormessage is not showing that it is inherrited into any roles. Shouldn't it be inherrited by searchbox? Interestingly, aria-invalid is showing as inherrited into 7 roles. It should be possible to use aria-errormessage on any element that supports aria-invalid. Maybe this is a script bug?

Was missing the placeholder section in the property

@jnurthen
Copy link
Member Author

Less important: the abstract role select is listed as inherrited for aria-disabled. Is that a bug in the script propagating inherited states and properties?

no - aria-disabled is on group so inherits to select and all of its child roles.

@jnurthen jnurthen merged commit 5b971a1 into master Oct 22, 2019
jnurthen added a commit that referenced this pull request Oct 25, 2019
Add Editors Note stating that implementations for this feature are still pending.

Removes the following 4 attributes from global attributes:
* aria-disabled
* aria-errormessage
* aria-haspopup
* aria-invalid

Adds the above attributes back as supported attributes on certain roles (and their subclasses), as follows:

aria-disabled

application
button
composite
gridcell
group
input
link
menuitem
scrollbar
separator
tab

aria-errormessage

application
checkbox
combobox
gridcell
listbox
radiogroup
slider
spinbutton
textbox
tree

aria-haspopup

application
button
combobox
gridcell
menuitem
slider
tab
textbox
treeitem

aria-invalid

application
checkbox
combobox
gridcell
listbox
radiogroup
slider
spinbutton
textbox
tree
carmacleod pushed a commit that referenced this pull request May 7, 2020
Removes the following 4 attributes from global attributes:
* aria-disabled
* aria-errormessage
* aria-haspopup
* aria-invalid

Adds the above attributes back as supported attributes on certain roles (and their subclasses), as follows:

aria-disabled

application
button
composite
gridcell
group
input
link
menuitem
scrollbar
separator
tab

aria-errormessage

application
checkbox
combobox
gridcell
listbox
radiogroup
slider
spinbutton
textbox
tree

aria-haspopup

application
button
combobox
gridcell
menuitem
slider
tab
textbox
treeitem

aria-invalid

application
checkbox
combobox
gridcell
listbox
radiogroup
slider
spinbutton
textbox
tree
@jnurthen jnurthen deleted the GlobalStatesAndProperties branch July 23, 2020 22:19
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.

Revising what ARIA attributes should be considered global
8 participants