Last Comment Bug 499093 - Fix discrepancy between tr count and row count in nsHTMLTableAccessible::IsProbablyForLayout
: Fix discrepancy between tr count and row count in nsHTMLTableAccessible::IsPr...
Status: RESOLVED FIXED
[sg:investigate][qa?]
: access, testcase
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla13
Assigned To: alexander :surkov
:
Mentors:
http://www.bicycling.com
Depends on:
Blocks: tablea11y 498913
  Show dependency treegraph
 
Reported: 2009-06-18 01:34 PDT by Marco Zehe (:MarcoZ)
Modified: 2012-05-30 09:34 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
wontfix


Attachments
Testcase. (530 bytes, text/html)
2009-06-18 01:34 PDT, Marco Zehe (:MarcoZ)
no flags Details
patch (2.71 KB, patch)
2012-02-16 01:27 PST, alexander :surkov
mzehe: review+
Details | Diff | Review

Description Marco Zehe (:MarcoZ) 2009-06-18 01:34:34 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2009-06-19 10:15:41 PDT
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.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2009-06-19 13:35:13 PDT
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.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2009-06-19 13:36:05 PDT
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.
Comment 4 David Bolter [:davidb] 2009-06-19 13:47:12 PDT
Sounds good, how do we close this now?
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2009-06-19 14:21:06 PDT
Like so :)
Comment 6 alexander :surkov 2009-06-19 18:54:48 PDT
(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.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2009-06-19 19:37:42 PDT
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.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2009-06-19 19:42:58 PDT
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.
Comment 9 alexander :surkov 2009-06-21 18:12:59 PDT
Reopen per comment #8: we shouldn't rely on DOM since we are frame oriented for HTML tables.
Comment 10 Daniel Veditz [:dveditz] 2009-06-24 15:45:50 PDT
What are the implications of comment 9 on fixing bug 498913?
Comment 11 alexander :surkov 2009-06-24 17:37:51 PDT
(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.
Comment 12 David Bolter [:davidb] 2009-10-13 06:34:50 PDT
Alexander are you taking this one?
Comment 13 alexander :surkov 2009-10-16 00:21:00 PDT
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.
Comment 14 Marco Zehe (:MarcoZ) 2009-11-26 06:56:08 PST
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?
Comment 15 David Bolter [:davidb] 2009-11-26 11:15:19 PST
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.
Comment 16 David Bolter [:davidb] 2009-11-26 11:16:50 PST
Oops that did land on 1.9.2 as trunk. So yes I agree we should land on 1.9.1
Comment 17 alexander :surkov 2009-12-03 07:02:16 PST
(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 :)
Comment 18 alexander :surkov 2012-02-16 01:27:39 PST
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.
Comment 19 Marco Zehe (:MarcoZ) 2012-02-16 02:10:01 PST
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?
Comment 20 alexander :surkov 2012-02-16 02:15:51 PST
(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
Comment 21 Marco Zehe (:MarcoZ) 2012-02-16 02:45:53 PST
Comment on attachment 597716 [details] [diff] [review]
patch

r=me, then.
Comment 22 alexander :surkov 2012-02-16 23:19:38 PST
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1fb79b857a
Comment 23 Ed Morley [:emorley] 2012-02-17 18:04:19 PST
https://hg.mozilla.org/mozilla-central/rev/6d1fb79b857a
Comment 24 Ioana (away) 2012-05-30 09:34:27 PDT
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?)

Note You need to log in before you can comment on or make changes to this bug.