455482 - In a select nested within a label, the accname contains each option's text, should only contain label's text.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

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

the accName for the label, combobox, and combobox_list accessibles is:
"Search in: Newsticker Entire site".

However, it should only contain:
"Search in:"

If the select is moved outside the label and the label is associated with the select, everything is OK.

Comment 1

[Aaron Leventhal]

Also for ARIA we're looking at a proposal for accName subtree collection not to walk into descendant widget containers like listbox.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> In this construct:
>   <label>Search in:<select id="combo2" name="search">
>     <option>Newsticker</option>
>     <option>Entire site</option>
>   </select></label>
> 
> the accName for the label, combobox, and combobox_list accessibles is:
> "Search in: Newsticker Entire site".
> 
> However, it should only contain:
> "Search in:"

Or it should be "Search in: Newsticker" like we do for xul:menulist rightnow?

Comment 3

[alexander :surkov (:asurkov)]

It will be fixed with the bug 455886. Just we should remeber about it :)

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (In reply to comment #0)
> > In this construct:
> >   <label>Search in:<select id="combo2" name="search">
> >     <option>Newsticker</option>
> >     <option>Entire site</option>
> >   </select></label>
> > 
> > the accName for the label, combobox, and combobox_list accessibles is:
> > "Search in: Newsticker Entire site".
> > 
> > However, it should only contain:
> > "Search in:"
> 
> Or it should be "Search in: Newsticker" like we do for xul:menulist rightnow?

I just ask because it looks very similar to the example 
<label>blabla <input value="10"/></label>
and we include "10" into the input name.

Comment 5

[Aaron Leventhal]

Alex, good question.

Yes, I think the accessible name should contain the user input value. This is to make sure the user hears enough context in this case:

[ ] Search in [combobox] when you log in

If the author doesn't want the user input value in the name, they can always leave the control outside the <label>.

Comment 6

[Aaron Leventhal]

Alex, I have an idea. At first it sounds hacky but I think it actually might make sense.

The times when you need the value in the name is:
1. The control inside the label is being used to label another control
and
2. The control inside the is labeling itself but there is label content before *and* after it. This is to avoid an accName of "Delete messages after days" for "Delete messages after ____ days".

What I think we should do is this:
When a control inside a label, and the accName is being used to label that control, don't use the value if it's at the very beginning or end of the label.
IOW, treat these:
<label>[control] label-content</label>
<label>label-content [control]</label>
as if the control is not included in the accName.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Alex, I have an idea. At first it sounds hacky but I think it actually might
> make sense.
> 
> The times when you need the value in the name is:
> 1. The control inside the label is being used to label another control
> and

So this is like
<label>input: <input id="input" value="10"/></label>
<button aria-labelledby="input">btn</button>

the name for the button is "input: 10", right?

> 2. The control inside the is labeling itself but there is label content before
> *and* after it. This is to avoid an accName of "Delete messages after days" for
> "Delete messages after ____ days".

<label>input: <input value="10"/> input</label>

the name for input is "input: 10 input", right?

Comment 8

[alexander :surkov (:asurkov)]

removing dublicate becasue I'm not sure this bug is really part of the bug 455886 per Aaron's comment #6 since we could change the current algorithm.

Comment 9

[James Teh [:Jamie]]

(In reply to comment #6)
> When a control inside a label, and the accName is being used to label that
> control, don't use the value if it's at the very beginning or end of the label.
This is similar to the idea I proposed in comment 24 of bug 452767, except I only accounted for the value being at the end, not the beginning. Both is probably better. Imo, this is the solution to the more general problem described in bug 452767.

Comment 10

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

Just adding Eitan to cc list... he might want to take this one.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to comment #9)
> (In reply to comment #6)
> > When a control inside a label, and the accName is being used to label that
> > control, don't use the value if it's at the very beginning or end of the label.
> This is similar to the idea I proposed in comment 24 of bug 452767, except I
> only accounted for the value being at the end, not the beginning. Both is
> probably better. Imo, this is the solution to the more general problem
> described in bug 452767.

I would be happy to try this out. Patch coming soon.

Comment 12

[alexander :surkov (:asurkov)]

Eitan, I'm not sure this time is the right time to fix the code because we want to do reorganization of the name computation code in bug 455886. Ideally it also should handle this bug. Once we polish name calculation in ARIA guide (https://developer.mozilla.org/En/ARIA_User_Agent_Implementors_Guide) then I think I'll start to work on that bug.

Comment 13

[Eitan Isaacson [:eeejay]]

Ah, a day too late :)
I got a patch working about an hour ago. But I also saw your patch in bug 455886, and I agree that once that is applied to trunk, this bug will be a lot more straightforward to solve.

Oh well, it was educational. I will move on to something else ;)

Comment 14

[alexander :surkov (:asurkov)]

Ok, then put your patch to review, but please you add mochitests so that we will ensure we don't break it in bug 455886.

Comment 15

[Eitan Isaacson [:eeejay]]

Attachment: patch 1

This is untested and ugly. But what the heck, maybe it will be useful until we rework GetName.

This patch does the following: 
1. Only add control value to a label's name when it is in the middle of the label text.
2. Don't append all the option values when calculating a label with a nested select.
3. Test cases.

Comment 16

[alexander :surkov (:asurkov)]

Should we handle the following case as well:

<span id="label">Hello<input aria-labelledby="label">Hello</span>

Also, the current specification says (https://developer.mozilla.org/En/ARIA_User_Agent_Implementors_Guide):

If the current node is:
(i) not the node that the overall name is being computed for, or 
(ii) not at the very beginning or end of the element used to label it, then:
Append the current value for the node to the name

Therefore when we compute name for label in your mochitests then we should always include value from child control.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 344513 [details] [diff] [review]
patch 1

cancelling review until comments are addressed

Comment 18

[alexander :surkov (:asurkov)]

Eitan, it sounds the fist part of your patch is enough to fix this bug. The second part of your patch is for bug 433895 and bug 452767. No?

Comment 19

[Eitan Isaacson [:eeejay]]

Alex, yeah you are right. It should be in bug 452767. I'll split the patch and the test cases. I got confused because the discussion on bug 452767 seemed to move here.

Comment 20

[Eitan Isaacson [:eeejay]]

Attachment: patch 2

This patch only fixes the combobox bug: Don't append each option name.
Also the "combo in the middle" test is in this patch, since it is the only case that will not change when we fix bug 452767.

Comment 21

[Eitan Isaacson [:eeejay]]

(In reply to comment #16)
> If the current node is:
> (i) not the node that the overall name is being computed for, or 
> (ii) not at the very beginning or end of the element used to label it, then:
> Append the current value for the node to the name
> 
> Therefore when we compute name for label in your mochitests then we should
> always include value from child control.

You are right, I read that as "and" and not "or". Could we move this conversation to bug 452767?

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 344640 [details] [diff] [review]
patch 2

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff -r 89c97fd8a881 accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp	Fri Oct 24 09:07:02 2008 -0700
>+++ b/accessible/src/base/nsAccessible.cpp	Fri Oct 24 09:53:19 2008 -0700
>@@ -1623,11 +1623,16 @@
>   // Append all the text into one flat string
>   PRUint32 numChildren = 0;
>   nsCOMPtr<nsIDOMXULSelectControlElement> selectControlEl(do_QueryInterface(aContent));
>-  
>-  if (!selectControlEl && aContent->Tag() != nsAccessibilityAtoms::textarea) {
>+  nsCOMPtr<nsIAtom> tag = aContent->Tag();
>+
>+  if (!selectControlEl && 
>+      tag != nsAccessibilityAtoms::textarea && 
>+      tag != nsAccessibilityAtoms::select) {
>     // Don't walk children of elements with options, just get label directly.
>     // Don't traverse the children of a textarea, we want the value, not the
>     // static text node.
>+    // Don't traverse the children of a select element, we only want the
>+    // current value.
>     numChildren = aContent->GetChildCount();
>   }
> 
>diff -r 89c97fd8a881 accessible/tests/mochitest/test_nsIAccessible_name.html
>--- a/accessible/tests/mochitest/test_nsIAccessible_name.html	Fri Oct 24 09:07:02 2008 -0700
>+++ b/accessible/tests/mochitest/test_nsIAccessible_name.html	Fri Oct 24 09:53:19 2008 -0700
>@@ -128,6 +128,11 @@
> 
>       testName("textareawithchild", "Story: Bar");
> 
>+      /////////////////////////////////////////////////////////////////////////
>+      // label with nested combobox
>+
>+	  testName("comboinmiddle", "Subscribe to ATOM feed.");

nit: please make right indentation here.

>+	   

nit: not empty new line


>+  <!-- A label with a nested control in the middle -->
>+  <form>
>+    <label id="comboinmiddle">Subscribe to
>+	<select id="combo3" name="occupation">
>+      <option>ATOM</option>
>+      <option>RSS</option>
>+	</select>
>+	feed.
>+    </label>
>+  </form>

nit: right indentation here please as well.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 344640 [details] [diff] [review]
patch 2

asking additional review from marco

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> Created an attachment (id=344640) [details]
> patch 2
> 
> This patch only fixes the combobox bug: Don't append each option name.
> Also the "combo in the middle" test is in this patch, since it is the only case
> that will not change when we fix bug 452767.

I don't mind, just change bug summary to reflect taken approach.

Comment 25

[Marco Zehe (:MarcoZ)]

Comment on attachment 344640 [details] [diff] [review]
patch 2

r=me

Comment 26

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/13b5c75e3709

Comment 27

[Benjamin Smedberg]

This landed prior to branching