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)

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

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

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?

Because Bug 358720 caused this, blocking1.8.1.1?

> 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 on attachment 245432 [details] [diff] [review] patch approved for 1.8 branch, a=dveditz for drivers

landed trunk and 1.8 branch

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!

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.

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.

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.

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.

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

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 on attachment 259192 [details] [diff] [review] override CacheChildren in HTMLTableAccessible Until April 4 please use Ginn or Surkov for reviews.

Surkov, got a chance to look at this?

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?

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.

(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?

(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 on attachment 259192 [details] [diff] [review] override CacheChildren in HTMLTableAccessible r=me with my answered/fixed comments above.

with Surkov's comments addressed.

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).

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

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.

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.

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.

bug 375934 filed for rowspan/colspan.