Last Comment Bug 690222 - data table elements used to determine layout-guess attribute shouldn't be picked from nested tables
: data table elements used to determine layout-guess attribute shouldn't be pic...
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: tablea11y
  Show dependency treegraph
 
Reported: 2011-09-28 20:37 PDT by alexander :surkov
Modified: 2011-10-01 04:43 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.32 KB, patch)
2011-09-29 07:15 PDT, alexander :surkov
no flags Details | Diff | Review
patch2 (14.35 KB, patch)
2011-09-29 07:35 PDT, alexander :surkov
dbolter: review+
Details | Diff | Review

Description alexander :surkov 2011-09-28 20:37:13 PDT
example:

<table>
<tr><td><table summary="hello"><tr><td></td></tr></table></td></tr>
</table>

here's nested table is data table because of @summary attribute presence but outer table should be layout table while currently it's not.
Comment 1 alexander :surkov 2011-09-28 22:11:42 PDT
(In reply to alexander surkov from comment #0)
> example:
> 
> <table>
> <tr><td><table summary="hello"><tr><td></td></tr></table></td></tr>
> </table>
> 

@summary example is not correct, but if you'd put caption into nested table then it's good demonstration of the problem.

In order to fix this bug we need to fix nsHTMLTableAccessible::HasDescendant to work against accessible tree rather than DOM tree. Actually I think HasDescendant should be removed at all and be replaced on inline loop.
Comment 2 alexander :surkov 2011-09-29 07:15:52 PDT
Created attachment 563401 [details] [diff] [review]
patch
Comment 3 Marco Zehe (:MarcoZ) 2011-09-29 07:28:13 PDT
Comment on attachment 563401 [details] [diff] [review]
patch

Nice! So the outer tables are still layout tables while the inner ones inside the table21.x ones are to be considered data tables, right? If so, do we want to test the inner tables for being data tables, too?
Comment 4 alexander :surkov 2011-09-29 07:34:12 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Comment on attachment 563401 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Nice! So the outer tables are still layout tables while the inner ones
> inside the table21.x ones are to be considered data tables, right? 

yes.

> If so, do
> we want to test the inner tables for being data tables, too?

we don't have a logic paths where we check outer table, so I wouldn't care to add them. If this logic is introduced eventually then that will be a right time to test it.
Comment 5 alexander :surkov 2011-09-29 07:35:18 PDT
Created attachment 563403 [details] [diff] [review]
patch2

fix PRBool usage while I'm here
Comment 6 David Bolter [:davidb] 2011-09-29 07:40:06 PDT
Comment on attachment 563403 [details] [diff] [review]
patch2

>+    if (childElm->Tag() == nsGkAtoms::tbody) {
>+      PRUint32 rowCnt = childElm->GetChildCount();
>+      for (PRUint32 rowIdx = 0; rowIdx < rowCnt; rowIdx++) {
>+        nsIContent* rowElm = childElm->GetChildAt(rowIdx);
>+        if (rowElm->IsHTML() && rowElm->Tag() == nsGkAtoms::tr) {
>+          PRUint32 cellCnt = rowElm->GetChildCount();
>+          for (PRUint32 cellIdx = 0; cellIdx < cellCnt; cellIdx++) {
>+            nsIContent* cellElm = rowElm->GetChildAt(cellIdx);
>+            if (cellElm->IsHTML() && cellElm->Tag() == nsGkAtoms::th) {
>+              RETURN_LAYOUT_ANSWER(false,
>+                                   "Has th -- legitimate table structures");
>+            }
>+          }

Do we really need to check each cell or just the first one?

D
Comment 7 alexander :surkov 2011-09-29 07:43:45 PDT
(In reply to David Bolter [:davidb] from comment #6)

> Do we really need to check each cell or just the first one?

That's HTML, spec doesn't deny to mix td and th afaik.
Comment 8 David Bolter [:davidb] 2011-09-29 08:16:09 PDT
Comment on attachment 563403 [details] [diff] [review]
patch2

r=me.

Aside: in general I prefer pushing decisions like this out to the AT, but at the same time I don't think AT should deal with ISimpleDOM so if this helps them stop using it, fine with me.
Comment 9 David Bolter [:davidb] 2011-09-29 08:17:42 PDT
It might be worth getting someone from content to take a quick look. If HasDescendant was very performant I'd be more leery of this patch.
Comment 10 alexander :surkov 2011-09-29 09:17:47 PDT
(In reply to David Bolter [:davidb] from comment #9)
> It might be worth getting someone from content to take a quick look. If
> HasDescendant was very performant I'd be more leery of this patch.

I don't think GetElementsByTagName is so smart that it's faster then plain children iteration. CC'ing Olli just in case.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-29 09:37:56 PDT
Please don't use GetChildAt but GetFirstChild/GetNextSibling if just possible.
Comment 12 alexander :surkov 2011-09-29 09:40:23 PDT
(In reply to Olli Pettay [:smaug] from comment #11)
> Please don't use GetChildAt but GetFirstChild/GetNextSibling if just
> possible.

ok, GetChildAt results in children array creation, right? I'll fix that.
Comment 13 alexander :surkov 2011-09-30 18:12:13 PDT
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/af057311517a
Comment 14 alexander :surkov 2011-09-30 21:02:23 PDT
(In reply to alexander surkov from comment #13)
> inbound land
> http://hg.mozilla.org/integration/mozilla-inbound/rev/af057311517a

forgot to address Olli comment, follow up:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24fc6ed19796

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