360184 - Accessible row/column and row/column header of tables are incorrect

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Willie Walker]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061109 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061109 Minefield/3.0a1

The accessible row and column of a table should be 0-based values that accurately reflect the row and column of a table cell.  The row and column headers, if present, should also be obtainable.

Reproducible: Always

Steps to Reproduce:
1. Load the attached URL in Firefox and enable caret browsing
2. Run the attached app
3. Arrow through the table cells and pay attention to the output

Actual Results:  
The row/col for each of the table cells is something like this:

       Col 0:  Col 1:  Col 2:
Row 0: 0/1     0/2     1/0
Row 1: 1/1     1/2     2/0
Row 2: 2/1     2/2     3/0
Row 3: 3/1     3/2     4/0

In addition, the column header exists in HTML, but doesn't seem to be able to be found via the AT-SPI.

Expected Results:  
The row/col for each of the table cells is something like this:

       Col 0:  Col 1:  Col 2:
Row 0: 0/0     0/1     0/2
Row 1: 1/0     1/1     1/2
Row 2: 2/0     2/1     2/2
Row 3: 3/0     3/1     3/2

In addition, the column header should be able to be discovered.

The relevant code in the test app looks like this:

    index = event.source.getIndexInParent()
    row = table.getRowAtIndex(index)
    col = table.getColumnAtIndex(index)
    rowHeader = table.getRowDescription(row)
    colHeader = table.getColumnDescription(col)
    print "row=%d, col=%d rowHeader='%s', colHeader='%s'" \
          % (row, col, rowHeader, colHeader)

Comment 1

[Willie Walker]

Attachment: Simple table with headers

Comment 2

[Willie Walker]

Attachment: Simple test case to show problem. See bug description for more info.

Comment 3

[Evan Yan]

confirmed.
the table's children are in a mess.

Comment 4

[Evan Yan]

Attachment: patch

it's a regression caused by the patch of bug 358720

Comment 5

[Evan Yan]

for missing row and column header, it's because we haven't implemented nsHTMLTableAccessible::GetRowDescription() and GetColumnDescription() right now.
see
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTableAccessible.cpp#483

I can understand that column description is what's in tag <th>, but what should row description be?

Comment 6

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

Because Bug 358720 caused this, blocking1.8.1.1?

Comment 7

[Willie Walker]

> I can understand that column description is what's in tag <th>, but what should
> row description be?

I don't know.  :-)  The AT-SPI IDL is a bit vague on what the return value should be if a header doesn't exist.  I'm guessing a NULL would be the thing to do since it would be nice to distinguish between an empty header and no header at all.

Comment 8

[Daniel Veditz [:dveditz]]

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

approved for 1.8 branch, a=dveditz for drivers

Comment 9

[Ginn Chen]

landed trunk and 1.8 branch

Comment 10

[Jay Patel [:jay]]

