Fix discrepancy between tr count and row count in nsHTMLTableAccessible::IsProbablyForLayout

RESOLVED FIXED in Firefox 13

Status

()

Core
Disability Access APIs
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access, testcase})

Trunk
mozilla13
x86
Windows XP
access, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox-esr10 wontfix)

Details

(Whiteboard: [sg:investigate][qa?], URL)

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
Created attachment 383887 [details]
Testcase.

This is spun off bug 498913. The root cause for bug 498913 is that on www.bicycling.com, in the very last table on that page, the link to their RSS feed is put outside the last TR element, right before the closing </table>. This causes a discrepancy between TR elements and the row count we're receiving from the table interface.
Jonas: is this discrepancy limited to the accessibility system, or is it coming from DOM-land? It sounds like the kind of thing that could cause exploitable bugs.
Group: core-security
Whiteboard: [sg:investigate]
The fact that a DOM can look like that is no surprised. You can nest anything inside anything using DOM methods like Node.insertBefore.

What is surprising to me is that the HTML parser outputs a DOM like that. I would have expected the <link> to be inserted before the table.

In any case there is at most surprising behavior in the HTML parser here. The DOM itself is a totally valid DOM which any of our code should deal fine with. Though apparently the accessibility code wasn't, which is a bug in the accessibility code.
For what it's worth, the HTML5 parser will behave as I would have expected. So not sure it's worth filing a bug against our current parser, given that it's on its way out.
Sounds good, how do we close this now?
Like so :)
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
(Assignee)

Comment 6

8 years ago
(In reply to comment #5)
> Like so :)

It's ok if there is no anything wrong with parser but I believe we still have a11y problem: we shouldn't rely on count of html:tr elements if it can to not correspond to row frames count.
I'm not really following which two counts you are talking about. However if you want to get a list of all the rows, you could use nsIDOMHTMLTableElement::GetRows. That will return an nsIDOMHTMLCollection containing just the <tr> elements. Iterating that collection will give you just <tr> elements, and the length will be the number of such <tr> elements.

Note that today it's quite possible to create a DOM such as

<table>
  <tbody>
    <tr>...</tr>
    <someRandomElement></someRandomElement>
    <tr>...</tr>
  </tbody>
</table>

If you use the rows collection, the <someRandomElement> will not show up, which I suspect is what you want.
Oh, but if you're walking frames, then yeah, you can not rely on the DOM to correspond to the actual number of rows. In my example above I believe that there will be an anonymous row-frame for the <someRandomElement>, whereas the DOM has no idea if that is displayed as a table-row or not (or indeed if it's displayed at all or not).

In fact, there can be table frames without any table elements at all, if the page uses display:table and friends.
(Assignee)

Comment 9

8 years ago
Reopen per comment #8: we shouldn't rely on DOM since we are frame oriented for HTML tables.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
What are the implications of comment 9 on fixing bug 498913?
(Assignee)

Comment 11

8 years ago
(In reply to comment #10)
> What are the implications of comment 9 on fixing bug 498913?

We need to get rid the logic from nsHTMLTableAccessible relying on html:tr elements because our accessible tree for HTML tables is frame oriented. It should be a right fix of the bug 498913.
(Assignee)

Updated

8 years ago
Blocks: 491681
Alexander are you taking this one?
(Assignee)

Comment 13

8 years ago
I'm not sure we need to fix this bug until we resolve bug 513664. Because display style based tables might be our of law.
(Reporter)

Comment 14

8 years ago
This, or rather the original bug 498913 is also reproducible with Firefox 3.5.5 and NVDA 2009.1 by:

1. visiting http://gettopup.com/
2. Under the "Some Examples" heading, choose the sixth example "Google".
3. Pressing ENTER with the virtual cursor on that link will crash Firefox like this:
http://crash-stats.mozilla.com/report/index/7397227f-ce1a-41b6-a317-e49732091126

This is the exact problem.

Since we won't be porting outr table-related work into the 1.9.1 branch, can we fix this problem on the 1.9.1 branch either like we did for bug 498913 or by reworking this portion of the code so it counts row accessibles instead of row tags?
I think we should take this patch everywhere but trunk:
https://bug498913.bugzilla.mozilla.org/attachment.cgi?id=383743

I think crashing is worse than have slightly incorrect accessible table info. We should probably get active on bug 513664.
Oops that did land on 1.9.2 as trunk. So yes I agree we should land on 1.9.1
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> Oops that did land on 1.9.2 as trunk. So yes I agree we should land on 1.9.1

Agree. It appears the fix from the bug 513664 is all we want now. We should take this patch everywhere we need it. The long term solution should be found in bug 513664 as I mentioned in comment #13. Ah, and yes: let's land the patch linked from comment 15 :)
Keywords: testcase
(Assignee)

Comment 18

5 years ago
Created attachment 597716 [details] [diff] [review]
patch

just traverse rows by accessible tree, it should be consistent to frame tree. Also this approach prevents to checking colors between rows of nested tables. I don't think this feature was intentional and it doesn't seem to be practice.
Assignee: nobody → surkov.alexander
Status: REOPENED → ASSIGNED
Attachment #597716 - Flags: review?(marco.zehe)
(Reporter)

Comment 19

5 years ago
Comment on attachment 597716 [details] [diff] [review]
patch

>+  for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
>+    nsAccessible* child = GetChildAt(childIdx);

Are we not supposed to not use getChildAt any more for performance? Instead, iterating via getFirstChild/GetNextSibling etc?
(Assignee)

Comment 20

5 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #19)
> Comment on attachment 597716 [details] [diff] [review]
> patch
> 
> >+  for (PRUint32 childIdx = 0; childIdx < childCount; childIdx++) {
> >+    nsAccessible* child = GetChildAt(childIdx);
> 
> Are we not supposed to not use getChildAt any more for performance? Instead,
> iterating via getFirstChild/GetNextSibling etc?

this is accessible tree
(Reporter)

Comment 21

5 years ago
Comment on attachment 597716 [details] [diff] [review]
patch

r=me, then.
Attachment #597716 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 22

5 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1fb79b857a
https://hg.mozilla.org/mozilla-central/rev/6d1fb79b857a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
status-firefox13: --- → fixed
Group: core-security
status-firefox-esr10: --- → wontfix
Whiteboard: [sg:investigate] → [sg:investigate][qa+]

Comment 24

5 years ago
Tried to reproduce this issue on 
Mozilla/5.0 (Windows NT 5.1; rv:13.0a1) Gecko/20120212 Firefox/13.0a1 (20120212031149)
and verify it on
Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0 (20120528154913)

Apparently, www.bicycling.com now uses lists instead of tables (the page only contains one table and it doesn't have any elements between the tags for closing the tr and the table).

The steps from comment 14 don't make any of the build crash. 

The testcase attached to this bug looks the same on both builds too: the table is fully displayed, except for the link written between the </tr> and </table> tags.


Can anyone provide some guidelines about how to verify this fix? (perhaps how to verify tr count and row count from nsHTMLTableAccessible::IsProbablyForLayout?)
Whiteboard: [sg:investigate][qa+] → [sg:investigate][qa?]
You need to log in before you can comment on or make changes to this bug.