Closed Bug 1296769 Opened 4 years ago Closed 4 years ago

introduce a11y API

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached patch patchSplinter Review
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/
Attachment #8783092 - Flags: review?(yzenevich)
Attachment #8783092 - Flags: review?(bugs)
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?
Attachment #8783092 - Flags: review?(yzenevich) → review+
(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 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+.
Attachment #8783092 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Otherwise the patch fails
> test_interfaces.html 
er, actually, the test definitely fails with that.
(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
(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.
(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?
nope. You shouldn't need anything in Bindings.conf when doing usual webidl stuff.
(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
(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?
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.
Attached patch patchSplinter Review
* renamed jsapi to AOM (accessible object model, a current name of the draft)
* test was rewritten to enable pref on start up
Attachment #8783967 - Flags: review?(yzenevich)
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');
}
Attachment #8783967 - Flags: review?(yzenevich) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/37341d5c1bd4
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: weba11y
You need to log in before you can comment on or make changes to this bug.