241015 - Remote xul have no accessibility API support

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Aaron Leventhal]

Not sure if this also happens under Linux/ATK support.

Steps:
Open up http://www.mozilla.org/docs/tutorials/sitenav/6.xul
Launch MSAA SDK Inspect tool
Point to objects or focus them

What happens:
Nothing in either the HTML or XUL is being as exposed as an accessible object.
No decendants of the content area are accessible

What should happen:
Objects are exposed

Comment 1

[Aaron Leventhal]

Benjamin says that the accessible helper used in XBL doesn't have privileges.

We're currently running code like this (from
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/checkbox.xml):
 23     <implementation implements="nsIDOMXULCheckboxElement,
nsIAccessibleProvider">
 24       <property name="accessible">
 25         <getter>
 26           <![CDATA[
 27             var accService =
Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);
 28             return accService.createXULCheckboxAccessible(this);
 29           ]]>
 30         </getter>
 31       </property>

Comment 2

[Aaron Leventhal]

Javascript error console says
Error: [Exception... "'Permission denied to get property UnnamedClass.classes'
when calling method: [nsIAccessibleProvider::accessible]"  nsresult: "0x8057001e
(NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]

Comment 3

[Aaron Leventhal]

So, the accessibility module is able to get the nsIAccessibleProvider for the
remote XUL node, but not get a property from it.

How do we get permission?

Comment 4

[Aaron Leventhal]

The problem is this:
the accessibility service which is used is not remote safe and xbl bindings run
in the non-trusted security mode of the page itself.

So to fix this we can:
1) Fix XBL, or
2) Not do this in XBL, or 
3) Find a way to provide an accessible in XBL without using a non-remote safe
interface

Comment 5

[timeless]

nsIClassInfo

Comment 6

[Aaron Leventhal]

Timeless, is it that simple? We don't want untrusted content to have access to
nsIAccessible.

Comment 7

[timeless]

oh, then you'll have to talk to caps and find out who the caller is.

Comment 8

[Aaron Leventhal]

This would probably fix bug 297069.

Comment 9

[Nian Liu(n/a in a long time)]

*** Bug 320921 has been marked as a duplicate of this bug. ***

Comment 10

[Aaron Leventhal]

*** Bug 323450 has been marked as a duplicate of this bug. ***

Comment 11

[Aaron Leventhal]

The caller of nsIAccesibleProvider::GetAccessible() is in trusted C++ code in mozilla/accessible, yet it is not allowed to get the results because the XBL is running in the context of the page. How can we fix this? Can we fix it without blessing that interface to be used by untrusted code to create accessibles?

Comment 12

[Boris Zbarsky [:bzbarsky]]

It's not that it's not allowed to get the results, nor is this a problem with the nsIAccessibleProvider interface.  It's just that accessing Components.classes in the XBL throws.  Untrusted content is not allowed to call createInstance or getService, and not even allowed to touch Components.classes.

So this raises two obvious questions:

1)  Is it safe to give untrusted content access to nsIAccessibilityService?
2)  If so, is it safe to give untrusted content access to the objects returned by 
    createXULCheckboxAccessible?

Comment 13

[Aaron Leventhal]

(In reply to comment #12)
>> 1)  Is it safe to give untrusted content access to nsIAccessibilityService?
> 2)  If so, is it safe to give untrusted content access to the objects returned
> by 
>     createXULCheckboxAccessible?

No it's not safe for 2 reasons:
1) You can change focus, select options and invoke actions via accessibility APIs. In the future you will be able to set text in textfields. If content could get an accessible it could go up the content's accessible parent chain and get into chrome.
2) The a11y code has never been analyzed closely for security. Why increase the surface area for attacks if we don't need to.

So I guess we need some other way for the accessibility service to decide what kind of accessible to create for each xbl widget.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.... Could we have bindings tell the box object or something what sort of accessible to return?  And then have a non-scriptable (or scriptable and security-checked) API that XUL documents implement that accessibility can call that will check whether there's a box object, and if so ask it for the info about the accessible, etc?

Comment 15

[Aaron Leventhal]

Originally nsIAccessibleProvider was going to be an extension mechanism so script authors could create new kinds of accessibles as long as those implemented nsIAccessible. However, we're going to be positioning xhtml2:role and the DHTML a11y properties as the method for extending accessibility to new widgets.

So, we are best off killing nsIAccessibleProvider and just having nsAccessibilityService::CreateAccessibleByXULMarkup() which looks at the tag name and possibly some of the attributes and creates an accessible from that. It will be faster that way as well.

Comment 16

[Aaron Leventhal]

(In reply to comment #15)
> So, we are best off killing nsIAccessibleProvider and just having
> nsAccessibilityService::CreateAccessibleByXULMarkup()
This is going to touch a lot of files. I only plan on fixing it in toolkit.

Comment 17

[Aaron Leventhal]

Taking off Firefox 2 list -- we just realized this is not easily doable without adding a new API or changing nsIAccessibleProvider.

Comment 18

[alexander :surkov (:asurkov)]

Aaron, can I assign this bug to me? I want to approve factory idea (from bug 337680) and if it works then I use it for xforms accessibility (right now I can't do it for xforms until bug 342638 is fixed).

Comment 19

[alexander :surkov (:asurkov)]

Probably I was too quick in bug assignment. The factory idea is very good for html since html is a thing that isn't changed. Probably it's good for xforms too since xforms have strict specification. But xul afaik hasn't specifcation. And I can't say that statement "one tagname - one accessible object" is true for xul.

For example:
<button/> and <statusbarpanel class="statusbarpanel-iconic"/> has one the same accessible object whereas <statusbarpanel/> hasn't accessible object, <statusbarpanel/> has xul:button display though.

AaronL, what do you think?

Comment 20

[alexander :surkov (:asurkov)]

I noticed that factory for creating accessible object idea is not suits fine as I can think before. We can create right accessible object having DOM node though but I guess it looks not so good because we define element in one place and link it with accessible object in other place. In that case I like more idea of accessible provider since binding says what accessible object should be used for element. But as you said everyone can get access for accessibility object and it's not good. We can modify idea of accessible provider. If I'll get right then xhtml:role attribute is supported currenly. We can support our own values of @xhtml:role for xul/xforms and e.t.c. elements and put this attribute on xbl:content element. For example, something like:

<binding id="button">
  <content xhtml:role="xul:button">
   <!-- llalal -->
  </content>
</binding>

If xhtml:role isn't right thing for this then we can support it by using some another attribute.

AaronL?

Comment 21

[alexander :surkov (:asurkov)]

It looks like CreateXULSelectOptionAccessible(), createXULSelectListAccessible() aren't used.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch

BL bindings implement property 'accessibleType' of nsIAccessibleProvider interface. accessibilityService returns accessible object by that type.

Comment 23

[Håkan Waara]

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

Would it be possible to put the new types inside a new header instead?

We already have the problem of nsIAccessible.idl being full of constants.

Comment 24

[Aaron Leventhal]

(In reply to comment #23)
> (From update of attachment 228685 [details] [diff] [review] [edit])
> Would it be possible to put the new types inside a new header instead?
> 
> We already have the problem of nsIAccessible.idl being full of constants.

Yes, good idea.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 228685 [details] [diff] [review] [edit] [edit])
> > Would it be possible to put the new types inside a new header instead?
> > 
> > We already have the problem of nsIAccessible.idl being full of constants.
> 
> Yes, good idea.
> 

If we put constants into header file then how bindings will use them? I don't like a lot if bindings will use numbers instead constants.

Comment 26

[Aaron Leventhal]

Yes, I realized that afterward. We have to keep it in nsIAccessible because of the bindings.

Comment 27

[Aaron Leventhal]

Okay, keep them in nsIAccessibleProvider, but please change them to an enum.

Also, XBL form controls was an attempt to rewrite HTML form controls that never materialized. It was recently resolved WONTFIX in bug 57209. Just remove the handling of XBL-based HTML form control types from this completely.

Comment 28

[Aaron Leventhal]

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

This looks good, although I think we probably want to use enum instead of const.  A superreviewer would know best. Also don't forget to remove the XBL form control stuff, we simply don't need it.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch2

accessibility support of xblfc is removed,
nsIAccessibleProvider uses constants instead enum per http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/rules.html

Comment 30

[Asaf Romano (gone)]

Comment on attachment 228918 [details] [diff] [review]
patch2

I'm not a sr.

Comment 31

[Asaf Romano (gone)]

(In reply to comment #28)
> (From update of attachment 228685 [details] [diff] [review] [edit])
> This looks good, although I think we probably want to use enum instead of
> const.

No enum support in XPIDL.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #30)
> (From update of attachment 228918 [details] [diff] [review] [edit])
> I'm not a sr.
> 

Sorry, I guess I didn't understand AaronL right. Does this bug need super-review or will second review be enough?

Comment 33

[Asaf Romano (gone)]

As far as I know you do need sr (superreview, not just second-review) on a11y module patches.

Comment 34

[Aaron Leventhal]

(In reply to comment #33)
> As far as I know you do need sr (superreview, not just second-review) on a11y
> module patches.

No we are not requiring sr= for trunk a11y patches at this time. However, we will  ask for sr= on patches which need to go on branch or have something which might benefit from an sr= looking at it.

Comment 35

[alexander :surkov (:asurkov)]

Removing dependency on bug 59701 since we choosed way doesn't require any priveleges for executed code.

Comment 36

[alexander :surkov (:asurkov)]

Asaf Romano, mainly I ask you for review to check toolkit changes. The patch is big but very simple. It just changed nsIAccessibleProvider interface. The interface has accesibleType property (that returns type of accessible object) instead accessible property (that returns accessible object).

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch3

updated to trunk

Comment 38

[Asaf Romano (gone)]

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

toolkit changes are fine, r=mano (neil or jag should rs the xpfe part).

Comment 39

[alexander :surkov (:asurkov)]

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

Neil, please check xpfe changes.

Comment 40

[[e-mail]]

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

>   <binding id="popup" extends="chrome://global/content/bindings/popup.xml#popup-base">
>     <content>
>       <xul:arrowscrollbox class="popup-internal-box" flex="1" orient="vertical">
>         <children/>
>       </xul:arrowscrollbox>
>     </content>
> 
>     <implementation implements="nsIDOMXULPopupElement, nsIAccessibleProvider">
>-      <property name="accessible">
>+      <property name="accessibleType" readonly="true">
>         <getter>
>           <![CDATA[
>-            var accService = Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);
>             if (this.localName == "popup" || this.localName == "menupopup")
>-              return accService.createXULMenupopupAccessible(this);
>+              return Components.interfaces.nsIAccessibleProvider.XULMenupopup;
>             else if (this.localName == "tooltip")
>-              return accService.createXULTooltipAccessible(this);
>+              return Components.interfaces.nsIAccessibleProvider.XULTooltip;
>             return null;
>           ]]>
>         </getter>
>       </property>
I seem to remember this got mucked around a bit (most recently for bug 339237). I originally spotted a problem because it uses "return null;" which obviously isn't a valid accessible type. However, I think it would be clearer to make this type always return XULMenupopup and add a type to the tooltip binding to always return XULTooltip. r=me with this fixed.

Comment 41

[alexander :surkov (:asurkov)]

Attachment: check in

Comment 42

[alexander :surkov (:asurkov)]

Attachment: updated

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The checkin of this patch overwrote the changes to toolkit/mozapps/extensions/content/extensions.xml that were in the previous revision of that file -- rob_strong noticed and fixed this problem.  Were there any similar problems?

Such mistakes tend to be a sign of misuse of CVS.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that that mistake was in attachment 230428 [details] [diff] [review] (but not in attachment 230125 [details] [diff] [review]).

Comment 45

[Karsten Düsterloh]

Comment on attachment 230428 [details] [diff] [review]
updated

>Index: xpfe/global/resources/content/bindings/toolbar.xml

This was the wrong file to patch if you intended to patch SeaMonkey.
It's not used anymore and will eventually be deleted (should be doing this very soon, it obviously seems) as the comment at it's top clearly states. 
You should have patched /suite/common/bindings/toolbar.xml instead.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> The checkin of this patch overwrote the changes to
> toolkit/mozapps/extensions/content/extensions.xml that were in the previous
> revision of that file -- rob_strong noticed and fixed this problem.

It looks like I didn't upldate mozilla source before previous patch applying to update it to trunk. After that I always overview new patch. The patch was big I didn't notice any wrong in extensions.xml. It's my bad. I'm very sorry.

> Were there any similar problems?

I know one similar problem, but it wasn't realized since I catched sight of it. It's next to last patch of bug 337674, there I forgot to add new files. It's not nice details in the light of my cvs rights asking. But I guess it's better to be honest in that.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> (From update of attachment 230428 [details] [diff] [review] [edit])
> >Index: xpfe/global/resources/content/bindings/toolbar.xml
> 
> This was the wrong file to patch if you intended to patch SeaMonkey.
> It's not used anymore and will eventually be deleted (should be doing this very
> soon, it obviously seems) as the comment at it's top clearly states. 
> You should have patched /suite/common/bindings/toolbar.xml instead.
> 

Bindings of /suite/common/bindings/toolbar.xml are extended from bindings from xpfe's toolbar.xml bindings. When seamonkey will migrate to toolkit then they will be extedned from toolkit's toolbar.xml bindings. Now xpfe and toolkit toolbar bindings have accessibility support both. Should suite's toolbar bindings have special accessibility support or did I miss something?

Comment 48

[Karsten Düsterloh]

> Bindings of /suite/common/bindings/toolbar.xml are extended from bindings from
> xpfe's toolbar.xml bindings.

Actually: no, not really.

> When seamonkey will migrate to toolkit then they will be extedned from
> toolkit's toolbar.xml bindings.

We're already packaging the toolkit file, not the xpfe one.

> Now xpfe and toolkit toolbar bindings have accessibility support both.
> Should suite's toolbar bindings have special accessibility support or did
> I miss something?

No, but I did; my apologies. :-/
So the patching of the xpfe toolbar.xml was just unneeded.

Comment 50

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381