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

feat(aria): allow DPUB ARIA roles #584

Closed
wants to merge 1 commit into from

Conversation

rdeltour
Copy link
Contributor

This PR adds DPUB ARIA roles to the lookup table (+ integration tests).

DPUB ARIA roles can be used in HTML (not restricted to EPUB or XHTML), it's in the final stages of Rec track and mappings are reasonably implemented (see the implementation report for DPUB AAM).

This PR is of course awaiting decision on #580 (I'm in touch with Wilco), but submitted to have a common understanding on the proposed changes.

Closes #580

@WilcoFiers
Copy link
Contributor

After our discussion at TPAC I had another look at these. Most of these seem fine. They work progressively. Even if they aren't supported, having them in the document won't harm accessibility. The ones that need some refinement are the ones that don't:

The link roles:

doc-noteref, doc-backlink, doc-biblioref, doc-glossref and doc-noteref

I think these need to either be fully supported in all our platform (which I don't expect), or we need to add a rule that requires a fallback for when they're not supported. (e.g. allow them only on <a href="">).

doc-biblioentry / doc-cover

I think a similar thing applies here. Either these need to be supported, or we require that they are used on elements that fall back to something that works (<img> / <li>).

Thoughts?

@WilcoFiers WilcoFiers added waiting on feedback/tests needs discussion More discussion is needed to continue and removed waiting on feedback/tests labels Nov 15, 2017
@WilcoFiers
Copy link
Contributor

@rdeltour Any updates / thoughts on my previous comment?

@rdeltour
Copy link
Contributor Author

rdeltour commented Dec 1, 2017

@WilcoFiers sorry for the silence. Yes I believe that limiting the elements on which these roles are allowed makes a lot of sense until it gets wider implemented.

I can have a look at the implem. Do you have an idea when you'd like to have this or when you want to publish the next release?

@WilcoFiers
Copy link
Contributor

@rdeltour Thanks! Yes that would be excellent. Please follow the rule proposal process. I don't want to compound on this PR, so I suggest we create these as three separate rules with separate PRs. Once those are in we can merge this.

As for timeline, we're going to release axe-core 2.6 within the next two weeks, so that's probably too short notice. The nearest planned release after that is 3.0, which we're aiming at February. So if you can have these in by January, that's perfect.

@rdeltour
Copy link
Contributor Author

rdeltour commented Dec 1, 2017

I don't want to compound on this PR, so I suggest we create these as three separate rules with separate PRs.

Just to make sure we're one the same page, these 3 rules would be:

  • one rule for the link-extending roles
  • one rule for the listitem-extending roles
  • one rule for the img-extending role

Correct?

Also, these rules would rely on the DPUB ARIA roles to be defined, so I'd base them on this PR branch, alright?

@WilcoFiers
Copy link
Contributor

Sounds good

@WilcoFiers WilcoFiers added waiting on feedback/tests and removed needs discussion More discussion is needed to continue labels Dec 4, 2017
@marcysutton
Copy link
Contributor

Just checking up on this PR. Are you planning to add anything to this one?

@rdeltour
Copy link
Contributor Author

Are you planning to add anything to this one?

Yes, maybe.
As suggested by @WilcoFiers, I would spawn 3 other PRs for related rules. This one PR might be affected, not entirely sure yet.

Wilco and I have been in touch offline. I'm intending to work on it in early Feb (can't find the time in Jan), hoping to get it ready to possibly integrate it in 3.0.

@rdeltour
Copy link
Contributor Author

rdeltour commented Feb 14, 2018

hey @WilcoFiers. I’m having a fresh look at this PR. Rather than the 3 new rules envisioned above, what about only one new rule that more generically checks that the element’s implicit role is equal to the role’s superclass role?

Here’s the (incomplete) description for the proposed new rule:

  • name: aria-roles-fallback
  • desc: "Ensures unsupported ARIA roles are only used on elements with an implicit fallback role"
  • selector: [role], then JS matcher function to filter the roles that needs to be checked (like doc-backlink, doc-biblioentry, doc-cover, etc).
  • (single) check: has-implicit-role-fallback (any)
    1. get the element’s role value
    2. get the superclass role of the element’s role (the type fields in aXe’s role lookup table)
    3. returns true if the element’s implicit role is equal to the superclass role.

WDYT?

Also, another potentially acceptable fallback mechanism is to define more than one roles (narrower roles first, fallbacks after). But this depends on #724.
This could be an alternative "any" check in this rule.

@WilcoFiers
Copy link
Contributor

@rdeltour The reason I suggested multiple rules is so that you can turn them off individually. I don't see how we could do that if they are part of a single rule. Matchers aren't configurable unfortunately, so I'm not sure how that can be solved without an architectural change (which I don't think we should do, at least not this close to the 3.0 release).

As for fallback roles. I very much support that idea. If you can get the AT testing done to verify that none of the AT in our baseline will just ignore the role attribute all together if the first one is unknown, I'd love to have that as an option.

@rdeltour
Copy link
Contributor Author

The reason I suggested multiple rules is so that you can turn them off individually. I don't see how we could do that if they are part of a single rule

Right, but I’m not sure there’s a use case for enabling one of these 3 and not the others? The idea is that role values would be removed from the matcher once they have reasonable support, and that could safely wait for a dot release.

The extreme solution is to have one rule per role that requires a fallback, but I don’t think we’d want that either :-)

@WilcoFiers
Copy link
Contributor

Maybe, maybe not... I've seen stranger things (it's a good show). I'd be alright with it if we make this a DPUB specific rule. So like a dpub-role-fallback, that limits it and allows us to do other stuff for other rules.

@rdeltour
Copy link
Contributor Author

So like a dpub-role-fallback, that limits it and allows us to do other stuff for other rules

OK. The check would be generic anyway, so could be easily used in other context (or in custom per-role rules).

@WilcoFiers
Copy link
Contributor

@rdeltour How's the new rule coming along? We're aiming for the release in 2 weeks, so if we want to include this rule and get it localised we've got another week at most. Will you be able to put together a pull request by then?

@rdeltour
Copy link
Contributor Author

rdeltour commented Mar 2, 2018

How's the new rule coming along?

Proposed in #759 (nothing like a hard deadline to get things done 😅)

@WilcoFiers
Copy link
Contributor

This is replaced by #759, yes? If so, please close this PR.

@rdeltour
Copy link
Contributor Author

rdeltour commented Mar 4, 2018

This is replaced by #759, yes? If so, please close this PR.

Yeah, I based #759 off that one, so it does include the commit for this one. I wasn’t sure if you wanted to merge in two steps, or all at once in #759.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants