GetAccessible() could be fooled by additional table ancestor

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

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

Tracking

({access, regression})

Trunk
x86
Linux
access, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.36 KB, patch
surkov
: review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 317662 [details] [diff] [review]
patch
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?
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 318360 [details] [diff] [review]
add mochitest
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?
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 12

11 years ago
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>
(Assignee)

Comment 15

11 years ago
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+
(Assignee)

Comment 17

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.