455886 - AccessibleNameFromSubtree(): don't recurse into subtrees for roles that don't use name from subtree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Some roles don't allow the name to be aggregated from descendants. 
This is marked up for ARIA roles in:
http://mxr.mozilla.org/seamonkey/source/accessible/src/base/nsARIAMap.cpp
eNameOkFromChildren -> can aggregate name from descendants
eNameLabelOrTitle -> cannot

The idea is to not add up all the text for every list item in a list, or every cell in a grid, etc. That makes no sense for a speakable name. Right now we only follow that rule at the top of a subtree, but we need to follow it for every node.

Also, we need to follow that rule for native markup with the same roles. IOW, "listbox" in ARIA, but "select" in HTML.

I've attempted to write down what I think the ideal name algorithm is here:
http://developer.mozilla.org/en/ARIA_User_Agent_Implementors_Guide#Name_Computation

Comment 1

[Aaron Leventhal]

If we make it more general (not just avoiding ARIA composite widgets, but avoiding walking into any composite widget), then we also fix bug 455482.

Comment 2

[Aaron Leventhal]

This is blocking work on the new improved structure for ARIA tree views. The example should work once this is fixed. (I have an action to let James Craig and PFWG know when this is ready in a nightly build).

Comment 3

[alexander :surkov (:asurkov)]

So should we exclude the logic of eNameOkFromChildren/eNameLabelOrTitle from ARIA structures and create new structure based on roles?

Comment 4

[Aaron Leventhal]

Alex, you can create a new structure based on the role. It should basically be a list of "composite" roles where they are expected to often have many descendants. It should match what the ARIA role table says now for eNameOkFromChildren/eNameLabelOrTitle. If you do that then obviously it means you can remove that field from the ARIA role table.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

[Aaron Leventhal]

Hi Alex, this is impressive. I haven't finished reviewing nsNameUtils.cpp yet. I'm glad you're moving things into a separate nsNameUtils library. It will be easier to update in the long term.

Some questions:
1. ERecursiveNameRule -- there seem to be too many enum values here. I will have to look, but I'm surprised we have so many different cases. Something seems a bit wrong. Can it be simplified? It could use some deeper comments because several of them sound the same. Especially the rarely used ones -- and I don't mind if there are examples right there in the comments, e.g. // At the meoment this is only used for treeitem, which is special because blah
This is confusing and I want to make sure there is no doubt that we need all the complexity and what it's for
2. We need a way to compare the accessible names we compute in Firefox nightly builds now and the ones with this patch, to make sure there are no changes except where we intend.
3. Have you seen this?
http://developer.mozilla.org/en/ARIA_User_Agent_Implementors_Guide#Name_Computation
I'm not sure if it's 100% correct, but I need it to be. We had some other bugs somewhere like, aria-labelledby should override the name from subtree, not add to it. Anyway, please look and let me know where you see problems and what follow up bugs we might need (what's the difference between that and what we do now).
4. Please add an assertion to make sure that the number of roles and the number of name rule entries is the same. It can be like this:
http://mxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsAccessibleWrap.cpp#1290
Also, please add a comment to nsIAccessibleRole that if new roles are added they should also be added to the name rule map in nsNameUtils.cpp

Comment 7

[Aaron Leventhal]

On
nsNameUtils::GetNameFromSubtree(nsIAccessible *aAccessible, nsAString& aName)

The name "GetNameFromSubtree" makes me think it won't append, but it does. Should that be renamed or should it do aName.Truncate() ?

Comment 8

[Aaron Leventhal]

Actually, are we sure that we need a different GetNameFromSubtree() from AppendFlatStringFromSubtree()? 

What's the main reason for having 2 different methods?

Comment 9

[Aaron Leventhal]

Gee, looking at just nsNameUtils.h confuses me. I'm not sure why there are so many different methods and cases.

What I have in my mind, at least after writing the Name_Computation algorithm in the implementor's guide, is more streamlined.

Comment 10

[Aaron Leventhal]

Eitan, when will your tool be ready to just be able to compare the accessible trees generated by 2 different versions of Firefox? We need to be able to see what accessible name changes occur from this patch.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> On
> nsNameUtils::GetNameFromSubtree(nsIAccessible *aAccessible, nsAString& aName)
> 
> The name "GetNameFromSubtree" makes me think it won't append, but it does.
> Should that be renamed or should it do aName.Truncate() ?

Yes, I think it should truncate the name because it is used only if we didn't get the name somehow else.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Actually, are we sure that we need a different GetNameFromSubtree() from
> AppendFlatStringFromSubtree()? 
> 
> What's the main reason for having 2 different methods?

Theoretically we can but it will change the current behaviour. For example, now if aria-labelledby is used then we will compute the name from subtree in any way, it doesn't depend on role. I just tried to preserve all our mochitests (I just changed them one time only).

I'm not sure possibly it should be following up bug because it may be not safe enough. Though it will lead to some changes of proposed code so that we don't need to distinguish nameRule and recurseNameRule constants.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Gee, looking at just nsNameUtils.h confuses me. I'm not sure why there are so
> many different methods and cases.
>

So:

eRecursiveNameRuleFromName - it's for controls like buttons
eRecursiveNameRuleFromNameAndValue - it's for textboxes

We could combine them but I'm not sure we won't get a case when we don't want a value from accessible having a value
 
> What I have in my mind, at least after writing the Name_Computation algorithm
> in the implementor's guide, is more streamlined.

Actually, NameUtils contains methods to calculate name from subtree. IIRC your guide contains few sentences about that :)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Gee, looking at just nsNameUtils.h confuses me. I'm not sure why there are so
> many different methods and cases.

Sorry I forgot to list all constants:

eRecursiveNameRuleFromNameOrChildren

it's for the case
<label>hello: <button>btn</button></label>

we calculate the name for the button
1) get label and calculate name from subtree
2) get name for the button (empty) therefore get the name from button children.

This constant is not real case, it's rather a code trick I think.

eRecursiveNameRuleFromText

It's for text leafs and whitespaces. Also special constant

eRecursiveNameRuleFromChildren

It's for text containers like for html:p.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Gee, looking at just nsNameUtils.h confuses me. I'm not sure why there are so
> many different methods and cases.

about methods. I tried to keep methods small:

two of them is to navigate through accessible tree. It's good to navigate through accessible tree because we check accessible roles to understand how to compute the name

two of them is to navigate through DOM tree. It's for the case when we reference (for example labelled by) on the hidden node

and one to get the name from text leaf accessible or whitespace like html:br, it is used from accessible tree navigation methods and from DOM tree navigation methods.

and one is to add spaces to the name, it goes from nsAccessible unchanged

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> 2. We need a way to compare the accessible names we compute in Firefox nightly
> builds now and the ones with this patch, to make sure there are no changes
> except where we intend.

That would be great I think, so theoretically our mochitest should handle that. Though of course they don't provide 100% testing.

> 3. Have you seen this?
> http://developer.mozilla.org/en/ARIA_User_Agent_Implementors_Guide#Name_Computation
> I'm not sure if it's 100% correct, but I need it to be.

Yes, I tried to save the current approach as much as possible, tried to keep our mochitests and tried to follow this guide. Though since the patch is big and bring major code changes then I can make something wrong.

> We had some other bugs
> somewhere like, aria-labelledby should override the name from subtree, not add
> to it. Anyway, please look and let me know where you see problems and what
> follow up bugs we might need (what's the difference between that and what we do
> now).

When we get the fixes of name calculation from subtree then I think we should try to move  most of name calculation logic into nsNameUtils.

> 4. Please add an assertion to make sure that the number of roles and the number
> of name rule entries is the same. It can be like this:
> http://mxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsAccessibleWrap.cpp#1290
> Also, please add a comment to nsIAccessibleRole that if new roles are added
> they should also be added to the name rule map in nsNameUtils.cpp

Comment 18

[Aaron Leventhal]

(In reply to comment #13)
> (In reply to comment #9)
> > Gee, looking at just nsNameUtils.h confuses me. I'm not sure why there are so
> > many different methods and cases.
> >
> 
> So:
> 
> eRecursiveNameRuleFromName - it's for controls like buttons
> eRecursiveNameRuleFromNameAndValue - it's for textboxes
> 
> We could combine them but I'm not sure we won't get a case when we don't want a
> value from accessible having a value
Let's do that in a separate bug. If there is a value, use it. I just looked at the ::GetValue() impl's we have. We need to change them so that URL's are only exposed for the value in MSAA. So <a> and <area> need to have their ::GetValue() impls moved to msaa wrap classes. The value should be reserved for the user-facing value entered for the control.

> > What I have in my mind, at least after writing the Name_Computation algorithm
> > in the implementor's guide, is more streamlined.
> 
> Actually, NameUtils contains methods to calculate name from subtree. IIRC your
> guide contains few sentences about that :)
Actually it is supposed to say everything it needs to about that. Look at 2(g). At that point it is computing name from subtree, which says "recursively implement this name computation for each child, starting with step 1". Since it is recursive and appends the text for each child, that is the name from the subtree.

Comment 19

[Aaron Leventhal]

I would like to be able to get rid of all these constants:
 eRecursiveNameRule*
and just have a boolean on whether name from subtree is ok.

That would match docs Name_Computation.
Basically we would work on precedence for each node:
First try (a). If no still no name text for that node try (b), etc.
Finally, append to current name results, with spaces if necessary.

Eventually, I want Name_Computation docs and our code to match exactly. Both will probably have to change. And I want to use Eitan's test tool to make sure it's exactly right.

Comment 20

[Eitan Isaacson [:eeejay]]

I am working on a visual diff tool now. I hope to have something working by the beginning of next week.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #19)
> I would like to be able to get rid of all these constants:
>  eRecursiveNameRule*
> and just have a boolean on whether name from subtree is ok.

Ok, eRecursiveNameRule makes the coding simpler, at least these eRecursiveNameRuleFromText and eRecursiveNameRuleFromNameOrChildren.

eRecursiveNameRuleFromText
We can get rid it if we will try to get the text if the accessible or node hasn't children.

eRecursiveNameRuleFromNameOrChildren
This one is for
<label>hello<button>button</button>hello</hello>

I can't see how to get the name from button children if we have two constants: try to get the accessible name or go inside children. Though probably if we will follow your algorithm then all will be ok.

I'm not sure how we could get rid another constants because these constants bring flexibility. And if we will remove them then I think we could get easy regressions so that we won't be able to fix them without reintroducing eRecursiveNameRule constants. For sure, I understand your point to have clean algorithm, but truly I'm bit afraid to do this. I'm afraid because I can't keep in my mind all possible cases of name calculation. So I introduced these constants to have a lot of flexibility and make the code much closer to the current version. Here we do major step: we remove all specific cases for elememnts and start to use information based on roles. I'm not sure I would love to do another major steps.

Comment 22

[alexander :surkov (:asurkov)]

Aaron, we could get rid eRecursiveNameRule constants one by one in following up bugs. I tend to agree clear algorithm should be able to work without them. But I would like to consider every example separately rather than all of them together. I'm afraid we could find a case when this algorithm doesn't fit to some examples. As well it will be easy to track regressions and fix them.

Comment 23

[Aaron Leventhal]

I'm slightly afraid of reviewing this patch, because it's so big :P 
But that's not really your fault.

Also, I wonder when we would actually get to doing the follow up patches.

Comment 24

[Aaron Leventhal]

Basically, we need Eitan's tool and then we can be sure to do things right.

Comment 25

[Aaron Leventhal]

Eitan, we don't need a visual diff yet -- for we just need to know if there were any difference in accessible names or descriptions.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> I'm slightly afraid of reviewing this patch, because it's so big :P 
> But that's not really your fault.
> 
> Also, I wonder when we would actually get to doing the follow up patches.

This depends on priority we will set for this. You know me I like to clean the code, so I just need green light :)

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Eitan, we don't need a visual diff yet -- for we just need to know if there
> were any difference in accessible names or descriptions.

Aaron, I wonder what pages we should test?

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch2

up to dated to trunk

Comment 29

[Marco Zehe (:MarcoZ)]

Comment on attachment 342234 [details] [diff] [review]
patch2

1. Please also adjust the names collected in test_nsIAccessible_selects.html, these tests fail with this patch applied since the logic was fixed.

2. In Options/Privacy, the checkbox for "Delete history after 90 days" is broken. Its accessible name only contains "90 days", leaving out the original label for the checkbox itself. Same on the following textbox. The code is here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/privacy.xul#108
Please fix this occurrence and add a test based on this block to test_nsIAccessible_name.xul.

Comment 30

[alexander :surkov (:asurkov)]

That's very interesting case, so the name for 

<checkbox id="checkbox" label="checkbox-label" aria-labelledby="checkbox">

should be "checkbox-label". Do I understand correct?

Comment 31

[Marco Zehe (:MarcoZ)]

Yes. And if there are additional items, they should be appended, like the value of the textbox plus the following label, as in the example pointed to above.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> Yes. And if there are additional items, they should be appended, like the value
> of the textbox plus the following label, as in the example pointed to above.

Great, now this trick case is implemented by some bogus code (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#1635), bogus because it duplicates the code from ::GetXULName(). Here I tried to avoid this. This means when I get ID from aria-labelledby then I should get the accessible name for ID. That's recursion. If we really need this and if it's really correct then I should rethink the proposed patch.

Comment 33

[alexander :surkov (:asurkov)]

Ok, since aria-labelledby is the only one way that allows recursion and I really want to use ::GetName instead of current duplicating code of ::GetName() (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsAccessible.cpp#1635), then I think I should use ::GetName without ARIA stuffs if recursion is occurred. Therefore it makes sense to fix bug 453591 to separate ARIA stuffs from content stuffs.

Comment 34

[Aaron Leventhal]

> Therefore it makes sense to fix bug 453591 to separate ARIA stuffs
> from content stuffs.
That one is fixed now.

Can we get an updated patch for this?

Comment 35

[alexander :surkov (:asurkov)]

I think I need to finish the code clean up related with name calculation (outside of nsAccessible, in derivative classes). Therefore I need to clarify ARIA implementors guide. There are unanswered questions there.

Comment 36

[Aaron Leventhal]

Okay, I have updated https://developer.mozilla.org/en/ARIA_User_Agent_Implementors_Guide#Name_Computation
I believe it is satisfactory now.

Comment 37

[Aaron Leventhal]

Comment on attachment 342234 [details] [diff] [review]
patch2

Awaiting new patch.

Comment 38

[James Craig]

I updated some ARIA Tree code examples to use the new nameFrom calculation.

http://cookiecrook.com/test/aria/tree/ariatree.html
http://cookiecrook.com/test/aria/tree/ariatree2.html

Comment 39

[David Bolter [:davidb] (NeedInfo me for attention)]

Note that the user agent aria implementation guide is in w3c land now. Please see: http://www.w3.org/WAI/PF/aria-implementation/#Text_Equivalent_Computation

What is the status of this bug?

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)

> What is the status of this bug?

Several days ago I worked on this bug again but I didn't find nice solution to support recursive aria-labelledby, so I decided to get back to this bug a bit later so that I will be able to look at the problem from new angle.

Comment 41

[Aaron Leventhal]

PFWG asks about this nearly every week. They need an implementation of this for Last Call.

What about the recursion is difficult? Is there something that we could simplify in the name algorithm that would not affect authoring much, but make things easier to implement?

Comment 42

[alexander :surkov (:asurkov)]

Iirc ARIA computation guide doesn't address recursions. We need to support the following example:

<checkbox id="checkbox" aria-labelledby="checkbox" label="hello"/>
should have "hello" accessible name

as well more complex examples with aria-labelledby and html:label@for (xul:label@control) usage.

Ok since it's urgent bug then I'll get back.

Comment 43

[Aaron Leventhal]

I believe the guide actually addresses this, by saying that aria-labelledby is not recursive. I would include computed labelledby relations in this, which are created via <label for> markup. The idea is to remove the need to worry about infinite loops.

One caveate: if you are actually computing a description via aria-describedby you can use labelledby relations on nodes you walk into. Just don't do not use labelledby more than 1 level deep.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> I believe the guide actually addresses this, by saying that aria-labelledby is
> not recursive. I would include computed labelledby relations in this, which are
> created via <label for> markup. The idea is to remove the need to worry about
> infinite loops.
> 
> One caveate: if you are actually computing a description via aria-describedby
> you can use labelledby relations on nodes you walk into. Just don't do not use
> labelledby more than 1 level deep.

You're right. I meant to say "deny recursive aria-labelledby". But in the meantime we support recursive aria-labelledby and it's used in firefox UI at least (see comment #29). So it would be regression to follow ARIA implementation guide.

Comment 45

[Aaron Leventhal]

I'm confused why the new algorithm would break that exmple.

If we get the name for the checkbox, it should follow labelledby. The aria-labelledby says it is labelled by:
1. Itself (avoid recursion and use contents of label attribute)
2. The textbox (avoid labelledby and use the value)
3. The label that comes after with id=rememberberAfter (use the child contents)

132         <checkbox id="rememberHistoryDays"
133                   label="&rememberDaysBefore.label;"
134                   accesskey="&rememberDaysBefore.accesskey;"
135                   oncommand="gPrivacyPane.onchangeHistoryDaysCheck();"
136                   aria-labelledby="rememberHistoryDays historyDays rememberAfter"/>
137         <textbox id="historyDays" type="number" size="3"
138                  aria-labelledby="rememberHistoryDays historyDays rememberAfter"
139                  onkeyup="gPrivacyPane.onkeyupHistoryDaysText();"
140                  preference="browser.history_expire_days_min"/>
141         <label id="rememberAfter"> &rememberDaysAfter2.label;</label>

So, I don't see why there woudl be a regression.

Comment 46

[alexander :surkov (:asurkov)]

What is "current user-entered value", how can I know if nsIAccessible::value is user-entered? Is "user-entered" or "user-typeable" assumed here ?

For example:

<img href="moz.moz"/> - accessible value "moz.moz"
<input value="default value"/> - accessible value is "default value"
<input/> + input.value == "hello" (user typed "hello") - accessible value is "hello"

Could you say what is user-entered and what is not user-entered in these examples?

Comment 47

[Aaron Leventhal]

"User-entered value" was intended to mean the same thing as nsIAcc::value.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> "User-entered value" was intended to mean the same thing as nsIAcc::value.

So, example

<a href="moz.moz"><img src="hello"/></a>

name for html:a should be "hello moz.moz", because html:a allows name from subtree, img has name "hello" and has a value "moz.moz" as linkable accessible. That's right?

Comment 49

[Aaron Leventhal]

That's a case where nsIAccessible::value should not implement those. It's a hack that it does, in order to support the old hack in MSAA that accValue can hold a URL. We should only implement this value hack in src/msaa/ns*Wrap classes.

Comment 50

[alexander :surkov (:asurkov)]

Attachment: wip

1) eFromValue constants are temporary to make correct names for linkable accessibles.
2) test_name_markup fails due to wrong mutation events

Comment 51

[alexander :surkov (:asurkov)]

Aaron, for the case

<span id="label"><a href="moz.moz" aria-labelledby="label">link</a></span>

name for span will include value of html:a ("moz.moz"), is it what we expect?

Comment 52

[alexander :surkov (:asurkov)]

(In reply to comment #49)
> That's a case where nsIAccessible::value should not implement those. It's a
> hack that it does, in order to support the old hack in MSAA that accValue can
> hold a URL. We should only implement this value hack in src/msaa/ns*Wrap
> classes.

Aaron, should we turn all nsLinkableAccessible methods into corresponding MSAA methods implementing in MSAA wrap classes (value, actions, states, taking focus)?

Comment 53

[Marco Zehe (:MarcoZ)]

(In reply to comment #51)
> Aaron, for the case
> 
> <span id="label"><a href="moz.moz" aria-labelledby="label">link</a></span>
> 
> name for span will include value of html:a ("moz.moz"), is it what we expect?

Definitely not! I'm speaking as a user here, and I would not want the link's value in there. Perhaps we sould specify which accessible's values we use exactly. The ones that spring to mind and make sense are:
- textbox
- Combobox (selected option) and derivatives.

Looking at other examples, i don't currently see where else we might find the value of use. Or am I overlooking something?

Comment 54

[Aaron Leventhal]

Marco, a slider, a progressmeter or a single select list box.

Anything where the use of nsIAcc::value was legitimate. In the case of using it for URLs/HREFs, that's an MSAA hack and should be moved to the msaa wrap classes, in order to not affect the name algorithm.

Comment 55

[Aaron Leventhal]

(In reply to comment #52)
> Aaron, should we turn all nsLinkableAccessible methods into corresponding MSAA
> methods implementing in MSAA wrap classes (value, actions, states, taking
> focus)?

So far I'm only talking about adding an nsLinkableAccessibleWrap for value. I don't know that we want to change the others. We should check with the Orca folks to see if they only need those behaviors/properties on the actual link objects, and if so, yes -- then it sounds like we could move the entire class.

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> (In reply to comment #52)
> > Aaron, should we turn all nsLinkableAccessible methods into corresponding MSAA
> > methods implementing in MSAA wrap classes (value, actions, states, taking
> > focus)?
> 
> So far I'm only talking about adding an nsLinkableAccessibleWrap for value. I
> don't know that we want to change the others. We should check with the Orca
> folks to see if they only need those behaviors/properties on the actual link
> objects, and if so, yes -- then it sounds like we could move the entire class.

As well we should move then nsHTMLLinkAccessible::GetValue to wrap class because it is inherited from nsHyperTextAccessibleWrap.

Btw, can you or Marco clear this question with Orca's team?

Comment 57

[alexander :surkov (:asurkov)]

Another example I want to get feedback.

<label id="combo2">Search in:<select name="search">
    <option>Newsticker</option>
    <option>Entire site</option>
  </select></label>

1) name for label is "Search in: Newsticker" because we get name from subtree, 1) from text node 2) from html:select name which is empty 3) from html:select value

