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

RESOLVED FIXED in mozilla10

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla10
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
(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.
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 2

6 years ago
Created attachment 563401 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #563401 - Flags: review?(bolterbugz)

Comment 3

6 years ago
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?
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 563403 [details] [diff] [review]
patch2

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
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/af057311517a
(Assignee)

Comment 14

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.