1343537 - implement ARIA DPUB extension

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[alexander :surkov (:asurkov)]

spec link: https://www.w3.org/TR/dpub-aria-1.0/
mapping link: https://rawgit.com/w3c/aria/master/dpub-aam/dpub-aam.html#mapping_role_table
ia2 thread on mapping: https://lists.linuxfoundation.org/pipermail/accessibility-ia2/2016-September/002192.html

From Joanie's email:
"As you may be aware, the Digital Publishing ARIA Task Force is trying to
transition its Digital Publishing Accessibility API Mappings spec to
Candidate Recommendation.  The CfC to request transition did pass, and
the request was submitted. However, the PowersThatBe™ indicated that our
stated exit criteria was not sufficiently quantifiable and suggested
inclusion of "each role is successfully mapped in at least two
implementations.""

webkit implemented it:
https://bugs.webkit.org/show_bug.cgi?id=168978

any objections/comments about spec/implementation/timing/anything?

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #0)

> any objections/comments about spec/implementation/timing/anything?

Unfortunately we can't work on this while we have higher priorities, especially: e10s a11y is not ready/usable.

What dos this work entail?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #1)
> (In reply to alexander :surkov from comment #0)
> 
> > any objections/comments about spec/implementation/timing/anything?
> 
> Unfortunately we can't work on this while we have higher priorities,
> especially: e10s a11y is not ready/usable.

It's not that kind of work to have any noticeable impact on e10s a11y or anything else I believe, since it's just ARIA role map extension. So it shouldn't be a big deal if that is your only one concern.

> What dos this work entail?

Probably it will make web a little more accessible, and DPUB group can move forward with the spec. I cannot think of any implications for Gecko; the performance of adding a coupe of dozens roles is unaffected since the role search complexity o(log n) number is not really changed.

Comment 3

[James Craig]

David, this is mainly for EPUB and other types of online publications. It required no new WebCore roles, so the WebKit implementation was merely a list of role aliases. The rest of the patch is automated tests.

https://bugs.webkit.org/attachment.cgi?id=303043&action=prettypatch

Comment 4

[James Craig]

Better link: https://trac.webkit.org/changeset/213235/trunk/Source/WebCore/accessibility/AccessibilityObject.cpp

Comment 5

[alexander :surkov (:asurkov)]

Setting myself as a mentor. Here's two easy steps to implement the new roles:

1) add DPUB roles from [1] to ARIAMap.cpp [2]. Refer to RoleMap.h [3] to ensure that cross-platform roles were choosen appropriately
2) add test_dpub.html test file similar to role/test_aria.html [3]

[1] https://rawgit.com/w3c/aria/master/dpub-aam/dpub-aam.html#mapping_role_table
[2] https://dxr.mozilla.org/mozilla-central/source/accessible/base/ARIAMap.cpp?q=ARIAMap.cpp&redirect_type=direct#38
[3] https://dxr.mozilla.org/mozilla-central/source/accessible/base/RoleMap.h?q=RoleMap.h&redirect_type=direct
[4] https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/role/test_aria.html

Comment 6

[Joanmarie Diggs]

Attachment: proposed patch

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8876670 [details] [diff] [review]
proposed patch

Review of attachment 8876670 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thank you so much for doing it!

::: accessible/atk/Platform.cpp
@@ +172,3 @@
>          atkMinorVersion = strtol(endPtr + 1, &endPtr, 10);
> +        if (*endPtr == '.')
> +          atkMicroVersion = strtol(endPtr + 1, &endPtr, 10);

per [1]:
"If the str is empty or does not have the expected form, no conversion is performed, and (if str_end is not NULL) the value of str is stored in the object pointed to by str_end."

which means it reuturns endPtr unchanged for no conversion case. I think it'd be safer to have a check for atkMajor/MinorVersion != 0L.

[1] http://en.cppreference.com/w/cpp/string/byte/strtol