Willie:  Would you mind verifying this fix with the latest 2.0.0.1 candidate builds?  I tried your testcase and was having trouble getting the .py file to work on Mac (haven't used python on it before):

Traceback (most recent call last):
  File "bug_360184.py", line 29, in ?
    import bonobo
ImportError: No module named bonobo

Figured it would be quicker to ask you to run the test yourself.  So if you can, please do and replace the fixed1.8.1.1 keyword with verified1.8.1.1.  Thanks!

Comment 11

[Willie Walker]

We've been convinced by the Mozilla team to give up on accessibility support for FF2, and we've all agreed to instead focus on FF3.  I just tried to reproduce the bug with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061210 Minefield/3.0a1, and it still exists.

Comment 12

[Evan Yan]

Willie, the patch is landed on 1.8 branch, because it fixed a regression on 1.8 branch, see comment #4.

For this bug, the patch, which fixes the regression, resolved part of the problem. yes, there is still some problem, it was my mistake not noticing that.

I'll work on this, and I think the further patch will not land on 1.8 branch.

Comment 13

[Willie Walker]

Thanks Evan!  BTW, we worked around this in Orca, so this is not such a high priority bug for us.  Other bugs, such as crashing when attempting to do a find (https://bugzilla.mozilla.org/show_bug.cgi?id=362401) and the various bugs regarding caret handling (https://bugzilla.mozilla.org/show_bug.cgi?id=363198, https://bugzilla.mozilla.org/show_bug.cgi?id=363214, https://bugzilla.mozilla.org/show_bug.cgi?id=363200) are higher priority problems for us.

Comment 14

[Evan Yan]

HTML table has a child for <caption>, the child can have different index in HTML table's children, according to where the <caption> is in HTML code.

We didn't think about the <caption> in GetRowAtIndex() and GetColumnAtIndex(), that's the cause of the problem.

Comment 15

[Evan Yan]

I'm back to this bug.

the problem is, HTML script is not welformed, which make it difficult to deal with the <caption> node.

The tag <caption> could be placed anywhere inside <table>, say between <tbody> and the first <tr>, or between two <tr>s, or even between some <td>s. Then the DOM node of <caption> can also be anywhere in the DOM tree of <table>. Our accessible tree is effect as well. We can't predict which child of the table node is the caption, so we can't calculate the cell's row/column index from its child index.
And there could be multiple <caption>s.

one possible solution is, override CacheChildren() in nsHTMLTableAccessible.cpp, move <caption> accessibles to be children of a captions accessible as follows.

table
--captions
----caption1
----caption2
--cell1
--cell2
....
--celln

Comment 16

[Evan Yan]

Attachment: override CacheChildren in HTMLTableAccessible

current accessible nodes structure:

table
--caption (if any)
--cell1
--cell2
...
--celln

We deal with only one caption, because only one caption can be got from DOM Table interface, and only one caption will be rendered in screen also.

The patch also fix a bug of nsHTMLTableAccessible::GetCaption()

Comment 17

[Aaron Leventhal]

Comment on attachment 259192 [details] [diff] [review]
override CacheChildren in HTMLTableAccessible

Until April 4 please use Ginn or Surkov for reviews.

Comment 18

[Evan Yan]

Surkov, got a chance to look at this?

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 259192 [details] [diff] [review]
override CacheChildren in HTMLTableAccessible

>+  PRBool mHasCaption;

Probably it's better to keep this like a method, it allows to decrease memory using.

> nsHTMLTableAccessible::nsHTMLTableAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell):
> nsAccessibleWrap(aDomNode, aShell)
> { 
> }

If you would like to keep hasCaption as a member then you should initialize it here.

>+void nsHTMLTableAccessible::CacheChildren()
>+{

Probably the next will be looks more simplier.
1) get caption element
2) run through nodes if current traversed node is caption then setFirstChild()
3) if there is not caption node and current traversed node is the first child then setFirstChild
4) set parent

>+      if (content && content->Tag() == nsAccessibilityAtoms::caption) {

What's about namespace check?

>@@ -172,17 +227,21 @@ nsHTMLTableAccessible::GetCaption(nsIAcc
>   nsCOMPtr<nsIAccessibilityService>
>     accService(do_GetService("@mozilla.org/accessibilityService;1"));
>   NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE);

You can use here nsAccessNode::GetAccService() I guess.

>   accService->GetCachedAccessible(captionNode, mWeakShell, aCaption);
>   if (*aCaption)
>     return NS_OK;
> 
>-  accService->CreateHyperTextAccessible(captionNode, aCaption);
>+  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(captionNode));
>+  NS_ENSURE_TRUE(presShell && content, NS_ERROR_FAILURE);
>+  nsIFrame* frame = presShell->GetPrimaryFrameFor(content);
>+  accService->CreateHyperTextAccessible(frame, aCaption);
>   nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(*aCaption));
>   return accessNode ? accessNode->Init() : NS_ERROR_FAILURE;

I wonder is it better to call nsIAccessibilityService::getAccessible... instead this?

> }
> 
> NS_IMETHODIMP
> nsHTMLTableAccessible::SetCaption(nsIAccessible *aCaption)
> {
>   nsresult rv = NS_OK;
>@@ -406,47 +465,56 @@ nsHTMLTableAccessible::GetIndexAt(PRInt3
> 
>   nsresult rv = NS_OK;
> 
>   PRInt32 columns;
>   rv = GetColumns(&columns);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   *_retval = aRow * columns + aColumn;
>+  if (mHasCaption) {
>+    (*_retval)++;
>+  }
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsHTMLTableAccessible::GetColumnAtIndex(PRInt32 aIndex, PRInt32 *_retval)
> {
>   NS_ENSURE_ARG_POINTER(_retval);
> 
>   nsresult rv = NS_OK;
> 
>   PRInt32 columns;
>   rv = GetColumns(&columns);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (mHasCaption) {
>+    aIndex--;
>+  }

I wonder why? How does it depend on caption presence?

Comment 20

[Evan Yan]

Thanks for review, please see my comments in line.

(In reply to comment #19)
> (From update of attachment 259192 [details] [diff] [review])
> >+  PRBool mHasCaption;
> 
> Probably it's better to keep this like a method, it allows to decrease memory
> using.
> 
If we use a method, we have to check whether mFirstChild is caption every time.

> > nsHTMLTableAccessible::nsHTMLTableAccessible(nsIDOMNode* aDomNode, nsIWeakReference* aShell):
> > nsAccessibleWrap(aDomNode, aShell)
> > { 
> > }
> 
> If you would like to keep hasCaption as a member then you should initialize it
> here.
> 
will do

> >+void nsHTMLTableAccessible::CacheChildren()
> >+{
> 
> Probably the next will be looks more simplier.
> 1) get caption element
> 2) run through nodes if current traversed node is caption then setFirstChild()
> 3) if there is not caption node and current traversed node is the first child
> then setFirstChild
> 4) set parent
> 
When traversing, there could be multiple caption nodes.

What I do in the patch:

 DOM tree                       Accessible Tree
table                            table
--cell1                          --caption1
--caption1                       --cell1
--cell2             ==>          --cell2
--caption2                       ....
....                             --celln
--celln                      (caption2 is thrown away, see comment #16)

> >+      if (content && content->Tag() == nsAccessibilityAtoms::caption) {
> 
> What's about namespace check?
> 
we are in nsHTMLTableAccessible, do we still need to check namespace?

> >@@ -172,17 +227,21 @@ nsHTMLTableAccessible::GetCaption(nsIAcc
> >   nsCOMPtr<nsIAccessibilityService>
> >     accService(do_GetService("@mozilla.org/accessibilityService;1"));
> >   NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE);
> 
> You can use here nsAccessNode::GetAccService() I guess.
> 
will do.

> >   accService->GetCachedAccessible(captionNode, mWeakShell, aCaption);
> >   if (*aCaption)
> >     return NS_OK;
> > 
> >-  accService->CreateHyperTextAccessible(captionNode, aCaption);
> >+  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
> >+  nsCOMPtr<nsIContent> content(do_QueryInterface(captionNode));
> >+  NS_ENSURE_TRUE(presShell && content, NS_ERROR_FAILURE);
> >+  nsIFrame* frame = presShell->GetPrimaryFrameFor(content);
> >+  accService->CreateHyperTextAccessible(frame, aCaption);
> >   nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(*aCaption));
> >   return accessNode ? accessNode->Init() : NS_ERROR_FAILURE;
> 
> I wonder is it better to call nsIAccessibilityService::getAccessible... instead
> this?
> 
The former code uses CreateHyperTextAccessible() but passed a wrong argument. I fixed the bug in the patch. I think it's OK to call CreateHyperTextAccessible, so didn't change it.

> > }
> > 
> > NS_IMETHODIMP
> > nsHTMLTableAccessible::SetCaption(nsIAccessible *aCaption)
> > {
> >   nsresult rv = NS_OK;
> >@@ -406,47 +465,56 @@ nsHTMLTableAccessible::GetIndexAt(PRInt3
> > 
> >   nsresult rv = NS_OK;
> > 
> >   PRInt32 columns;
> >   rv = GetColumns(&columns);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   *_retval = aRow * columns + aColumn;
> >+  if (mHasCaption) {
> >+    (*_retval)++;
> >+  }
> > 
> >   return NS_OK;
> > }
> > 
> > NS_IMETHODIMP
> > nsHTMLTableAccessible::GetColumnAtIndex(PRInt32 aIndex, PRInt32 *_retval)
> > {
> >   NS_ENSURE_ARG_POINTER(_retval);
> > 
> >   nsresult rv = NS_OK;
> > 
> >   PRInt32 columns;
> >   rv = GetColumns(&columns);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+  if (mHasCaption) {
> >+    aIndex--;
> >+  }
> 
> I wonder why? How does it depend on caption presence?
> 
if there is caption, the index of caption accessible is 0, index of cell accessibles will start from 1.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> > >-  accService->CreateHyperTextAccessible(captionNode, aCaption);
> > >+  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
> > >+  nsCOMPtr<nsIContent> content(do_QueryInterface(captionNode));
> > >+  NS_ENSURE_TRUE(presShell && content, NS_ERROR_FAILURE);
> > >+  nsIFrame* frame = presShell->GetPrimaryFrameFor(content);
> > >+  accService->CreateHyperTextAccessible(frame, aCaption);
> > >   nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(*aCaption));
> > >   return accessNode ? accessNode->Init() : NS_ERROR_FAILURE;
> > 
> > I wonder is it better to call nsIAccessibilityService::getAccessible... instead
> > this?
> > 
> The former code uses CreateHyperTextAccessible() but passed a wrong argument. I
> fixed the bug in the patch. I think it's OK to call CreateHyperTextAccessible,
> so didn't change it.

Yes, but I'm not big fan to spray out the logic of accessiblity service via accessible module. Actually getAccessible() makes this work though I get getAccessible() is too big for this. Can you think some way to pass it?

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #20)

> > What's about namespace check?
> > 
> we are in nsHTMLTableAccessible, do we still need to check namespace?

It's better to be calm. Just add content->IsNodeOfType(nsINode::eHTML).

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 259192 [details] [diff] [review]
override CacheChildren in HTMLTableAccessible

r=me with my answered/fixed comments above.

Comment 24

[Evan Yan]

Attachment: patch to check in

with Surkov's comments addressed.

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 259660 [details] [diff] [review]
patch to check in


> 
>-  nsCOMPtr<nsIAccessibilityService>
>-    accService(do_GetService("@mozilla.org/accessibilityService;1"));
>-  NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE);
>-
>-  accService->GetCachedAccessible(captionNode, mWeakShell, aCaption);
>-  if (*aCaption)
>-    return NS_OK;
>-
>-  accService->CreateHyperTextAccessible(captionNode, aCaption);
>-  nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(*aCaption));
>-  return accessNode ? accessNode->Init() : NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
>+  nsIFrame* frame = nsnull;
>+  PRBool isHidden;
>+  return GetAccService()->GetAccessible(captionNode, presShell, mWeakShell, &frame, &isHidden, aCaption);

As I said it's not good too because it's performance issue. Actually we don't need to check anything if we know what type of accessible we want. Moreover theoretically GetAccessible() may return something different we think of. I'd say leave it as it was but file new bug since I assume it's not unique place where we spread out accessibility service logig via whole ally code (I mean the checking for cached accessible and nsIAccessNode::Init() call).

Comment 26

[Ginn Chen]

Checking in nsHTMLTableAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.cpp,v  <--  nsHTMLTableAccessible.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in nsHTMLTableAccessible.h;
/cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.h,v  <--  nsHTMLTableAccessible.h
new revision: 1.23; previous revision: 1.22
done

Comment 27

[Joanmarie Diggs]

Attachment: test case

getRowAtIndex() and getColumnAtIndex() are still not quite right when a table includes at least one cell that spans multiple rows and/or columns.  I've attached this sample as a test case.  I'll attach the output and explanation next.

Comment 28

[Joanmarie Diggs]

Attachment: output (described below)

The attached shows the output from examining the previously-attached test case.  For each cell, the first line shows the results when accessing a cell's details via its coordinates.  The second shows the coordinates returned via getRowAtIndex() and getColumnAtIndex().  The presence of a cell which spans multiple cells throws off all of the cells which follow.

Comment 29

[Evan Yan]

Good catch!

Our current indexing model for HTML table cell doesn't support rowspan/colspan. I'd like to discuss this in a separated bug.

Comment 30

[Evan Yan]

bug 375934 filed for rowspan/colspan.

Comment 31

[Firefox Bug Husbandry Bot]

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