Closed Bug 400066 Opened 15 years ago Closed 14 years ago

Setting role on table leads to nodes being <dead> in accerciser

Categories

(Core :: Disability Access APIs, defect, P4)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

Attached file Testcase
Testcase is attached. Removing the "role='wairole:log'" attribute off the table fixes the issue. I'm not sure what else to add since I'm not sure what is going on behind the scenes, but I'm pretty sure this has severe impact on a11y...
Flags: blocking-firefox3?
Assignee: nobody → aaronleventhal
Blocks: fox3access
Component: Disability Access → Disability Access APIs
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
What does <dead> means? And are you about children of table and table itself?
I'm not entirely clear on what <dead> means, I believe it means that there is supposed to be a child accessible (according to the parent), and then there isn't? These are the children of the table. The table itself was fine. If in doubt, you should ask Eitan.
DOMi shows tree accessible has one children (accessible for textnode 'blah ...'). What tree do you have?
Supposedly "<dead>" means None.

It could just as well be an Accerciser bug. I would suggest listing the children in the python console.
(In reply to comment #3)
> DOMi shows tree accessible has one children (accessible for textnode 'blah
> ...'). What tree do you have?
> 

The same, I guess, but if you remove the role, it shows the table has a child (a table cell) which has a child (the cell text). (instead of the text being a direct child of the table node)

(In reply to comment #4)
> Supposedly "<dead>" means None.
> 
> It could just as well be an Accerciser bug. I would suggest listing the
> children in the python console.
> 

With the table node focused, acc.childCount reports 1, but acc.getChildAtIndex(0) gets you None back.

For the table without role=wairole:log, acc.getChildAtIndex(0) returns the table cell accessible.
Alexander: do you have any clue what might be causing this with this added info? If you have any more questions, please let me know. :-)
This blocks ChatZilla's accessibility support from being exported to AT tools, and will block a large number of other web apps from becoming accessible as well. Requesting blocking1.9. 
Flags: blocking1.9?
Assignee: aaronleventhal → surkov.alexander
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Attached patch patch (obsolete) — Splinter Review
Gijs, if I get the problem correctly then you should see an assertion "Fail to get AtkObj" in debug build and this patch should fix the problem.
Surkov, do you think this is the same cause as bug 397485? In that bug it's a MustPrune() parent text object, so the ATK children aren't created but show up as embedded object chars.
It sounds these are similar but different things. If bug 397485 is caused by MustPrune() but this one is accessibles aren't created for text nodes.
Gijs, can you test the patch?
(In reply to comment #11)
> Gijs, can you test the patch?
> 

Yeah. I'm using a VM here to run Linux, but if that doesn't work out I am coming back home tonight and can poke a real linux machine there :-).

I should add that I have never seen this assertion, but perhaps that's because I haven't run debug builds. I forget. I'll try the patch whatever the case. :-)
There is good news and bad news. The good news is I don't see dead nodes anymore - none at all. The bad news is I don't see any child nodes at all - the table now has 0 children in accerciser, and in DOMI it looks like it did before, as far as I can tell. So, if I understand the issue, we want it to get table cells just like the table without role="log".
(In reply to comment #8)
> Created an attachment (id=288102) [details]
> patch
> 
> Gijs, if I get the problem correctly then you should see an assertion "Fail to
> get AtkObj" in debug build and this patch should fix the problem.
> 


I have just checked at home, and I *was* using a debug build to look at this, and on this build (from october) I do not see any of the assertions you mentioned. So it doesn't look like this is the problem.
Attached file Testcase - a11y internals (obsolete) —
Concretely, in this testcase, both alerts should display the same (testcase requires universalxpconnect because otherwise it can't poll the accessibility stuff)
(In reply to comment #13)
So, if I understand the issue, we want it to get
> table cells just like the table without role="log".
> 

Why do we need accessible for table cells? if table cell is not inside table then we do not expose accessible for it. It looks correct.
(In reply to comment #15)
> Created an attachment (id=288226) [details]
> Testcase - a11y internals

Gijs, does this test works with patch?
(In reply to comment #16)
> (In reply to comment #13)
> So, if I understand the issue, we want it to get
> > table cells just like the table without role="log".
> > 
> 
> Why do we need accessible for table cells? if table cell is not inside table
> then we do not expose accessible for it. It looks correct.
> 

But there *is* a table cell:
<table role="wairole:log"><tr><td>Blah blah blah</td></tr></table>

How is that not a table cell?

(In reply to comment #17)
> (In reply to comment #15)
> > Created an attachment (id=288226) [details] [details]
> > Testcase - a11y internals
> 
> Gijs, does this test works with patch?

No. It's essentially the same as the previous test but without having to manually compare DOMI results.
Comment on attachment 288226 [details]
Testcase - a11y internals

OK, so after some explanations from surkov, I guess this test is bogus. Sorry!
Attachment #288226 - Attachment is obsolete: true
Attached file Testcase with link
So on surkov's request, this is a testcase with a link inside the table cell. After the patch, there is now a <dead> child for the link in accerciser. Not sure how to check that in DOMI, however.

Also, the patch makes all the text nodes just go away - the table does not have children at all in accerciser. Surkov told me this is by-design, but it does pose a problem for ChatZilla as far as I can tell. Aaron, can you comment on that?
question from irc to Aaron

<surkov> the problem is we don't take into account text accessible
<surkov> that are cutted off from the tree in atk
<surkov> should we cut off text accessibles (depending on platform) on gecko layer?
<surkov> so if we are on linux then we have not text accessibles, if we are on windows then we do
<surkov> now in gecko we always have text accessibles and then remove them before we expose them for AT
<surkov> here the problem starts
<surkov> for example, we report there are children but actually there aren't
<surkov> therefore we get a mesh with child indexes and so on
Do we just need to make sure we provide child indices for (In reply to comment #21)
> <surkov> the problem is we don't take into account text accessible
> <surkov> that are cutted off from the tree in atk
> <surkov> should we cut off text accessibles (depending on platform) on gecko
> layer?
The problem is: if we cut off text accessibles in nsIAccessible layer, then nsIAccessibleText interfaces won't work, because nsHyperTextAccessible needs those text children to calculate what text it has.
Also, we still need those child text accessibles on Windows, for JAWS and Window-Eyes. We only want to remove them in ATK.

> <surkov> so if we are on linux then we have not text accessibles, if we are on
> windows then we do
> <surkov> now in gecko we always have text accessibles and then remove them
> before we expose them for AT
> <surkov> here the problem starts
> <surkov> for example, we report there are children but actually there aren't
> <surkov> therefore we get a mesh with child indexes and so on
Please explain more what's happening in the code. 

Attached patch patch2 (obsolete) — Splinter Review
Gijs, can you try this one?
Attachment #288102 - Attachment is obsolete: true
(In reply to comment #22)

> Please explain more what's happening in the code. 
> 

Here there are two problems actually

1. Gecko tree and Atk tree is different things. So nsIAccessible::childCount and ::getChildAt works with gecko children (i.e. with text accessibles too), therefore they return incorrect result for ATK since text accessibles are cut off from the tree. For example:
gecko tree:
section
  text acc
  button
  text acc
atk tree
section
  button

nsIAccessible::childCount return 3 when in atk should be 1, so when atk request first children for example, it doesn't receive an accessible but it should get a button.

2. Since html:table has aria role then we do not expose accessibles for html:td and since html:table is not hyper text accessible then text inside html:td is not accessible for AT. What we can do here?
(In reply to comment #24)
> 2. Since html:table has aria role then we do not expose accessibles for html:td
> and since html:table is not hyper text accessible then text inside html:td is
> not accessible for AT. What we can do here?
 
I'm curious: Why do the HTML:td elements no longer get rendered as table cell objects if the role is "log"? From a usability standpoint, still exposing a log as a table would make perfect sense, since you usually have a structured format (timestamp: action/event). Using screen readers, navigating tables is often a very good way to navigate structural information. So I would be interested to know what the thinking is to not expose a table with a role of "log" as a table any more.
Attached patch patch3 (obsolete) — Splinter Review
Gijs, can you try this one?
Attachment #288672 - Attachment is obsolete: true
(In reply to comment #26)
> Created an attachment (id=289790) [details]
> patch3
> 
> Gijs, can you try this one?
> 

Cool, that seems to work for both testcases, getting table cells everywhere. Happiness! :-)
Attachment #289790 - Flags: review?(ginn.chen)
Attachment #289790 - Flags: review?(ginn.chen) → review?(Evan.Yan)
Comment on attachment 289790 [details] [diff] [review]
patch3

This makes getChildCountCB from O(1) to O(n). Can we have a better solution?
(In reply to comment #28)
> (From update of attachment 289790 [details] [diff] [review])
> This makes getChildCountCB from O(1) to O(n). Can we have a better solution?
>
 
Does O(n) beat us? Because good practice is to save child count into local variable when you're going to use it.
(In reply to comment #29)
> Does O(n) beat us? 
> 
I would say it's acceptable if we have to.
Could you give more explanation for the patch? The change you made to getChildCountCB looks like there is some accessible other than hyper text have both of text and non-text child.
(In reply to comment #30)
> (In reply to comment #29)
> > Does O(n) beat us? 
> > 
> I would say it's acceptable if we have to.
> Could you give more explanation for the patch?

We don't expose text accessibles (http://lxr.mozilla.org/mozilla/source/accessible/src/atk/nsAccessibleWrap.cpp#373), therefore all I do is enusre atk object is exists for the given gecko accessible.
I mean this situation should have been covered by 

879     if (hyperText) {
880         // If HyperText, then number of links matches number of children
881         hyperText->GetLinks(&count);
882     }
(In reply to comment #32)
> I mean this situation should have been covered by 
> 
> 879     if (hyperText) {
> 880         // If HyperText, then number of links matches number of children
> 881         hyperText->GetLinks(&count);
> 882     }
> 

Sorry, didn't catch you. Can you give more details?
If an accessible has both of text child and non-text child, it should be nsHyperTextAccessible, right?
We just use hyperText->GetLinks() to get its non-text child.

In your patch, you tried to filter text child for an accessible which is not hyper text. So I think either the accessible should implements hyper text, or there is some thing wrong with its children.
Comment on attachment 289790 [details] [diff] [review]
patch3

only the accessible/src/base/nsAccessibilityService.cpp part is necessary.
Attachment #289790 - Flags: review?(Evan.Yan) → review-
Attached patch patch4Splinter Review
Attachment #289790 - Attachment is obsolete: true
Attachment #292297 - Flags: review?(ginn.chen)
Attachment #292297 - Flags: review?(ginn.chen) → review+
Attachment #292297 - Flags: approval1.9?
Attachment #292297 - Flags: approval1.9? → approval1.9+
checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.