804461 - build the context dependent tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Example: role="presentation" on HTML table gets rid all HTML table related descendants. We should do the same for role="presentation" on HTML list (like ul). On the another hand it doesn't make sense to expose context dependent accessible having no context like <div role="cell"> if it's not contained by grid.

Thoughts on implementation:
every accessible class should have a context, a bit wise flags describing not allowed accessibles, for example, generic Accessible class doesn't allow table or list related descendants, HTMLTableAccessible and etc overrides it. Morph nsAccessibiltiyService::GetOrCreateAccessible to take a context.

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #0)
> Example: role="presentation" on HTML table gets rid all HTML table related
> descendants. We should do the same for role="presentation" on HTML list
> (like ul). On the another hand it doesn't make sense to expose context
> dependent accessible having no context like <div role="cell"> if it's not
> contained by grid.
> 
> Thoughts on implementation:
> every accessible class should have a context, a bit wise flags describing
> not allowed accessibles, for example, generic Accessible class doesn't allow
> table or list related descendants, HTMLTableAccessible and etc overrides it.
> Morph nsAccessibiltiyService::GetOrCreateAccessible to take a context.

I'm not sure I see why it makes more sense to pass it some sort of context thing instead of just passing the parent directly.  I think we can probably do most if not all of these checks just based on mFlags and or Role of the accessible.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> I'm not sure I see why it makes more sense to pass it some sort of context
> thing instead of just passing the parent directly.  I think we can probably
> do most if not all of these checks just based on mFlags and or Role of the
> accessible.

parent instead flags should be fine if we are ok to trust our nsAccessibilityService, just in case if it wants to call some "illegal" method. I wouldn't use Role() since it might require computations depending on the tree state. mFlags must be a perfect fit. So I would restrict it to passing mFlags as argument so nobody gets an idea to use Role() or anything else for context detection proposes. Sounds ok?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > I'm not sure I see why it makes more sense to pass it some sort of context
> > thing instead of just passing the parent directly.  I think we can probably
> > do most if not all of these checks just based on mFlags and or Role of the
> > accessible.
> 
> parent instead flags should be fine if we are ok to trust our
> nsAccessibilityService, just in case if it wants to call some "illegal"
> method. I wouldn't use Role() since it might require computations depending

well, trust ourselves not to allow people to add those things :)

I think we should be able to fix Role() so its just a member lookup, which I've been meaning to do for a while.  A least assuming we don't want to support accessibles changing there role.

> on the tree state. mFlags must be a perfect fit. So I would restrict it to
> passing mFlags as argument so nobody gets an idea to use Role() or anything
> else for context detection proposes. Sounds ok?

I'm fine with that if mFlags gives you everything you need, but we don't have that many free bits left in mFlags so we should be careful with adding more things just for this.

Comment 4

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

Can you give me an example of what kind of flag we might be talking about? Something like eImposter?

So for example an isolated div with role="cell" would have the imposter bit flipped so that we know we can't call table methods?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #4)
> Can you give me an example of what kind of flag we might be talking about?
> Something like eImposter?
> 
> So for example an isolated div with role="cell" would have the imposter bit
> flipped so that we know we can't call table methods?

no, this is about accessible creation, so if eListAccessible & mFlags is not true then child accessible should not be a list item.

Comment 6

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

OK thanks.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > parent instead flags should be fine if we are ok to trust our
> > nsAccessibilityService, just in case if it wants to call some "illegal"
> > method. I wouldn't use Role() since it might require computations depending
> 
> well, trust ourselves not to allow people to add those things :)

this is so true

> I think we should be able to fix Role() so its just a member lookup, which
> I've been meaning to do for a while.  A least assuming we don't want to
> support accessibles changing there role.

it makes sense but not sure how. file a bug?

> > on the tree state. mFlags must be a perfect fit. So I would restrict it to
> > passing mFlags as argument so nobody gets an idea to use Role() or anything
> > else for context detection proposes. Sounds ok?
> 
> I'm fine with that if mFlags gives you everything you need, but we don't
> have that many free bits left in mFlags so we should be careful with adding
> more things just for this.