::: accessible/atk/nsMai.h
@@ +86,5 @@
>  {
>    return aMajor < atkMajorVersion ||
> +         (aMajor == atkMajorVersion && aMinor < atkMinorVersion) ||
> +         (aMajor == atkMajorVersion && aMinor == atkMinorVersion &&
> +          aMicro <= atkMicroVersion);

or
(aMajor == atkMajorVersion &&
 (aMinor < atkMinorVersion ||
  (aMinor == atkMinorVersion && aMicro <= atkMicroVersion)))

::: accessible/base/ARIAMap.cpp
@@ +243,5 @@
> +    eNoValue,
> +    eNoAction,
> +    eNoLiveAttr,
> +    kGenericAccType,
> +    kNoReqStates

states::READONLY missed?

@@ +343,5 @@
> +    eNoValue,
> +    eNoAction,
> +    eNoLiveAttr,
> +    kGenericAccType,
> +    kNoReqStates

readonly too?

::: accessible/base/Role.h
@@ +988,5 @@
> +  LANDMARK = 169,
> +
> +  /**
> +   * A specific type of ARIA landmark. The ability to distinguish navigation
> +   * landmarks from other types of landmarks is needed because macOS has a

maybe make it a bit more generic: ... types of landmarks is, for example, needed on macOS, where specific ASSubrole ... are used.

::: accessible/mac/mozAccessible.mm
@@ +751,5 @@
>  
> +  // macOS groups the specific landmark types of DPub ARIA into two broad
> +  // categories with corresponding subroles: Navigation and region/container.
> +  if (mRole == roles::NAVIGATION)
> +      return @"AXLandmarkNavigation";

nit: two lines indent, here and below

::: accessible/tests/mochitest/attributes/test_dpub_aria_xml-roles.html
@@ +57,5 @@
> +      testAttrs("doc_pullquote", {"xml-roles" : "doc-pullquote"}, true);
> +      testAttrs("doc_qna", {"xml-roles" : "doc-qna"}, true);
> +      testAttrs("doc_subtitle", {"xml-roles" : "doc-subtitle"}, true);
> +      testAttrs("doc_tip", {"xml-roles" : "doc-tip"}, true);
> +      testAttrs("doc_toc", {"xml-roles" : "doc-toc"}, true);

it's fine but I would probably put that into cycle like
let dpub_attrs = [
  'doc_abstract',
  'doc_acknowledgments',
  ...
];
for (let attr of dpub_attrs) {
  testAttrs(attr, {"xml-roles" : attr}, true);
}

Comment 8

[Joanmarie Diggs]

Attachment: proposed patch - review comments addressed

Thanks for the review! I believe I've addressed all of the issues you identified.

Comment 9

[Carsten Book [:Tomcat]]

needs ipc peer review:

remote: ************************** ERROR ****************************
remote: 
remote: Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
remote: This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
remote: 
rem

Comment 10

[Joanmarie Diggs]

I've been NEEDINFOed, but I'm not sure what I'm supposed to do in response. Do I re r? it or something else?

Comment 11

[Marco Zehe (:MarcoZ)]

Yes, you need to request review from an IPC peer.

Comment 12

[alexander :surkov (:asurkov)]

I'm not sure how bad to add new method here, but I think we can do if (roleAtom) {} instead of adding HasARIARole method in this particular case.

Comment 13

[Joanmarie Diggs]

Attachment: proposed patch - review comments addressed; no ipc

(In reply to alexander :surkov from comment #12)
> I'm not sure how bad to add new method here, but I think we can do if
> (roleAtom) {} instead of adding HasARIARole method in this particular case.

Done. Thanks!

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8877269 [details] [diff] [review]
proposed patch - review comments addressed; no ipc

Review of attachment 8877269 [details] [diff] [review]:
-----------------------------------------------------------------

looks good with me, and shorter :) thank you

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e48771504aa
Implement ARIA DPUB extension. r=surkov

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e48771504aa