GetAccessible() could be fooled by additional table ancestor

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
10 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

10 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

10 years ago
Created attachment 317662 [details] [diff] [review]
patch
Attachment #317662 - Flags: review?(surkov.alexander)

Comment 2

10 years ago
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

10 years ago
Marco, it's inside the while loop

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

Comment 4

10 years ago
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

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

Comment 6

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

Comment 7

10 years ago
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

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

Comment 9

10 years ago
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

10 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>

Comment 11

10 years ago
(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

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

Comment 13

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

Comment 14

10 years ago
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

10 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 16

10 years ago
Comment on attachment 318360 [details] [diff] [review]
add mochitest

r=me then
Attachment #318360 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

10 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+

Comment 19

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