430758 - GetAccessible() could be fooled by additional table ancestor

Status: RESOLVED FIXED

Description

[Evan Yan]

Given a testcase like this

  <table>
  <tr>
  <td style="display:block">
    <table style="display:inline">
      <tr><td>cell0</td></tr>
    </table>
  </td>
  <td>cell1</td>
  </tr>
  </table>

GetAccessible() could be fooled by the outside table, so that create accessible for cell0 wrongly.

Comment 1

[Evan Yan]

Created attachment 317662 [details] [diff] [review]
patch

Comment 2

[Marco Zehe (:MarcoZ)]

Evan, is tableContent checked for validity (not NULL) before you use it in this patch? I notice there's a check for tableContent being not NULL below, but are we sure that we don't introduce a potential crasher here?

Comment 3

[Evan Yan]

Marco, it's inside the while loop

1503       while ((tableContent = tableContent->GetParent()) != nsnull) {

Comment 4

[alexander :surkov (:asurkov)]

Evan, could you look why we introduced the check for tableCellFrame and why we didn't use table tag name check? Was there special reason for this?
How will we deal with nested td elements?
Can you attach mochitest for this?

Comment 5

[alexander :surkov (:asurkov)]

We introduced that in bug 404881 before what we used "table" tag name

Comment 6

[alexander :surkov (:asurkov)]

Btw, would be nice to put into our mochitests the testcase from bug 409009

Comment 7

[alexander :surkov (:asurkov)]

I don't see special reason to check tableCellFrame though we need to check how we should deal with nested table cells.

Comment 8

[Evan Yan]

Created attachment 318360 [details] [diff] [review]
add mochitest

Comment 9

[alexander :surkov (:asurkov)]

Evan, could you add some tests for nested table cells as well like

<table>
  <tr><td>hello<td>hello</td></td></tr>
</table>

how we should/do deal with this?

Comment 10

[Evan Yan]

I think that is interpreted by layout module. From the view of dom, it has no difference from 

<table>
  <tr><td>hello</td><td>hello</td></tr>
</table>

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> I think that is interpreted by layout module. From the view of dom, it has no
> difference from 
> 
> <table>
>   <tr><td>hello</td><td>hello</td></tr>
> </table>
> 

How it's interpreted by layout module? I just think we may broke nsIAccessibleTable methods in this case.

Comment 12

[Evan Yan]

In domi, these two cases have the same dom tree. They should be the same thing to a11y. I also confirmed that we created the same accessible tree.

Anyway, I can add it into mochitest if you insist.

Comment 13

[alexander :surkov (:asurkov)]

No, I don't. If those nested td is processed on DOM level then it's fine.

Comment 14

[alexander :surkov (:asurkov)]

one more thing: how will we deal with this:

<table>
  <tr><td>
    <div style="display: table">
      <tr>
        <td>hee</td>
      </tr>
    </div>
  </td></tr>
</table>

Comment 15

[Evan Yan]

the dom tree is like

table
  tbody
    tr
      td
        div
    tr
      td
        text ("hee")

so we have accessible tree (on Solaris)

table
  table cell
     table  (div)
  table cell ("hee")

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 318360 [details] [diff] [review]
add mochitest

r=me then

Comment 17

[Evan Yan]

Comment on attachment 318360 [details] [diff] [review]
add mochitest

it fixes a regression, and with low risk.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 318360 [details] [diff] [review]
add mochitest

a1.9=beltzner

Comment 19

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <--  nsAccessibilityService.cpp
new revision: 1.280; previous revision: 1.279
done
Checking in accessible/tests/mochitest/test_nsIAccessibleTable_4.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessibleTable_4.html,v  <--  test_nsIAccessibleTable_4.html
new revision: 1.2; previous revision: 1.1
done