Closed Bug 804461 Opened 12 years ago Closed 11 years ago

build the context dependent tree

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
(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.
(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?
(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.
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?
(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.
(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).
Trev, would you be interested to take it?
Attached patch part1: preparations (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #681881 - Flags: review?(trev.saunders)
correct patch
Attachment #681881 - Attachment is obsolete: true
Attachment #681881 - Flags: review?(trev.saunders)
Attachment #681882 - Flags: review?(trev.saunders)
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?
Attachment #681882 - Flags: review?(trev.saunders) → review+
(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).
You know we can remove DocAccessible from constructor (yep) and mDoc from members and replace it on mContext->Document() (maybe). Sounds ok?
(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
(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?
ok
https://hg.mozilla.org/mozilla-central/rev/84c3118523eb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
it's not finished yet
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Mac OS X → All
Hardware: x86 → All
Attached patch part2: use context for tables (obsolete) — Splinter Review
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
Attachment #683939 - Flags: review?(trev.saunders)
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)
(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.
Attached patch part3: use context for lists (obsolete) — Splinter Review
Attachment #684302 - Flags: review?(trev.saunders)
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?
(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
Attachment #683939 - Attachment is obsolete: true
Attachment #683939 - Flags: review?(trev.saunders)
Attachment #685611 - Flags: review?(trev.saunders)
> > >+    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?
(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?
(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>
(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.
(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>
(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
it seems both of them will return false since th->GetParent() is tr which is not accessible. so they both seems working.
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
Attachment #685611 - Flags: review?(trev.saunders) → review+
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
Attachment #684302 - Flags: review?(trev.saunders) → review+
(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
Attachment #685611 - Attachment is obsolete: true
Attachment #684302 - Attachment is obsolete: true
(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
Whiteboard: [leave open]
(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
Whiteboard: [leave open]
Target Milestone: mozilla19 → mozilla20
https://hg.mozilla.org/mozilla-central/rev/791ce5d073bc
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Depends on: 834120
You need to log in before you can comment on or make changes to this bug.