Closed Bug 455482 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: eeejay)

References

(Blocks 1 open bug)

Details

(Keywords: access, fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking1.9.1?
Also for ARIA we're looking at a proposal for accName subtree collection not to walk into descendant widget containers like listbox.
Depends on: 455886
(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?
It will be fixed with the bug 455886. Just we should remeber about it :)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 455886
(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.
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>.
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.
(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?
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.
Assignee: nobody → surkov.alexander
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(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.
Just adding Eitan to cc list... he might want to take this one.
(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.
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.
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 ;)
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.
Attached patch patch 1 (obsolete) — Splinter Review
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.
Attachment #344513 - Flags: review?(surkov.alexander)
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 on attachment 344513 [details] [diff] [review]
patch 1

cancelling review until comments are addressed
Attachment #344513 - Flags: review?(surkov.alexander)
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?
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.
Attached patch patch 2Splinter Review
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.
Attachment #344513 - Attachment is obsolete: true
(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 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.
Attachment #344640 - Flags: review+
Comment on attachment 344640 [details] [diff] [review]
patch 2

asking additional review from marco
Attachment #344640 - Flags: review?(marco.zehe)
(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 on attachment 344640 [details] [diff] [review]
patch 2

r=me
Attachment #344640 - Flags: review?(marco.zehe) → review+
http://hg.mozilla.org/mozilla-central/rev/13b5c75e3709
Assignee: surkov.alexander → eitan
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
This landed prior to branching
Flags: blocking1.9.1? → blocking1.9.1-
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.