Closed Bug 430758 Opened 14 years ago Closed 14 years ago

GetAccessible() could be fooled by additional table ancestor

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: evan.yan, Assigned: evan.yan)

Details

(Keywords: access, regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #317662 - Flags: review?(surkov.alexander)
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?
Marco, it's inside the while loop

1503       while ((tableContent = tableContent->GetParent()) != nsnull) {
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?
We introduced that in bug 404881 before what we used "table" tag name
Btw, would be nice to put into our mochitests the testcase from bug 409009
I don't see special reason to check tableCellFrame though we need to check how we should deal with nested table cells.
Attached patch add mochitestSplinter Review
Attachment #317662 - Attachment is obsolete: true
Attachment #318360 - Flags: review?(surkov.alexander)
Attachment #317662 - Flags: review?(surkov.alexander)
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?
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>
(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.
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.
No, I don't. If those nested td is processed on DOM level then it's fine.
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>
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 on attachment 318360 [details] [diff] [review]
add mochitest

r=me then
Attachment #318360 - Flags: review?(surkov.alexander) → review+
Comment on attachment 318360 [details] [diff] [review]
add mochitest

it fixes a regression, and with low risk.
Attachment #318360 - Flags: approval1.9?
Comment on attachment 318360 [details] [diff] [review]
add mochitest

a1.9=beltzner
Attachment #318360 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.