search

daisy / ace

Ace by DAISY, an Accessibility Checker for EPUB
MIT License
75 stars 22 forks source link

fix: detect role=heading for outline #309

Closed pkra closed 4 years ago

pkra commented 4 years ago

Enables ace-extraction's getHeadings() to detect headings marked via role=heading; uses aria-level or the spec fallback.

Fixes #308

pkra commented 4 years ago

@rdeltour is this roughly the right direction?

rdeltour commented 4 years ago

@rdeltour is this roughly the right direction?

yes! 👍

pkra commented 4 years ago

Thanks, @rdeltour. Should I check aria-level values more carefully (e.g., non-negative) or will they be checked elsewhere anyway?

rdeltour commented 4 years ago

Should I check aria-level values more carefully (e.g., non-negative)

yes, I think all the values that are not conforming (0 or negative values) should be reported as heading level 2 (default value).

Ideally we should test this in browser+AT combinations to have a better idea how this is processed in real life, but until then sticking to the spec looks reasonable.

will they be checked elsewhere anyway?

Apparently it's not checked by Axe (see dequelabs/axe-core#1288). I don't think it's checked by EPUBCheck (to be verified).

pkra commented 4 years ago

@rdeltour I added a check for positive integers (also avoiding +'Infinity' becoming Infinity).

pkra commented 4 years ago

@rdeltour will you let me know if you need anything else from me?

rdeltour commented 4 years ago

@rdeltour will you let me know if you need anything else from me?

no, it all looks good 👍. I'm just a little overloaded these days 😅 I'll do a final review and merge it ASAP!

pkra commented 4 years ago

Thanks @rdeltour. No rush from my end, I just wanted to check before I lose track.

rdeltour commented 4 years ago

@danielweck can this be included in the upcoming 1.2.0 version?

danielweck commented 4 years ago

I propose that we merge this PR into the master branch, as originally intended. This may or may not be followed by an official release update (version 1.1.2, see https://github.com/daisy/ace/releases ) ... to be discussed.

Then I will (as usual) "backport" codebase changes from master into the ace-next branch (version 1.2.x beta). I am not sure about code conflicts yet, let's see...

danielweck commented 4 years ago

The master unit tests pass: https://travis-ci.org/github/daisy/ace/builds/672176684

danielweck commented 4 years ago

The ace-next unit tests pass too: https://travis-ci.org/github/daisy/ace/builds/672176804

danielweck commented 4 years ago

Thank you very much @pkra your contribution is much appreciated.

Note that the ace-next branch (Ace 1.2 beta) contains many updated NPM packages, including the Axe dependency. I will likely soon publish an NPM update version 1.2.0-beta.7 to the next package tag ( https://www.npmjs.com/package/@daisy/ace ), including this PR fix.

The ace-next branch is used to build the Ace desktop app ( https://github.com/daisy/ace-gui ) for which a release update is expected soon. The next version of Ace App will therefore include this PR fix too.