530039 - expose accessible action interface in DOMi

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Shawn Wilsher :sdwilsh]

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

>+    var viewerid = tab.id.replace("tab_", "");
>+    var viewer = this.viewers[viewerid];
>+    if ("inspectInNewView" in viewer)
>+      viewer.inspectInNewView();
why is this now needed?  It's not clear to me.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 413585 [details] [diff] [review])
> >+    var viewerid = tab.id.replace("tab_", "");
> >+    var viewer = this.viewers[viewerid];
> >+    if ("inspectInNewView" in viewer)
> >+      viewer.inspectInNewView();
> why is this now needed?  It's not clear to me.

This ability is not supported for every a11y property viewer so I just don't wanted to add this method stub to every prop viewer (for example, I add actionsViewer which hasn't inspectInNewView method)

Comment 4

[[e-mail]]

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

What sort of object has more than one action?

>+<!ENTITY descInvoke.label "Invoke">
Shouldn't there be an access key for this ;-)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 413585 [details] [diff] [review])
> What sort of object has more than one action?

For example container treeitem which has two actions.  The "select" action and "collapse/expand" action can  be exposed. Or image accessible might have two actions: "click" if it's inside of HTML anchor and "showlongdesc" action to show his long description in separate window.

> >+<!ENTITY descInvoke.label "Invoke">
> Shouldn't there be an access key for this ;-)

Probably it would be hard to assign access keys for n buttons (however now there are three choices only: no actions, one action and two actions). CC'ing Marco to get his opinion.

Comment 6

[[e-mail]]

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

>+    if ("inspectInNewView" in viewer)
>+      viewer.inspectInNewView();
Nit: remove the dummy inspectInNewView method from the attributes viewer?

>+      var actionItem = this.mDefaultActionItem.cloneNode(true);
[XBL would be another way of achieving this.]

>+    try {
>+      var keys = this.mAccessible.getKeyBindings(aActionIndex);
>+      var keysStr = "";
>+      for (var idx = 0; idx < keys.length; idx++)
>+        keysStr += keys.item(idx);
>+
>+      elm.textContent = keysStr;
>+    } catch (e) { }
Assuming you're expecting the exception, then IMHO keysStr belongs outside of the try/catch, so that you can clear any cloned value.

>+    btn.setAttribute("oncommand", "viewer.doCommand(" + aActionIndex + ");");
This isn't ideal but the only other option I can think of is to have a command handler on the action item container which read the action index as an attribute of the clicked button.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> >+      var actionItem = this.mDefaultActionItem.cloneNode(true);
> [XBL would be another way of achieving this.]

XBL is a nice idea but I think it should require to make any other property viewer tabs to follow this.

> >+    btn.setAttribute("oncommand", "viewer.doCommand(" + aActionIndex + ");");
> This isn't ideal but the only other option I can think of is to have a command
> handler on the action item container which read the action index as an
> attribute of the clicked button.

I have not strong opinion which version is better. If you prefer your version then I'll change.

Comment 8

[[e-mail]]

(In reply to comment #7)
> I have not strong opinion which version is better.
Don't worry about it.

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 413585 [details] [diff] [review])
> > What sort of object has more than one action?
> For example container treeitem which has two actions.  The "select" action and
> "collapse/expand" action can  be exposed.
I couldn't get the collapse/expand action to invoke; it always did a select.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (In reply to comment #7)
> > I have not strong opinion which version is better.
> Don't worry about it.
> 
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 413585 [details] [diff] [review])
> > > What sort of object has more than one action?
> > For example container treeitem which has two actions.  The "select" action and
> > "collapse/expand" action can  be exposed.
> I couldn't get the collapse/expand action to invoke; it always did a select.

I can see something bogus with accessible tree what might be related, I think it should be internal a11y problems and they shouldn't be related with the inspector patch it sounds. If you don't see how the patch might be guilty then let's take it. Later I will debug a11y code and file proper bugs.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I have not strong opinion which version is better.
> > Don't worry about it.
> > 
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 413585 [details] [diff] [review] [details])
> > > > What sort of object has more than one action?
> > > For example container treeitem which has two actions.  The "select" action and
> > > "collapse/expand" action can  be exposed.
> > I couldn't get the collapse/expand action to invoke; it always did a select.
> 
> I can see something bogus with accessible tree what might be related, I think
> it should be internal a11y problems and they shouldn't be related with the
> inspector patch it sounds. If you don't see how the patch might be guilty then
> let's take it. Later I will debug a11y code and file proper bugs.

However I can't reproduce "collapse/expand" action works as a select with tree of Library left panel.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3

sorry the wrong patch has been attached

Comment 13

[[e-mail]]

I was actually trying with the History sidebar, but the problem I now discover was that it was expanding the wrong element. I shall assume this is a core bug.

Comment 14

[alexander :surkov (:asurkov)]

Shawn, ping.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 415318 [details] [diff] [review]
patch3

nit: s/var/let/ in the new code please

r=sdwilsh with that fixed

Comment 16

[alexander :surkov (:asurkov)]

landed with addressed Shawn's comments - http://hg.mozilla.org/dom-inspector/rev/f0f1ac875ff4

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (From update of attachment 415318 [details] [diff] [review])
> nit: s/var/let/ in the new code please

Btw, Shawn what is an objection to replace var on let keyword? Is var keyword deprecated?

Comment 18

[Shawn Wilsher :sdwilsh]

No, but let has sane scoping rules that programmers tend to better understand.