846185 - don't call into nsIAccessibleProviders during a11y tree update

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 844074.

Since we don't allow DOM/frame mutations during accessible tree creation then nsIAccessibleProvider implemented in XBL bindings doesn't look safe. I suggested to replace it on attribute approach like

<binding>
  <content role="XULAlert">
  </content>
</binding>

Note, @role used on xbl:content is used to choose an accessible class (for example XULAlertAccessible). Anybody who uses @role on the element the binding is bound to doesn't change the used class (i.e. XULAlertAccessible + mARIARoleMap).

Comment 1

[kunal bansal]

I want to work on this bug but i am totally new to development though i am well acquainted with knowledge of c++.
So can i ?

Comment 2

[alexander :surkov (:asurkov)]

More detailed description:

The bug consist of two parts. You need to change nsAccessibilityService::CreateAccessibleByType
(http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1025)
so that it checks @role attribute instead nsIAccessibleProvider usage.

Second part is you need to remove nsIAccessibleProvider from all binding files (.xml files in toolkit/ folder) and put corresponding @role attribute at 'content' element.

More detailed: now we have bindings like (see for example
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/button.xml#12)

<binding>
  <implementation implements="nsIAccessibleProvider">
    <property name="accessibleType">
      return nsIAccessibleProvider.XULButton;
    </property>
  </implementation>
</binding>

then we query nsIAccessibleProvider and call 'accessibleType' in nsAccessibilityService::CreateAccessibleByType to get a constant we
choose accessible class by (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessibilityService.cpp#1025).

I want to get this replaced on:

<binding>
  <content role="XULButton">
  </content>
</binding>

then you get prorotype element for the content in CreateAccessibelByType and depending on the @role attribute value you create accessible object.

Note, you need to get the attribute value from XBL binding prototype element (see content/xbl folder):
1) get a binding element for the given content element (iirc see
either nsIContent or nsIDocument methods for this)
2) get a prototype element (I guess nsXBLPrototypeBinding.h is a right
class but I don't know that code well)
3) get its nsIContent element and then get 'role' attribute

Comment 3

[alexander :surkov (:asurkov)]

(In reply to kunal bansal from comment #1)
> I want to work on this bug but i am totally new to development though i am
> well acquainted with knowledge of c++.
> So can i ?

Pls read the previous comment and let me know if you want it since it might be complicated. Then I'll assign it to you.

Comment 4

[kunal bansal]

ya since it just contains of replacements and as i am well acquainted with knowledge of c++ ,I think i can work on this

Comment 5

[alexander :surkov (:asurkov)]

ok, cool, assigning

Comment 6

[kunal bansal]

sorry but i got struck at the first step ie.

1) get a binding element for the given content element
can you give me a hint where to start ?

Comment 7

[alexander :surkov (:asurkov)]

Boris, can you advise us how we can get xbl:content element for the bound element? (I suppose only one xbl:content is in use in case of binding inheritance)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Boris, can you advise us how we can get xbl:content element for the bound element?

Get the nsXBLBinding for it and then nsXBLBinding::GetAnonymousContent?

Comment 9

[Josh Matthews [:jdm]]

Kunal, can you conform that you're still working on this? Did comment 7 and comment 8 help?

Comment 10

[kunal bansal]

@josh i will not be available this week .So if anyone wants to take this ,surely can

Comment 11

[Sunny [:darkowlzz]]

Attachment: Modifications in nsAccessibilityService::CreateAccessibleByType

Submitting this patch with changes to |nsAccessibilityService::CreateAccessibleByType|. 

The 2nd part of the bug is to remove all |nsIAccessibleProvider| from all the binding files. While starting off with this, I got confused with various tags inside <binding>, and there being more than 1 <property> tag. 

For instance, http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/button.xml#22 has <property>s under <implementation>.
How do I convert these bindings with so many properties and into just <content> as per the requested implementation.
How do I implement the new <content> tag over the existing code in all the .xul files?

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 754755 [details] [diff] [review]
Modifications in nsAccessibilityService::CreateAccessibleByType

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

