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

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: aaronlev, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {access})

unspecified
x86
Windows XP
access
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 ?
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
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.
Blocks: 455482
(Reporter)

Comment 2

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

Comment 3

11 years ago
So should we exclude the logic of eNameOkFromChildren/eNameLabelOrTitle from ARIA structures and create new structure based on roles?
(Reporter)

Comment 4

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

Updated

11 years ago
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
(Assignee)

Comment 5

11 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #340298 - Flags: review?(aaronleventhal)
(Reporter)

Comment 6

11 years ago
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
(Reporter)

Comment 7

11 years ago
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() ?
(Reporter)

Comment 8

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

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

Comment 9

11 years ago
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.
(Reporter)

Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

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

Comment 16

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

Updated

11 years ago
Duplicate of this bug: 455482
(Reporter)

Comment 18

11 years ago
(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.
(Reporter)

Comment 19

11 years ago
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.
I am working on a visual diff tool now. I hope to have something working by the beginning of next week.
(Assignee)

Comment 21

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

Comment 22

11 years ago
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.
(Reporter)

Comment 23

11 years ago
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.
(Reporter)

Comment 24

11 years ago
Basically, we need Eitan's tool and then we can be sure to do things right.
(Reporter)

Comment 25

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

Comment 26

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

Comment 27

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

Comment 28

11 years ago
Posted patch patch2 (obsolete) — Splinter Review
up to dated to trunk
Attachment #340298 - Attachment is obsolete: true
Attachment #342234 - Flags: review?(aaronleventhal)
Attachment #340298 - Flags: review?(aaronleventhal)
(Assignee)

Updated

11 years ago
Attachment #342234 - Flags: review?(marco.zehe)
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.
Attachment #342234 - Flags: review?(marco.zehe) → review-
(Assignee)

Comment 30

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

Comment 32

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

Comment 33

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

Updated

11 years ago
Blocks: 459353
(Reporter)

Comment 34

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

Comment 35

11 years ago
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.
(Reporter)

Comment 36

11 years ago
Okay, I have updated https://developer.mozilla.org/en/ARIA_User_Agent_Implementors_Guide#Name_Computation
I believe it is satisfactory now.
(Reporter)

Updated

11 years ago
Attachment #342234 - Flags: review?(aaronleventhal)
(Reporter)

Comment 37

11 years ago
Comment on attachment 342234 [details] [diff] [review]
patch2

Awaiting new patch.

Comment 38

10 years ago
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
(Reporter)

Updated

10 years ago
Flags: wanted1.9.1?
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?
(Assignee)

Comment 40

10 years ago
(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.
(Reporter)

Comment 41

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

Comment 42

10 years ago
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.
(Reporter)

Comment 43

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

Comment 44

10 years ago
(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.
(Reporter)

Comment 45

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

Comment 46

10 years ago
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?
(Reporter)

Comment 47

10 years ago
"User-entered value" was intended to mean the same thing as nsIAcc::value.
(Assignee)

Comment 48

10 years ago
(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?
(Reporter)

Comment 49

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

Comment 50

10 years ago
Posted patch wip (obsolete) — Splinter Review
1) eFromValue constants are temporary to make correct names for linkable accessibles.
2) test_name_markup fails due to wrong mutation events
Attachment #342234 - Attachment is obsolete: true
(Assignee)

Comment 51

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

Comment 52

10 years ago
(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)?
(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?
(Reporter)

Comment 54

10 years ago
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.
(Reporter)

Comment 55

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

Comment 56

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

Comment 57

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

Comment 58

10 years ago
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
(Reporter)

Comment 59

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

Comment 60

10 years ago
(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?
(Reporter)

Comment 61

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

Comment 62

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

Comment 63

10 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #361116 - Attachment is obsolete: true
Attachment #361758 - Flags: review?(marco.zehe)
(Assignee)

Updated

10 years ago
Attachment #361758 - Flags: review?(david.bolter)
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".
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.
(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.
(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?
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?
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!
(Assignee)

Comment 70

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

Comment 72

10 years ago
Posted patch patch2Splinter Review
Attachment #361758 - Attachment is obsolete: true
Attachment #362551 - Flags: review?(david.bolter)
Attachment #361758 - Flags: review?(marco.zehe)
(Assignee)

Updated

10 years ago
Attachment #362551 - Flags: review?(marco.zehe)

Updated

10 years ago
Attachment #362551 - Flags: review?(marco.zehe) → review+
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.
(Assignee)

Comment 74

10 years ago
(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?
Sorry, was thinking about bug 462137, but that concerns the two types of visibility on descriptions, not names.
(Assignee)

Comment 76

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

Comment 77

10 years ago
(In reply to comment #76)

> It sounds (bug 462136 comment #2)

should be bug 462137 comment #2
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?
(Assignee)

Comment 79

10 years ago
(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 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)
Attachment #362551 - Flags: review?(david.bolter) → review+
(Assignee)

Comment 81

10 years ago
http://hg.mozilla.org/mozilla-central/rev/c22d817948a7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 480099
Depends on: 480697

Updated

10 years ago
Depends on: 503856
You need to log in before you can comment on or make changes to this bug.