Closed Bug 690222 Opened 13 years ago Closed 13 years ago

data table elements used to determine layout-guess attribute shouldn't be picked from nested tables

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
(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.
Summary: data table elements or DOM attributes 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 picked from nested tables
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #563401 - Flags: review?(bolterbugz)
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?
(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.
Attached patch patch2Splinter Review
fix PRBool usage while I'm here
Attachment #563401 - Attachment is obsolete: true
Attachment #563401 - Flags: review?(bolterbugz)
Attachment #563403 - Flags: review?(bolterbugz)
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
(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 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.
Attachment #563403 - Flags: review?(bolterbugz) → review+
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.
(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.
Please don't use GetChildAt but GetFirstChild/GetNextSibling if just possible.
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/af057311517a
https://hg.mozilla.org/mozilla-central/rev/24fc6ed19796
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: