1296769 - introduce a11y API

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

here's original version that me and Marcos were working on [1]
here's latest version of AOM (accessibility object model) [2]

This is a prototype of ideas from those two documents. It adds AccessibleNode interface with few functions for now.

[1] https://wicg.github.io/a11yapi/
[2] http://a11y-api.github.io/a11y-api/spec/

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8783092 [details] [diff] [review]
patch

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

looks cool

::: accessible/jsapi/AccessibleNode.h
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=40: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Here again, perhaps a URL to the draft would be nice.

@@ +14,5 @@
> +
> +namespace mozilla {
> +
> +namespace dom {
> +struct ParentObject;

nit: indendation

@@ +21,5 @@
> +namespace a11y {
> +
> +class Accessible;
> +
> +class AccessibleNode :  public nsISupports,

nit: extra space after ':'

@@ +26,5 @@
> +                        public nsWrapperCache
> +{
> +public:
> +  AccessibleNode(nsINode* aNode);
> + 

nit: whitespace

::: accessible/tests/browser/jsapi/browser_general.js
@@ +1,2 @@
> +add_task(function*(){
> +  var anode = document.accessibleNode;

nits:
var -> let
anode -> ...

@@ +1,5 @@
> +add_task(function*(){
> +  var anode = document.accessibleNode;
> +  ok(anode, 'no accessible node');
> +
> +  is(anode.role, 'chrome window', 'wrong role of a document accessible node');

As mentioned on IRC, lets have the success message instead of a failure one here and below, e.g. 'Document node accessible has a correct role.'

::: dom/base/nsINode.cpp
@@ +2586,5 @@
> +already_AddRefed<a11y::AccessibleNode>
> +nsINode::GetAccessibleNode()
> +{
> +#ifdef ACCESSIBILITY
> +  RefPtr<a11y::AccessibleNode> anode = new a11y::AccessibleNode(this);

nit: name anode should be better.

::: dom/webidl/AccessibleNode.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +

Some comment would be nice, perhaps a URL to the draft?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #1)

> ::: dom/webidl/AccessibleNode.webidl
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
> 
> Some comment would be nice, perhaps a URL to the draft?
> 
> ::: accessible/jsapi/AccessibleNode.h
> Here again, perhaps a URL to the draft would be nice.

I wanted to put something but refrained this time, because those versions above are not completely merged still, and the prototype may keep parts from both versions to demo benefits of one version vs another one. Keeping both may look confusing.

> > +
> > +namespace dom {
> > +struct ParentObject;
> 
> nit: indendation

two spaces? they usually don't do this under namespaces

> > +  var anode = document.accessibleNode;
> 
> nits:
> var -> let
> anode -> ...

I'm good with let, but didn't get 'anode' part

> > +  is(anode.role, 'chrome window', 'wrong role of a document accessible node');
> 
> As mentioned on IRC, lets have the success message instead of a failure one
> here and below, e.g. 'Document node accessible has a correct role.'

ok, 'wrong' replaced on 'correct'

> ::: dom/base/nsINode.cpp
> @@ +2586,5 @@
> > +already_AddRefed<a11y::AccessibleNode>
> > +nsINode::GetAccessibleNode()
> > +{
> > +#ifdef ACCESSIBILITY
> > +  RefPtr<a11y::AccessibleNode> anode = new a11y::AccessibleNode(this);
> 
> nit: name anode should be better.

suggestions?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8783092 [details] [diff] [review]
patch

> # descriptor to use when generating that interface's binding.
> 
> DOMInterfaces = {
> 
> 'AbstractWorker': {
>     'concrete': False
> },
> 
>+'AccessibleNode': {
>+    'nativeType': 'mozilla::a11y::AccessibleNode'
>+},
Could you possibly just put AccessibleNode implementation to mozilla::dom namespace 



>+++ b/dom/webidl/AccessibleNode.webidl
>@@ -0,0 +1,10 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/.
>+ */
>+
>+interface AccessibleNode {
This _must_ have the [Pref=...] check. Otherwise the patch fails test_interfaces.html (except that you enable the pref for all test. I think you should not do that)
and we don't want to expose the interface to the web.
With the patch  '"AccessibleNode" in window' ends up being true in all the web pages, which is wrong, if the stuff isn't really exposed.


Those fixed, r+.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #3)
> Otherwise the patch fails
> test_interfaces.html 
er, actually, the test definitely fails with that.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8783092 [details] [diff] [review]
> patch
> 
> > # descriptor to use when generating that interface's binding.
> > 
> > DOMInterfaces = {
> > 
> > 'AbstractWorker': {
> >     'concrete': False
> > },
> > 
> >+'AccessibleNode': {
> >+    'nativeType': 'mozilla::a11y::AccessibleNode'
> >+},
> Could you possibly just put AccessibleNode implementation to mozilla::dom
> namespace 

I could, however it is located in accessible folder which is under a11y namespace. Also this API is for sure DOM connected, but it is used to expose accessibility information. Anyway, I'm good to keep it under dom namespace, if you think it's better than a11y one. Could you confirm if this is still your preference?

> >+++ b/dom/webidl/AccessibleNode.webidl
> >@@ -0,0 +1,10 @@
> >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/.
> >+ */
> >+
> >+interface AccessibleNode {
> This _must_ have the [Pref=...] check. Otherwise the patch fails
> test_interfaces.html (except that you enable the pref for all test. I think
> you should not do that)

ok, I will

> and we don't want to expose the interface to the web.

do you mean if it is not pref'ed out?

> With the patch  '"AccessibleNode" in window' ends up being true in all the
> web pages, which is wrong, if the stuff isn't really exposed.

ok, I see

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to alexander :surkov from comment #5)
> I could, however it is located in accessible folder which is under a11y
> namespace. Also this API is for sure DOM connected, but it is used to expose
> accessibility information. Anyway, I'm good to keep it under dom namespace,
> if you think it's better than a11y one. Could you confirm if this is still
> your preference?
Web exposed APIs should use mozilla::dom by default. Some old stuff may not do it.
That is the reason why mozilla::dom doesn't need to be added to Bindings.conf.
A11y internal stuff can be then in a11y namespace.


> > and we don't want to expose the interface to the web.
> 
> do you mean if it is not pref'ed out?
Yes. Since the interface doesn't have Pref=... or similar, it gets exposed to the web.

Comment 7

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=820617f02dd6

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #6)
> (In reply to alexander :surkov from comment #5)
> > I could, however it is located in accessible folder which is under a11y
> > namespace. Also this API is for sure DOM connected, but it is used to expose
> > accessibility information. Anyway, I'm good to keep it under dom namespace,
> > if you think it's better than a11y one. Could you confirm if this is still
> > your preference?
> Web exposed APIs should use mozilla::dom by default. Some old stuff may not
> do it.
> That is the reason why mozilla::dom doesn't need to be added to
> Bindings.conf.

do I still need a record in 'Bindings.conf' for the 'AccessibleNode', because it's got empty, since 'nativeType' no longer required?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

nope. You shouldn't need anything in Bindings.conf when doing usual webidl stuff.

Comment 10

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f63f8ad550b

Comment 11

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7baad9de0ca1

Comment 12

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9649d7c02c8

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to Olli Pettay [:smaug] from comment #3)
> > Otherwise the patch fails
> > test_interfaces.html 
> er, actually, the test definitely fails with that.

I have pref on top of interface [1] but the test still fails [2]. Ideas?

[1] https://hg.mozilla.org/try/diff/e9649d7c02c8/dom/webidl/AccessibleNode.webidl
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=26158234&repo=try#L8883

Comment 14

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=965f04569791

Comment 15

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #13)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > (In reply to Olli Pettay [:smaug] from comment #3)
> > > Otherwise the patch fails
> > > test_interfaces.html 
> > er, actually, the test definitely fails with that.
> 
> I have pref on top of interface [1] but the test still fails [2]. Ideas?

I guess that is because I'm enabled the pref in testing/profiles/prefs_general.js for the browser tests. Olli, should I add the interface to test_interfaces.html or there's a way to flip the pref on and off for my test only?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

You should use pushPrefEnv in the test and then run the actual test in a new window or iframe so that the new js global is created with the pref set.

Comment 17

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35801429b1e4

Comment 18

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b872e96343f

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch

* renamed jsapi to AOM (accessible object model, a current name of the draft)
* test was rewritten to enable pref on start up

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8783967 [details] [diff] [review]
patch

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

looks good, some comments below

::: accessible/aom/AccessibleNode.cpp
@@ +43,5 @@
> +{
> +  return AccessibleNodeBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +/* virtual */ mozilla::dom::ParentObject

talked about this namespace nit

::: accessible/tests/mochitest/aom/test_general.html
@@ +26,5 @@
> +      "set": [
> +        [ "accessibility.AOM.enabled", true ],
> +      ],
> +    };
> +    return SpecialPowers.pushPrefEnv(ops);

I think once you pushed stuff to env, you need to also call SpecialPowers.popPrefEnv() at the end of the test.

@@ +29,5 @@
> +    };
> +    return SpecialPowers.pushPrefEnv(ops);
> +  }
> +
> +  // WebIDL conditional annotations for an interface are evaluate once per

nit: evaluate -> evaluated

@@ +34,5 @@
> +  // global, so we need to create an iframe to see the effects of calling
> +  // enablePref().
> +  function createIframe() {
> +    return new Promise((resolve) => {
> +      const iframe = document.createElement("iframe");

const here is confusing since you are setting properties on it down the line. I would just use let here.

@@ +43,5 @@
> +  }
> +
> +  // Check that the WebIDL is as expected.
> +  function checkImplementation(ifrDoc) {
> +    return new Promise((resolve, reject) => {

this stuff is not async so no need to wrap the whole thing in new Promise(...). The function will still be called before we get to the next then.

You can just do:

function checkImplementation(ifrDoc) {
  let anode = ifrDoc.accessibleNode;
  ok(anode, "DOM document has accessible node");
  is(anode.role, 'document', 'correct role of a document accessible node');
  is(anode.DOMNode, ifrDoc, 'correct DOM Node of a document accessible node');
}

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #20)

> ::: accessible/tests/mochitest/aom/test_general.html
> @@ +26,5 @@
> > +      "set": [
> > +        [ "accessibility.AOM.enabled", true ],
> > +      ],
> > +    };
> > +    return SpecialPowers.pushPrefEnv(ops);
> 
> I think once you pushed stuff to env, you need to also call
> SpecialPowers.popPrefEnv() at the end of the test.

apparently not, see bug 1280777 comment 8

> @@ +43,5 @@
> > +  }
> > +
> > +  // Check that the WebIDL is as expected.
> > +  function checkImplementation(ifrDoc) {
> > +    return new Promise((resolve, reject) => {
> 
> this stuff is not async so no need to wrap the whole thing in new
> Promise(...). The function will still be called before we get to the next
> then.

k

Comment 22

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37341d5c1bd4ab8334fd88feb58840c58b8e6749
Bug 1296769 - introduce a11y API, r=smaug, yzen

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37341d5c1bd4