we used 24 bits for now so we should be in a good shape still. If we run out of bits then we should start to be smarter (starting from idea that most of combinations described by mFlags aren't possible).

Comment 8

[alexander :surkov (:asurkov)]

Trev, would you be interested to take it?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: part1: preparations

Comment 10

[alexander :surkov (:asurkov)]

Attachment: part1: preparations

correct patch

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 681882 [details] [diff] [review]
part1: preparations

>+  nsAccTreeWalker(DocAccessible* aDoc, Accessible* aContext,
>+                  nsIContent* aContent, bool aWalkCache) :

it'd be nice to get rid of the nsIContent* arg and just get it from the accessible maybe, but that seems for the xul text field case.

>@@ -1516,8 +1516,7 @@
> {
>   // Search for accessible children starting from the document element since
>   // some web pages tend to insert elements under it rather than document body.
>-  nsAccTreeWalker walker(this, mDocument->GetRootElement(),

hm, we usually use nsCoreUtils::RoleContent() to get the content node for the document accessible right? so it seems odd to just use mDocument->GetRootElement() here, but replacing nsCoreUtils::RoleContent() might be nice.

>+bool
>+XULColorPickerAccessible::CanHaveAnonChildren()

why did you need to add this here?

btw should this method be const?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> Comment on attachment 681882 [details] [diff] [review]
> part1: preparations
> 
> >+  nsAccTreeWalker(DocAccessible* aDoc, Accessible* aContext,
> >+                  nsIContent* aContent, bool aWalkCache) :
> 
> it'd be nice to get rid of the nsIContent* arg and just get it from the
> accessible maybe, but that seems for the xul text field case.

For xul textbox and document, so we still need it. Anyway we need to rethink this code because bug 787858 doens't fit into existing approach.

> >@@ -1516,8 +1516,7 @@
> > {
> >   // Search for accessible children starting from the document element since
> >   // some web pages tend to insert elements under it rather than document body.
> >-  nsAccTreeWalker walker(this, mDocument->GetRootElement(),
> 
> hm, we usually use nsCoreUtils::RoleContent() to get the content node for
> the document accessible right? so it seems odd to just use
> mDocument->GetRootElement() here, but replacing nsCoreUtils::RoleContent()
> might be nice.

root element is not necessary role content element (for example html element and body element). Doing that we may loose accessible elements outside the body (that's valid case).

> >+bool
> >+XULColorPickerAccessible::CanHaveAnonChildren()
> 
> why did you need to add this here?

no need, I will remove them

> btw should this method be const?

It should be const method but I would rather try to replace it on flags approach, having it virtual doesn't make huge sense. But actually it may go away in nearest future at all (I keep in mind bug 787858).

Comment 13

[alexander :surkov (:asurkov)]

You know we can remove DocAccessible from constructor (yep) and mDoc from members and replace it on mContext->Document() (maybe). Sounds ok?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #13)
> You know we can remove DocAccessible from constructor (yep) and mDoc from
> members and replace it on mContext->Document() (maybe). Sounds ok?

I'll land this one as is and put that as follow up to avoid merging in my queue

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> You know we can remove DocAccessible from constructor (yep) and mDoc from
> members and replace it on mContext->Document() (maybe). Sounds ok?

that's probably fine, but it might be \epsilon slower if access to stuff near esp is faster than a random cached place in the heap.  Maybe just get the document in the constructor?

Comment 16

[alexander :surkov (:asurkov)]

ok

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 681882 [details] [diff] [review]
part1: preparations

http://hg.mozilla.org/integration/mozilla-inbound/rev/84c3118523eb

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84c3118523eb

Comment 19

[alexander :surkov (:asurkov)]

it's not finished yet

Comment 20

[alexander :surkov (:asurkov)]

Attachment: part2: use context for tables

it seems messy and it doesn't look less complicated to what we had, maybe we could reorg that later (like widget interface extension) but now I don't have good ideas

Comment 21

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

As long as it isn't messier I like the idea of passing a context. It might help in future cases and maybe we'll want more than an accessible, so we could create a AccessibleContext or something at that point.

(Reminds me of the style context passed in css frame construction)

Comment 22

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #21)
> As long as it isn't messier I like the idea of passing a context.

at the quick glance it is a right and neat idea but it doesn't save us from tree traversal (or at least its proposed implementation), so if we need to traverse trees up in either case then the idea (or, granted, its implementation) is not brilliant.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: part3: use context for lists

Comment 24

[Trevor Saunders (:tbsaunde)]

Comment on attachment 683939 [details] [diff] [review]
part2: use context for tables

approach seems generally fine, though I feel like the accessible creation stuff could be reorganized to make a good bit more sense.

>+    if (nsCoreUtils::IsHTMLTableHeader(aContent) &&
>+        aContext->Document()->HasAccessible(aContent->GetParentNode())) {

same as below, why only check there is an accessible when checking its the context is stronger, and faster

>-    Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc);
>+    Accessible* accessible =
>+      new HTMLOutputAccessible(aContent, aContext->Document());

use local var for doc to make things shorter?

>+      if (aContext->IsOfType(Accessible::eTableAccessible) &&

why not use AsTable() ?

>+          aContext->Document()->HasAccessible(aContent->GetParent())) {

why not do aContext->GetContent() == content->GetParent() ?

>+      // Accessible HTML table cell must be a child of accessible HTML table row.
>+      if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible)) {

why is it important its not a row of a different type?

I tend to think
<table>
<div role=row>
<td>foo</td>
</div></table>

is fine all as a table.

>+        if (parentFrame->GetType() == nsGkAtoms::tableOuterFrame &&
>+            aContext->Document()->HasAccessible(parentContent)) {

why not do aContext->GetContent() == parentContent ?

>+          newAcc = new HTMLTableRowAccessible(aContent, aContext->Document());

use a local var for the doc just to make things shorter?

>+    eTableAccessible = 1 << 24,
>+    eTableCellAccessible = 1 << 25,
>+    eTableRowAccessible = 1 << 26,

is there a good reason to use these instead of AsTable() / AsTableCell() / Role() or maybe NativeRole() ?

>+    eTableRowGroupAccessible = 1 << 27,

this doesn't seem to be used at all

>+    eXULTreeAccessible = 1 << 30

hm, this gets us really close to out of bits

>+class HTMLTableRowAccessible : public AccessibleWrap

inherit from EnumAccessible maybe?

>+  HTMLTableRowAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>+    AccessibleWrap(aContent, aDoc)
>+    { mFlags |= eTableRowAccessible | eHTMLTableRowAccessible; }

I've been sort of tempted to make Accessible's constructor take flags to add to mFlags before, I wonder if that would make more sense, or atleast doing that for EnumRoleAccessible then you wouldn't need this.

>+  ~HTMLTableRowAccessible() { }

virtual?

>+  NS_DECL_ISUPPORTS_INHERITED

why do you need to over ride this?

>-      testAccessibleTree("grid", accTree);
>+      //testAccessibleTree("grid", accTree);

why do you disable this?

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> approach seems generally fine, though I feel like the accessible creation
> stuff could be reorganized to make a good bit more sense.

that'd be nice

> >+    if (nsCoreUtils::IsHTMLTableHeader(aContent) &&
> >+        aContext->Document()->HasAccessible(aContent->GetParentNode())) {
> 
> same as below, why only check there is an accessible when checking its the
> context is stronger, and faster

having a table context doesn't mean that this th belongs to that header, for example, it can belong to presentational table.

> >-    Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc);
> >+    Accessible* accessible =
> >+      new HTMLOutputAccessible(aContent, aContext->Document());
> 
> use local var for doc to make things shorter?

locals for inlines aren't preferable but I can do that I think.

> 
> >+      if (aContext->IsOfType(Accessible::eTableAccessible) &&
> 
> why not use AsTable() ?

it's alternative to not existing IsTable() method, usually we have IsSomething() and AsSomething(). Moreover AsTable is virtual

> >+          aContext->Document()->HasAccessible(aContent->GetParent())) {
> 
> why not do aContext->GetContent() == content->GetParent() ?

that might do a trick

> >+      // Accessible HTML table cell must be a child of accessible HTML table row.
> >+      if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible)) {
> 
> why is it important its not a row of a different type?

> I tend to think
> <table>
> <div role=row>
> <td>foo</td>
> </div></table>
> 
> is fine all as a table.

they aren't really compatible if they are possible at all, sometimes people use crazy structures and we support some of them but I don't want to support any wild fantasy ;) at least unit it works and used in the wild

> >+    eTableAccessible = 1 << 24,
> >+    eTableCellAccessible = 1 << 25,
> >+    eTableRowAccessible = 1 << 26,
> 
> is there a good reason to use these instead of AsTable() / AsTableCell() /
> Role() or maybe NativeRole() ?

It seems we move away from Role() method usage and introduce AccessilbeType instead. AccessibleType is analogue of role.

> >+    eTableRowGroupAccessible = 1 << 27,
> 
> this doesn't seem to be used at all
> 
> >+    eXULTreeAccessible = 1 << 30
> 
> hm, this gets us really close to out of bits

bug 810572 will help

> >+class HTMLTableRowAccessible : public AccessibleWrap
> 
> inherit from EnumAccessible maybe?

extra member to keep a role? virtual Role() method works fine.

> >+  HTMLTableRowAccessible(nsIContent* aContent, DocAccessible* aDoc) :
> >+    AccessibleWrap(aContent, aDoc)
> >+    { mFlags |= eTableRowAccessible | eHTMLTableRowAccessible; }
> 
> I've been sort of tempted to make Accessible's constructor take flags to add
> to mFlags before, I wonder if that would make more sense, or atleast doing
> that for EnumRoleAccessible then you wouldn't need this.

I'm not sure, I thought about that, but ended up with that version

> >+  NS_DECL_ISUPPORTS_INHERITED
> 
> why do you need to over ride this?

we do that if we inherit from wrap class to not conflict with MSAA QueryInterface

Comment 26

[alexander :surkov (:asurkov)]

Attachment: part2: use context for tables (v2)

Comment 27

[Trevor Saunders (:tbsaunde)]

> > >+    if (nsCoreUtils::IsHTMLTableHeader(aContent) &&
> > >+        aContext->Document()->HasAccessible(aContent->GetParentNode())) {
> > 
> > same as below, why only check there is an accessible when checking its the
> > context is stronger, and faster
> 
> having a table context doesn't mean that this th belongs to that header, for
> example, it can belong to presentational table.

example markup? or maybe I don't understand what your trying to do here.

> > >-    Accessible* accessible = new HTMLOutputAccessible(aContent, aDoc);
> > >+    Accessible* accessible =
> > >+      new HTMLOutputAccessible(aContent, aContext->Document());
> > 
> > use local var for doc to make things shorter?
> 
> locals for inlines aren't preferable but I can do that I think.

I tend to not really care one way or the other and go for what's shorter.

> > 
> > >+      if (aContext->IsOfType(Accessible::eTableAccessible) &&
> > 
> > why not use AsTable() ?
> 
> it's alternative to not existing IsTable() method, usually we have
> IsSomething() and AsSomething(). Moreover AsTable is virtual

iirc you suggested adding IsTable() that called AsTable() or something.

> > I tend to think
> > <table>
> > <div role=row>
> > <td>foo</td>
> > </div></table>
> > 
> > is fine all as a table.
> 
> they aren't really compatible if they are possible at all, sometimes people
> use crazy structures and we support some of them but I don't want to support
> any wild fantasy ;) at least unit it works and used in the wild

true, I just wonder if its worth extra work to change what we do in crazy case.

> > >+    eTableAccessible = 1 << 24,
> > >+    eTableCellAccessible = 1 << 25,
> > >+    eTableRowAccessible = 1 << 26,
> > 
> > is there a good reason to use these instead of AsTable() / AsTableCell() /
> > Role() or maybe NativeRole() ?
> 
> It seems we move away from Role() method usage and introduce AccessilbeType
> instead. AccessibleType is analogue of role.

true, it would just be nice to have less dupplication.

> > >+    eTableRowGroupAccessible = 1 << 27,
> > 
> > this doesn't seem to be used at all
> > 
> > >+    eXULTreeAccessible = 1 << 30
> > 
> > hm, this gets us really close to out of bits
> 
> bug 810572 will help

ok, are you planning to work on it?

> > >+class HTMLTableRowAccessible : public AccessibleWrap
> > 
> > inherit from EnumAccessible maybe?
> 
> extra member to keep a role? virtual Role() method works fine.

are you argueing to get rid EnumAccessible then? or maybe we should make it a class templated on the role to return?

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #27)
> > > >+    if (nsCoreUtils::IsHTMLTableHeader(aContent) &&
> > > >+        aContext->Document()->HasAccessible(aContent->GetParentNode())) {
> > > 
> > > same as below, why only check there is an accessible when checking its the
> > > context is stronger, and faster
> > 
> > having a table context doesn't mean that this th belongs to that header, for
> > example, it can belong to presentational table.
> 
> example markup? or maybe I don't understand what your trying to do here.

this one is:

<table>
  <tr><th>should have an accessible</th>
  <tr><td>
    <table>
      <tr><th>shouldn't have an accessible</th></tr>
    </table>
  </td></tr>
</table>

> > > use local var for doc to make things shorter?
> > 
> > locals for inlines aren't preferable but I can do that I think.
> 
> I tend to not really care one way or the other and go for what's shorter.

I changed it already :)

> > > 
> > > >+      if (aContext->IsOfType(Accessible::eTableAccessible) &&
> > > 
> > > why not use AsTable() ?
> > 
> > it's alternative to not existing IsTable() method, usually we have
> > IsSomething() and AsSomething(). Moreover AsTable is virtual
> 
> iirc you suggested adding IsTable() that called AsTable() or something.

well, we always move back and forward, mFlags approach should work too. Actually I feel a little mess about recently added things. I think we can sort out things when we go ahead.

> > > >+    eTableRowGroupAccessible = 1 << 27,
> > > 
> > > this doesn't seem to be used at all

btw, I forgot to address it in latest patch

> > > >+    eXULTreeAccessible = 1 << 30
> > > 
> > > hm, this gets us really close to out of bits
> > 
> > bug 810572 will help
> 
> ok, are you planning to work on it?

I think I do

> > > >+class HTMLTableRowAccessible : public AccessibleWrap
> > > 
> > > inherit from EnumAccessible maybe?
> > 
> > extra member to keep a role? virtual Role() method works fine.
> 
> are you argueing to get rid EnumAccessible then?

I'm not sure I ever liked it

> or maybe we should make it
> a class templated on the role to return?

that seems reasonable, file a bug?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #28)

> > example markup? or maybe I don't understand what your trying to do here.
> 
> this one is:

it should be:

<table>
  <tr><th>should have an accessible</th>
  <tr><td>
    <table role="presentation">
      <tr><th>shouldn't have an accessible</th></tr>
    </table>
  </td></tr>
</table>

Comment 30

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #29)
> (In reply to alexander :surkov from comment #28)

in that case how is the context accessible for the inaccessible th a table row accessible?  The one node I see there that should be a table row is the outer tr which has a accessible td child so we won't search the td's kids for accessible children of the tr.

if the accessible we're looking for children of is below the accessible for the td then the context won't be a table row so of course we won't create a accessible for the th.

The only case I can think of where were looking at the content parent might matter is something like
<tr>
<div>
<td>not table cell table cell</td>
</div>
</tr>

where the div isn't going to be accessible so we search its kids for accessible children.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #30)
> (In reply to alexander :surkov from comment #29)
> > (In reply to alexander :surkov from comment #28)
> 
> in that case how is the context accessible for the inaccessible th a table
> row accessible?  The one node I see there that should be a table row is the
> outer tr which has a accessible td child so we won't search the td's kids
> for accessible children of the tr.

there are wild cases, they could do

<div role="table">
  <div role="row">
    <table role="presentation">
      <tr><th>not accessible header</th></tr>
    </table>
  </div>
</div>

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #31)
> (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> > (In reply to alexander :surkov from comment #29)
> > > (In reply to alexander :surkov from comment #28)
> > 
> > in that case how is the context accessible for the inaccessible th a table
> > row accessible?  The one node I see there that should be a table row is the
> > outer tr which has a accessible td child so we won't search the td's kids
> > for accessible children of the tr.
> 
> there are wild cases, they could do
> 
> <div role="table">
>   <div role="row">
>     <table role="presentation">
>       <tr><th>not accessible header</th></tr>
>     </table>
>   </div>
> </div>

sure, but that doesn't seem to be a case where doc->HasAccessible(th->GetParent()) works, but aContext->GetContent() == th->GetParent() doesn't

Comment 33

[alexander :surkov (:asurkov)]

it seems both of them will return false since th->GetParent() is tr which is not accessible. so they both seems working.

Comment 34

[Trevor Saunders (:tbsaunde)]

Comment on attachment 685611 [details] [diff] [review]
part2: use context for tables (v2)

>+    if (roleMapEntry) {
>+      // Create pure ARIA grid/treegrid related accessibles if they weren't used
>+      // on accessible HTML table elements.
>+      if ((roleMapEntry->accTypes & Accessible::eTableCellAccessible)) {
>+        if (aContext->IsOfType(Accessible::eTableRowAccessible) &&
>+            (frame->AccessibleType() != eHTMLTableCellAccessible ||
>+             !document->HasAccessible(content->GetParentNode()))) {

why not use aContext->GetContent() != parentNode here?

>   if (tag == nsGkAtoms::abbr ||
>       tag == nsGkAtoms::acronym ||
>       tag == nsGkAtoms::blockquote ||
>       tag == nsGkAtoms::dd ||
>       tag == nsGkAtoms::form ||
>       tag == nsGkAtoms::h1 ||
>       tag == nsGkAtoms::h2 ||
>       tag == nsGkAtoms::h3 ||
>       tag == nsGkAtoms::h4 ||
>       tag == nsGkAtoms::h5 ||
>       tag == nsGkAtoms::h6 ||
>       tag == nsGkAtoms::q) {

nit, mind making this take fewer lines while your here?

>+      if (aContext->IsOfType(Accessible::eTableAccessible) &&
>+          aContext->Document()->HasAccessible(aContent->GetParent())) {

use aContext->GetContent() == aContent->GetParent() ?

>+      // Accessible HTML table cell must be a child of accessible HTML table row.
>+      if (aContext->IsOfType(Accessible::eHTMLTableRowAccessible))
>+        newAcc = new HTMLTableCellAccessibleWrap(aContent, document);
>       break;

nit, blank line pls

>+    eTableRowGroupAccessible = 1 << 27,

remember to remove it :)

don't you need to set eTableCellAccessible in HTMLTableCellAccessible XULTreeGridCellAccessible and set the appropriate flag in each of the classes for XULListBoxAccessible when its being a table?

>+      accTree = {
>+        role: ROLE_TABLE,
>+        children: [
>+          { // div@role="row"
>+            role: ROLE_ROW,
>+            tagName: "DIV",
>+            children: [
>+              { // caption text leaf
>+                role: ROLE_TEXT_LEAF,
>+                name: "caption",
>+                children: [ ]
>+              },
>+              { // th text leaf
>+                role: ROLE_TEXT_LEAF,
>+                name: "header1",
>+                children: [ ]
>+              },
>+              { // td@role="columnheader"
>+                role: ROLE_COLUMNHEADER,
>+                name: "header2",
>+                children: [ { TEXT_LEAF: [ ] } ]
>+              }
>+            ]
>+          },
>+          { // tr@role="row"
>+            role: ROLE_ROW,
>+            tagName: "TR",
>+            children: [
>+              { // td text leaf
>+                role: ROLE_TEXT_LEAF,
>+                name: "cell1",
>+                children: [ ]
>+              },
>+              { // td@role="gridcell"
>+                role: ROLE_GRID_CELL,
>+                name: "cell2",
>+                children: [ { TEXT_LEAF: [ ] } ]
>+              }
>+            ]
>+          },
>+          { // div@role="row"
>+            role: ROLE_ROW,
>+            children: [
>+              { // div@role="gridcell"
>+                role: ROLE_GRID_CELL,
>+                children: [
>+                  { // text leaf from presentational table
>+                    role: ROLE_TEXT_LEAF,
>+                    name: "cell3",
>+                    children: [ ]
>+                  },
>+                ]
>+              }
>+            ]
>+          },
>+          { // div@role="row"
>+            role: ROLE_ROW,
>+            children: [
>+              { // div@role="gridcell"
>+                role: ROLE_GRID_CELL,
>+                children: [
>+                  { // table
>+                    role: ROLE_TABLE,
>+                    children: [
>+                      { // tr
>+                        role: ROLE_ROW,
>+                        children: [
>+                          { // td
>+                            role: ROLE_CELL,
>+                            children: [
>+                              { // caption text leaf of presentational table
>+                                 role: ROLE_TEXT_LEAF,
>+                                 name: "caption",
>+                                 children: [ ]
>+                              },
>+                              { // td text leaf of presentational table
>+                                role: ROLE_TEXT_LEAF,
>+                                name: "cell4",
>+                                children: [ ]
>+                              }
>+                            ]
>+                          }
>+                        ]
>+                      }
>+                    ]
>+                  }
>+                ]
>+              }
>+            ]
>+          }
>+        ]
>+      };

that's a *huge* test that I'm fairly sure really wants to be a couple seperate tree tests, mind too much making it so?  I think it'll help debuging future test failures if the test cases that fail are smaller.

>diff --git a/accessible/tests/mochitest/tree/test_brokencontext.html>     var tree =
>       { PUSHBUTTON: [ // table
>-        { NOTHING: [ // tbody

any idea how this used to pass?

r=me but it might be nice to look at the changes you make

Comment 35

[Trevor Saunders (:tbsaunde)]

Comment on attachment 684302 [details] [diff] [review]
part3: use context for lists

>+        } else if (frame->AccessibleType() == eHTMLLiAccessible ||
>+                   content->Tag() == nsGkAtoms::dt ||
>+                   content->Tag() == nsGkAtoms::li ||
>+                   content->Tag() == nsGkAtoms::dd) {

why not switch the order of these checks? I'd really expect gettingthe tag is faster than the virtual call that may do some work...

>+  if (aContext->IsOfType(Accessible::eListAccessible)) {
>+    // If list item is a child of accessible list then create an accessible for
>+    // it unconditionally by tag name. nsBlockFrame creates the list item
>+    // accessible for other elements styled as list items.
>+    if (aContext->Document()->HasAccessible(aContent->GetParent())) {

I'm not completely sure something like
<ol>
<div>
<li>item</li>
</div>
</ol>
is bad, but if we're going to do this why not do aContext->GetContent() == content->GetParent()

>+      if (aContext->IsOfType(Accessible::eListAccessible) &&
>+          aContext->Document()->HasAccessible(aContent->GetParent())) {

same, use aContext->GetContent() here too.

>+      if (aContent->Tag() == nsGkAtoms::dt || aContent->Tag() == nsGkAtoms::dd)
>+        break;
>       newAcc = new HyperTextAccessibleWrap(aContent, aContext->Document());
>       break;

why not invert the test see we only have one break?

nit, blank line after the if pls

Comment 36

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> Comment on attachment 685611 [details] [diff] [review]
> part2: use context for tables (v2)

> >+            (frame->AccessibleType() != eHTMLTableCellAccessible ||
> >+             !document->HasAccessible(content->GetParentNode()))) {
> 
> why not use aContext->GetContent() != parentNode here?

ok

> >       tag == nsGkAtoms::h6 ||
> >       tag == nsGkAtoms::q) {
> 
> nit, mind making this take fewer lines while your here?

I think I like the way they are (easy to skim through them), so I wouldn't change it.

> >+      if (aContext->IsOfType(Accessible::eTableAccessible) &&
> >+          aContext->Document()->HasAccessible(aContent->GetParent())) {
> 
> use aContext->GetContent() == aContent->GetParent() ?

ok

> nit, blank line pls

I don't mind

> >+    eTableRowGroupAccessible = 1 << 27,
> 
> remember to remove it :)

I did that already :)

> don't you need to set eTableCellAccessible in HTMLTableCellAccessible
> XULTreeGridCellAccessible and set the appropriate flag in each of the
> classes for XULListBoxAccessible when its being a table?

That'd be correct think but I would leave it until we have a case when we need it, especially, XULListboxAccessible that can be a table and can be a list.

> that's a *huge* test that I'm fairly sure really wants to be a couple
> seperate tree tests, mind too much making it so?  I think it'll help
> debuging future test failures if the test cases that fail are smaller.

makes sense

> >diff --git a/accessible/tests/mochitest/tree/test_brokencontext.html>     var tree =
> >       { PUSHBUTTON: [ // table
> >-        { NOTHING: [ // tbody
> 
> any idea how this used to pass?

I think we ended up with gEmptyLandmark as roleMapEntry and created generic accessible because of it. Thank you for bringing this up. I hate to figure out motivation for not evident side changes after years.

> r=me but it might be nice to look at the changes you make

sure, I'll file a patch

Comment 37

[alexander :surkov (:asurkov)]

Attachment: part2: use context for tables (v3)

Comment 38

[alexander :surkov (:asurkov)]

Attachment: part3: use context for lists (v2)

Comment 39

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #37)
> Created attachment 686437 [details] [diff] [review]
> part2: use context for tables (v3)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2e0e82aa7d

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f2e0e82aa7d

Comment 41

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #38)
> Created attachment 686446 [details] [diff] [review]
> part3: use context for lists (v2)

http://hg.mozilla.org/integration/mozilla-inbound/rev/791ce5d073bc

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/791ce5d073bc