it looks good, do you plan to fix bindings part?

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1134,5 @@
>  nsAccessibilityService::CreateAccessibleByType(nsIContent* aContent,
>                                                 DocAccessible* aDoc)
>  {
> +  nsAutoString role;
> +  if(!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::role, role)) {

nit: space after 'if' keyword

you know it's better to use aContent->FindAttrValueIn() and then you can use 'switch' (sorry I didn't say that when we talked on irc)

@@ +1271,5 @@
> +  }
> +  else if (role == "XULTooltip") {
> +    accessible = new XULTooltipAccessible(aContent, aDoc);
> +  }
> +  else if (role == "XULToolbarButton") { 

nit: space at the end of line

Comment 13

[Sunny [:darkowlzz]]

> you know it's better to use aContent->FindAttrValueIn() and then you can use
> 'switch' (sorry I didn't say that when we talked on irc)

Won't I get a string and how do I use that in a 'switch'?

Yes, I would like to fix the bindings part too. Please look at my previous comment and help me with my confusions in the new implementation of the tags.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Sunny [:darkowlzz] from comment #13)
> > you know it's better to use aContent->FindAttrValueIn() and then you can use
> > 'switch' (sorry I didn't say that when we talked on irc)
> 
> Won't I get a string and how do I use that in a 'switch'?

it returns you an index in the values array (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContent.h#437)

> Yes, I would like to fix the bindings part too.

doing it by parts is a good idea

Comment 15

[Sunny [:darkowlzz]]

unassigning. Would get back soon.

Comment 16

[Trevor Saunders (:tbsaunde)]

role approach doesn't work nicely because bindings can inherit so you need to look at parents content element too.  For now I've implemented approach of caching result of accessible provider in xbl prototype binding and have a11y just look at that.

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: don't call into js when creating accessibles

not terribly nice but its should be safer and makes debugging xbl a11y tree stuff nicer

r? smaug for content/xbl/ and surkov the rest of it

Comment 18

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

Comment on attachment 782724 [details] [diff] [review]
don't call into js when creating accessibles

I hadn't realized we have such odd accessibleType implementations in XBL.
So I'm not quite happy with this.
Perhaps we could use the role approach tough, if it is still ok to
add those odd special cases to a11y code.
I wouldn't put role to <content> but <binding> or something. That way we don't
copy the attribute to bound element, but would just rely on
mPrototypeBinding->mBinding or mNextBinding->mPrototypeBinding->mBinding to
provide the role value.

But still, it is not clear to me whether it is ok to put those hack so a11y C++ code. Somehow keeping them in xbl/js feels better.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #18)

> I hadn't realized we have such odd accessibleType implementations in XBL.
> So I'm not quite happy with this.
> Perhaps we could use the role approach tough, if it is still ok to
> add those odd special cases to a11y code.
> I wouldn't put role to <content> but <binding> or something. That way we
> don't
> copy the attribute to bound element, but would just rely on
> mPrototypeBinding->mBinding or mNextBinding->mPrototypeBinding->mBinding to
> provide the role value.

sounds good. It doesn't interfere with ARIA role what is also good because we create a proper base class and ARIA stuff just come from Accessible class like it works in case of HTML.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 782724 [details] [diff] [review]
> don't call into js when creating accessibles
> 
> I hadn't realized we have such odd accessibleType implementations in XBL.

neither had I till I debugged stuff.

> So I'm not quite happy with this.

me neither

> Perhaps we could use the role approach tough, if it is still ok to
> add those odd special cases to a11y code.

I don't really care which approach we use.

> I wouldn't put role to <content> but <binding> or something. That way we
> don't
> copy the attribute to bound element, but would just rely on
> mPrototypeBinding->mBinding or mNextBinding->mPrototypeBinding->mBinding to
> provide the role value.
> 
> But still, it is not clear to me whether it is ok to put those hack so a11y
> C++ code. Somehow keeping them in xbl/js feels better.

I feel the same way on the other hand there are already some somewhat hackish things in c++ for xul image menupopup and tree grid stuff.

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 782724 [details] [diff] [review]
> don't call into js when creating accessibles
> 
> I hadn't realized we have such odd accessibleType implementations in XBL.
> So I'm not quite happy with this.
> Perhaps we could use the role approach tough, if it is still ok to
> add those odd special cases to a11y code.
> I wouldn't put role to <content> but <binding> or something. That way we
> don't
> copy the attribute to bound element, but would just rely on
> mPrototypeBinding->mBinding or mNextBinding->mPrototypeBinding->mBinding to
> provide the role value.

to be clear the way xbl prototypes work is the following right?

if binding prototype b inherits from binding prototype a then a binding with prototype b will have mNextBinding->mPrototype point at the a prototype right?

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 782724 [details] [diff] [review]
don't call into js when creating accessibles

Trev, you will file a new patch, right?

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: wip bug 846185 - don't call into js when creating accessibles

this takes the roles approach and removes nsIAccessibleProvider, but it still has some bugs, it causes a bunch of test failures.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 795555 [details] [diff] [review]
wip bug 846185 - don't call into js when creating accessibles

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

r=me with comments fixed

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1151,3 @@
>      return nullptr;
>  
> +  if (role.EqualsLiteral("outerdoc")) {

nit: can you please leave constant names unchanged "outerdoc" vs OuterDoc, it's easier for readability

@@ +1154,4 @@
>      nsRefPtr<Accessible> accessible = new OuterDocAccessible(aContent, aDoc);
>      return accessible.forget();
>    }
> + 

nit: whitespace

@@ +1159,4 @@
>  #ifdef MOZ_XUL
> +  // XUL controls
> +  if (role.EqualsLiteral("xulalert")) {
> +    accessible = new XULAlertAccessible(aContent, aDoc);

please add a space, otherwise all ifs are jammed together

@@ +1170,1 @@
>        accessible = new XULColorPickerTileAccessible(aContent, aDoc);

nit: indentation is not fixed

@@ +1200,5 @@
> +    uint32_t cells = 0;
> +    for (nsIContent* child = listItem->GetFirstChild(); child;
> +         child = child->GetNextSibling()) {
> +      if (child->IsXUL() && child->Tag() == nsGkAtoms::listcell)
> +        cells++;

you don't need to count them all, let's create a cell accessible as long as we have cell different from this one

::: browser/metro/base/content/bindings/browser.xml
@@ +787,5 @@
>          <getter>
>            <![CDATA[
>              throw "accessibleType: Supports Remote?";
>            ]]>
>          </getter>

no accessibleType anymore

::: toolkit/content/widgets/general.xml
@@ +98,5 @@
>                  onget="return this.webNavigation.document;"/>
>      </implementation>
>    </binding>
>  
> +  <binding id="statusbarpanel" display="xul:button" role="xulbutton">

no related <impl> change?

::: toolkit/content/widgets/menulist.xml
@@ -392,5 @@
> -          <![CDATA[
> -            // Droppable is currently used for the Firefox bookmarks dialog only
> -            return (this.getAttribute("droppable") == "false") ?
> -                   Components.interfaces.nsIAccessibleProvider.XULTextBox :
> -                   Components.interfaces.nsIAccessibleProvider.XULCombobox;

no content change for this?

::: toolkit/content/widgets/popup.xml
@@ -257,5 @@
> -        <getter>
> -          <![CDATA[
> -          return (this.getAttribute("noautofocus") == "true") ?
> -                       Components.interfaces.nsIAccessibleProvider.XULAlert :
> -                       Components.interfaces.nsIAccessibleProvider.XULPane;

for things like that I would prefer to have separate bindings rather than moving this logic into a11y core like

panel {
  -moz-binding(#panel);
}

panel[autofocus="true"] {
  -moz-binding("panel-alert");
}

@@ +466,5 @@
>      </handlers>
>    </binding>
>  
> +  <binding id="tooltip" role="xultooltip"
> +    extends="chrome://global/content/bindings/popup.xml#popup-base">

pls fix indentation here and in other places

::: toolkit/content/widgets/preferences.xml
@@ +1292,5 @@
>      </handlers>
>    </binding>
>            
> +  <binding id="panebutton" role="xullistitem"
> +    extends="chrome://global/content/bindings/radio.xml#radio">

indent

::: toolkit/content/widgets/radio.xml
@@ +9,5 @@
>     xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>     xmlns:xbl="http://www.mozilla.org/xbl">
>  
> +  <binding id="radiogroup" role=xulradiogroup"
> +    extends="chrome://global/content/bindings/general.xml#basecontrol">

indent it

::: toolkit/content/widgets/toolbar.xml
@@ +92,5 @@
>        </method>
>      </implementation>
>    </binding>
>    
> +  <binding id="toolbar" role="xultoolbar" 

nit: space

@@ +93,5 @@
>      </implementation>
>    </binding>
>    
> +  <binding id="toolbar" role="xultoolbar" 
> +    extends="chrome://global/content/bindings/toolbar.xml#toolbar-base">

pls line it up with attributes

@@ +511,5 @@
>      </implementation>
>    </binding>
>  
> +  <binding id="menubar" role="xulmenubar" 
> +    extends="chrome://global/content/bindings/toolbar.xml#toolbar-base" display="xul:menubar">

same as above

@@ +555,5 @@
>          <handler event="DOMMenuItemInactive">this._updateStatusText("");</handler>
>      </handlers>
>    </binding>
>  
> +  <binding id="toolbardecoration" role="toolbarseperator" extends="chrome://global/content/bindings/toolbar.xml#toolbar-base">

separator (not seperator)

::: toolkit/content/widgets/tree.xml
@@ +1166,5 @@
>        
>      </handlers>
>    </binding>
>  
> +  <binding id="treecol-base" extends="chrome://global/content/bindings/tree.xml#tree-base" role="xultreeitem">

xultreecolumnitem
btw, probably it'd be nicer to have role on own role

Comment 25

[Trevor Saunders (:tbsaunde)]

Attachment: bug 846185 - don't call into js when creating accessibles

I think I fixed all you issues want to check?

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 796712 [details] [diff] [review]
bug 846185 - don't call into js when creating accessibles

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

seems so

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1216,2 @@
>        accessible = new XULListCellAccessibleWrap(aContent, aDoc);
>        break;

nit: pls align these two lines properly

Comment 27

[Trevor Saunders (:tbsaunde)]

Comment on attachment 796712 [details] [diff] [review]
bug 846185 - don't call into js when creating accessibles

Smaug, could you take a look at the content/ bits? they're all pretty simple and small

Comment 28

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

Comment on attachment 796712 [details] [diff] [review]
bug 846185 - don't call into js when creating accessibles

>+++ b/content/xbl/src/nsXBLPrototypeBinding.h
>@@ -31,17 +31,17 @@ class nsCSSStyleSheet;
> // The XBLPrototypeBinding class
> 
> // Instances of this class are owned by the nsXBLDocumentInfo object returned
> // by XBLDocumentInfo().  Consumers who want to refcount things should refcount
> // that.
> class nsXBLPrototypeBinding
> {
> public:
>-  already_AddRefed<nsIContent> GetBindingElement();
>+  nsIContent* GetBindingElement() const { return mBinding; }
Why this change?

Comment 29

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

Comment on attachment 796712 [details] [diff] [review]
bug 846185 - don't call into js when creating accessibles

But it should be fine.

Comment 30

[Trevor Saunders (:tbsaunde)]

(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 796712 [details] [diff] [review]
> bug 846185 - don't call into js when creating accessibles
> 
> >+++ b/content/xbl/src/nsXBLPrototypeBinding.h
> >@@ -31,17 +31,17 @@ class nsCSSStyleSheet;
> > // The XBLPrototypeBinding class
> > 
> > // Instances of this class are owned by the nsXBLDocumentInfo object returned
> > // by XBLDocumentInfo().  Consumers who want to refcount things should refcount
> > // that.
> > class nsXBLPrototypeBinding
> > {
> > public:
> >-  already_AddRefed<nsIContent> GetBindingElement();
> >+  nsIContent* GetBindingElement() const { return mBinding; }
> Why this change?

iirc I wanted to make it const or inline it and just got rid of the refcounting since I was there, but it was a while ago

Comment 31

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7dcecd40428

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7dcecd40428

Comment 33

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

We probably need to document this change somewhere.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9d6cd52cdd83
don't call into js when creating accessibles r=surkov, smaug