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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
14.35 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #563401 -
Flags: review?(bolterbugz)
Comment 3•13 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•13 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•13 years ago
|
||
fix PRBool usage while I'm here
Attachment #563401 -
Attachment is obsolete: true
Attachment #563401 -
Flags: review?(bolterbugz)
Attachment #563403 -
Flags: review?(bolterbugz)
Comment 6•13 years ago
|
||
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•13 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 8•13 years ago
|
||
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+
Comment 9•13 years ago
|
||
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•13 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.
Comment 11•13 years ago
|
||
Please don't use GetChildAt but GetFirstChild/GetNextSibling if just possible.
Assignee | ||
Comment 12•13 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•13 years ago
|
||
inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/af057311517a
Assignee | ||
Comment 14•13 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
Comment 15•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•