2) name for select is "Search in:" because get name from label but skip value because of item f) of ARIA name computation guide

3) name for list that is child of html:select is "Search in: Newsticker" becuase item f) doesn't work here (for reference "do not append this text if it will be the first or last part of a text equivalent calculation for the same node that it came from").

Does it sounds correct?

Comment 58

[alexander :surkov (:asurkov)]

Another examples

<button>
  <ul>
     <li>item1</li>
     <li>item2</li>
  </ul>
</button>

name for button is "item1 item2"

however

<label>
  <select size=3">
    <option>item1</option>
    <option>item2</option>
</label>

name for label is also "item1 item2"

because select@size="3" and ul have both role ROLE_LIST

Comment 59

[Aaron Leventhal]

Regarding #57, that sounds correct.

Regarding #58, we have a separate ROLE_LIST vs. ROLE_LISTBOX. We should treat document structure (LIST, PARAGRAPH, etc.) differently from widgets (LISTBOX). In the case of document structure we shouldn't calculate the name from a subtree, but if an ancestor is doing that, we shouldn't stop the recursion. So for a doc structure, we need something like AllowNameFromSubtreeInAncestor to allow recursion to pass through. But, don't compute the name for the actual ROLE_LIST or ROLE_PARAGRAPH to be the subtree.

Comment 60

[alexander :surkov (:asurkov)]

(In reply to comment #59)
> Regarding #57, that sounds correct.
> 
> Regarding #58, we have a separate ROLE_LIST vs. ROLE_LISTBOX. We should treat
> document structure (LIST, PARAGRAPH, etc.) differently from widgets (LISTBOX).

So we have the bug then since we expose select@size as ROLE_LIST.

> In the case of document structure we shouldn't calculate the name from a
> subtree, but if an ancestor is doing that, we shouldn't stop the recursion. So
> for a doc structure, we need something like AllowNameFromSubtreeInAncestor to
> allow recursion to pass through. But, don't compute the name for the actual
> ROLE_LIST or ROLE_PARAGRAPH to be the subtree.

Aaron, could you provide an example for each statement?

Comment 61

[Aaron Leventhal]

> So we have the bug then since we expose select@size as ROLE_LIST.
Right, that's a bug.

> > In the case of document structure we shouldn't calculate the name from a
> > subtree, but if an ancestor is doing that, we shouldn't stop the recursion. So
> > for a doc structure, we need something like AllowNameFromSubtreeInAncestor to
> > allow recursion to pass through. But, don't compute the name for the actual
> > ROLE_LIST or ROLE_PARAGRAPH to be the subtree.
> 
> Aaron, could you provide an example for each statement?

<ul> <-- no name for itself, but can contribute subtree to name of an ancestor
  <li>Blah</li>

...

<select size="5">    <-- No name from subtree, ever
  <option>Blah</option>

Comment 62

[alexander :surkov (:asurkov)]

(In reply to comment #61)
> > So we have the bug then since we expose select@size as ROLE_LIST.
> Right, that's a bug.

I filed bug 477606 for this.

Comment 63

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 64

[Marco Zehe (:MarcoZ)]

Comment on attachment 361758 [details] [diff] [review]
patch

A few typos I found on first look:

>+   * Iterates accessible children and caclulates text equivalent from each
>+   * child.

"calculates" misses an l here.

>+   * Concantenates stings and appends space between them.

"Concatenates" "strings".

>+   * The accessible the text equivalent is computed for in the end. It's used
>+   * for 1) avoid recursion (recursive>ARIA labels and descriptions, labels and
>+   * description from markup - all that can be recurive is denied once some of
>+   * these was triggered), 2) implement step f. of text equivalent computation
>+   * guide - getting text equivalent from accessible value.

Aside from "recursive", which is a typo, I believe this should be reworded. David, any good suggestions?

>+ * Run through accessible tree of the given inentifier so that we ensure
>+ * accessible tree is created.

"identifier".

Comment 65

[David Bolter [:davidb] (NeedInfo me for attention)]

This bug is a great example of how accessibility can be intricate. I'll need a bit of time for my review please :)

Marco, I'll offer a suggestion for wording when I have a clue.

Comment 66

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #52)

> As well we should move then nsHTMLLinkAccessible::GetValue to wrap class
> because it is inherited from nsHyperTextAccessibleWrap.
> 
> Btw, can you or Marco clear this question with Orca's team?

To follow up here, I checked with Will, Orca expects no value to be returned from a link.

Comment 67

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #64)
> (From update of attachment 361758 [details] [diff] [review])

> >+   * The accessible the text equivalent is computed for in the end. It's used
> >+   * for 1) avoid recursion (recursive>ARIA labels and descriptions, labels and
> >+   * description from markup - all that can be recurive is denied once some of
> >+   * these was triggered), 2) implement step f. of text equivalent computation
> >+   * guide - getting text equivalent from accessible value.
> 
> Aside from "recursive", which is a typo, I believe this should be reworded.
> David, any good suggestions?
> 

Maybe:

"The accessible for which we are computing a text equivalent. It is useful for bailing out during recursive text computation, or for special cases like step f. of the ARIA implementation guide."

OR

Surkov, what do you think about adding a param to the methods, and doing away with the global?

Comment 68

[David Bolter [:davidb] (NeedInfo me for attention)]

I've taken this patch on a few dates and we are starting to like each other. I sort of like the additional eFrom*'s which makes me very nervous because Aaron did not.

What is the status of our use of Speclenium? This seems like a great place to use it.

Also, maybe we can ask Will or Joanie to try a build of FF with this patch against the Orca regression suite?

Comment 69

[Marco Zehe (:MarcoZ)]

I plan on taking a build with this patch on a Speclenium ride tomorrow. Planned to do it today but was kept away by a burning tree which I had to help fix. :-) Running this against the Orca test suite would certainly be tremendously helpful!

