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

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Attachment: 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...

Comment 1

[alexander :surkov (:asurkov)]

What does <dead> means? And are you about children of table and table itself?

Comment 2

[:Gijs (he/him)]

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.

Comment 3

[alexander :surkov (:asurkov)]

DOMi shows tree accessible has one children (accessible for textnode 'blah ...'). What tree do you have?

Comment 4

[Eitan Isaacson [:eeejay]]

Supposedly "<dead>" means None.

It could just as well be an Accerciser bug. I would suggest listing the children in the python console.

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[:Gijs (he/him)]

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. :-)

Comment 7

[:Gijs (he/him)]

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. 

Comment 8

[alexander :surkov (:asurkov)]

Attachment: 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.

Comment 9

[Aaron Leventhal]

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.

Comment 10

[alexander :surkov (:asurkov)]

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.

Comment 11

[alexander :surkov (:asurkov)]

Gijs, can you test the patch?

Comment 12

[:Gijs (he/him)]

(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. :-)

Comment 13

[:Gijs (he/him)]

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".

Comment 14

[:Gijs (he/him)]

(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.

Comment 15

[:Gijs (he/him)]

Attachment: Testcase - a11y internals

Concretely, in this testcase, both alerts should display the same (testcase requires universalxpconnect because otherwise it can't poll the accessibility stuff)

Comment 16

[alexander :surkov (:asurkov)]

(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.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Created an attachment (id=288226) [details]
> Testcase - a11y internals

Gijs, does this test works with patch?

Comment 18

[:Gijs (he/him)]

(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 19

[:Gijs (he/him)]

Comment on attachment 288226 [details]
Testcase - a11y internals

OK, so after some explanations from surkov, I guess this test is bogus. Sorry!

Comment 20

[:Gijs (he/him)]

Attachment: 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?

Comment 21

[alexander :surkov (:asurkov)]

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

Comment 22

[Aaron Leventhal]

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. 

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch2

Gijs, can you try this one?

Comment 24

[alexander :surkov (:asurkov)]

(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?

Comment 25

[Marco Zehe (:MarcoZ)]

(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.

Comment 26

[alexander :surkov (:asurkov)]

Attachment: patch3

Gijs, can you try this one?

Comment 27

[:Gijs (he/him)]

(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! :-)

Comment 28

[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?

Comment 29

[alexander :surkov (:asurkov)]

(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.

Comment 30

[Evan Yan]

(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.

Comment 31

[alexander :surkov (:asurkov)]

(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.

Comment 32

[Evan Yan]

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     }

Comment 33

[alexander :surkov (:asurkov)]

(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?

Comment 34

[Evan Yan]

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 35

[Ginn Chen]

Comment on attachment 289790 [details] [diff] [review]
patch3

only the accessible/src/base/nsAccessibilityService.cpp part is necessary.

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 37

[alexander :surkov (:asurkov)]

checked in