Comment 70

[alexander :surkov (:asurkov)]

(In reply to comment #67)

> Surkov, what do you think about adding a param to the methods, and doing away
> with the global?

Truly I don't see a way. I think it must be static because it prevents recursive calls of GetNameFromSubtree.

Comment 71

[alexander :surkov (:asurkov)]

Try builds are available at https://build.mozilla.org/tryserver-builds/2009-02-15_23:[e-mail]-try-a381a016cb7/

Comment 72

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 73

[Marco Zehe (:MarcoZ)]

Comment on attachment 362551 [details] [diff] [review]
patch2

r=me for the test part. Question: Is this patch already supporting aria-labelledby pointing to visibility: hidden; or display: none; elements? If so, I'd want to see tests added for that as well.

Comment 74

[alexander :surkov (:asurkov)]

(In reply to comment #73)
> (From update of attachment 362551 [details] [diff] [review])
> r=me for the test part. Question: Is this patch already supporting
> aria-labelledby pointing to visibility: hidden; or display: none; elements? If
> so, I'd want to see tests added for that as well.

We have test for hidden labels (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_nsIAccessible_name.html?force=1#49). It's not enough?

Comment 75

[Marco Zehe (:MarcoZ)]

Sorry, was thinking about bug 462137, but that concerns the two types of visibility on descriptions, not names.

Comment 76

[alexander :surkov (:asurkov)]

(In reply to comment #75)
> Sorry, was thinking about bug 462137, but that concerns the two types of
> visibility on descriptions, not names.

It sounds (bug 462136 comment #2) that bug is about we don't create related accessibles if nodes are hidden. We don't have good description testing at all. I think we'll introduce them once we start description code reorg to follow ARIA text equiv computation guide.

Comment 77

[alexander :surkov (:asurkov)]

(In reply to comment #76)

> It sounds (bug 462136 comment #2)

should be bug 462137 comment #2

Comment 78

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 362551 [details] [diff] [review]
patch2

>+function waitForEvent(aEventType, aTarget, aFunc, aContext, aArg1, aArg2)
>+{
>+  var handler = {
>+    handleEvent: function handleEvent(aEvent) {
>+      if (!this.mTarget || this.mTarget == aEvent.DOMNode) {
>+        unregisterA11yEventListener(this.mEventType, this);
>+
>+        window.setTimeout(
>+          function (aFunc, aContext, aArg1, aArg2)
>+          {
>+            aFunc.call(aContext, aArg1, aArg2);
>+          },
>+          0,
>+          this.mFunc, this.mContext, this.mArg1, this.mArg2
>+        );
>+      }
>+    },
>+
>+    mEventType: aEventType,
>+    mFunc: aFunc,
>+    mContext: aContext,
>+    mArg1: aArg1,
>+    mArg2: aArg2,
>+    mTarget: aTarget

Why do you need these properties stored in the handler object? The closure created by the handleEvent function should protect your arguments so you could probably do away with the mFxxxx stuff right?

Comment 79

[alexander :surkov (:asurkov)]

(In reply to comment #78)

> Why do you need these properties stored in the handler object? The closure
> created by the handleEvent function should protect your arguments so you could
> probably do away with the mFxxxx stuff right?

Yes. But I can't get a habit to live with this. I prefer more explicit stuffs (kind of c++ style). But if you like then I can get rid them.

Comment 80

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 362551 [details] [diff] [review]
patch2

(In reply to comment #79)
> (In reply to comment #78)
> 
> > Why do you need these properties stored in the handler object? The closure
> > created by the handleEvent function should protect your arguments so you could
> > probably do away with the mFxxxx stuff right?
> 
> Yes. But I can't get a habit to live with this. I prefer more explicit stuffs
> (kind of c++ style). But if you like then I can get rid them.

Yeah it can be tricky going back and forth between C++ and JS for sure. My preference is to get rid of them though because it actually helps me remember they are different languages :)

r=me (either way)

Comment 81

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/c22d817948a7