Closed Bug 453371 Opened 16 years ago Closed 16 years ago

textarea accessible name does not show current value when it has a text child

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(1 file, 4 obsolete files)

Proposed patch
1.99 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3pre) Gecko/2008090215 Minefield/3.0.3pre

If an HTML <textarea> is nested in a <label>, and it has default text provided as a child node, the accessible name will remain static and not change when the value of the textarea changes.

Reproducible: Always

Steps to Reproduce:
1. Load a page with a textarea with default text and an associated label.
2. Examine the textarea accessible's name
3. Change the text in the textarea
4. Examine the name again.
Actual Results:  
The name remains the same

Expected Results:  
The name should reflect the changes in the textarea.

The expected behavior is pending on bug #452767.
Attached patch mochitest (obsolete) — — Splinter Review 
This mochitest compares the accessible name before and after the textarea was modified. If the textarea has no children, the test passes, if it does, it fails.
Attached patch Proposed patch (obsolete) — — Splinter Review 
I'm not too happy with this, but I can't think of anything more elegant at the moment.
Attachment #336581 - Flags: review+
Attachment #336581 - Flags: review+ → review?(surkov.alexander)
Eitan, patch looks ok but could you merge your mochitest with existing test_nsIAccessible_name.html, also you feel free to file combined patch (code fix + mochitest).
Assignee: nobody → eitan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Alex,

I'm still using CVS, i'll switch to hg and recreate the patch as you requested.
Attached patch Proposed patch (obsolete) — — Splinter Review 
This is the fix and a mochitest addition to test_nsIAccessible_name.html.
Attachment #336565 - Attachment is obsolete: true
Attachment #336581 - Attachment is obsolete: true
Attachment #336674 - Flags: review?(surkov.alexander)
Attachment #336581 - Flags: review?(surkov.alexander)
Comment on attachment 336674 [details] [diff] [review]
Proposed patch

nit: please double check the style of your patch, use two spaces as indent.

>+      //////////////////////////////////////////////////////////////////////////
>+      // textarea name
>+
>+      // textarea's name should have the value, 
>+	  // which initially is the text child

nit: specified by a text child, probably?

>+	  testName("textareawithchild", "Story: Foo");

nit: add new line here

>+      var elem = document.getElementById("textareawithchild");
>+	  elem.value = "Bar";
>+	  // new textarea name should reflect the value change. 

nit: move this comment before value changing

>+	  testName("textareawithchild", "Story: Bar");

if it should reflect the value change, why doesn't it reflect? :)

I would like to look at new patch before r+
Attachment #336674 - Flags: review?(surkov.alexander)
Comment on attachment 336674 [details] [diff] [review]
Proposed patch

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff -r f96f0b755e14 accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp	Wed Sep 03 12:22:16 2008 -0400
>+++ b/accessible/src/base/nsAccessible.cpp	Wed Sep 03 10:59:45 2008 -0700
>@@ -1706,7 +1706,12 @@
>   // Append all the text into one flat string
>   PRUint32 numChildren = 0;
>   nsCOMPtr<nsIDOMXULSelectControlElement> selectControlEl(do_QueryInterface(aContent));
>-  if (!selectControlEl) {  // Don't walk children of elements with options, just get label directly
>+  nsCOMPtr<nsIAtom> tag = aContent->Tag();
>+  
>+  if (!selectControlEl && tag != nsAccessibilityAtoms::textarea) {  

nit: there are extra spaces in the end of line

I would write:

aContent->Tag() != nsAccessibilityAtoms::textarea

because it doesn't make sense to store nsCOMPtr if selectControlEl is false

>+    // Don't walk children of elements with options, just get label directly

nit: please put here '.' (dot) or add 'and'

>+    // Don't traverse the children of a textarea, we want the value, not the
>+    // static text node.
>     numChildren = aContent->GetChildCount();
>   }
Attached patch Proposed patch (obsolete) — — Splinter Review 
I hope I got to all the feedback you gave. Especially the whitespaces.
Attachment #336674 - Attachment is obsolete: true
Attachment #336853 - Flags: review?(surkov.alexander)
(In reply to comment #6)
> if it should reflect the value change, why doesn't it reflect? :)

I'm not sure I understand this question. I wrote "should" because that is what the result of the test "should" be. :)
Comment on attachment 336853 [details] [diff] [review]
Proposed patch

>+  if (!selectControlEl && aContent->Tag(); != nsAccessibilityAtoms::textarea) {
The semicolon after "tag()" causes a compile error.
Attached patch Proposed patch — — Splinter Review 
Doh! Thanks Marco.
Attachment #336853 - Attachment is obsolete: true
Attachment #336870 - Flags: review?(surkov.alexander)
Attachment #336853 - Flags: review?(surkov.alexander)
Attachment #336870 - Flags: review?(surkov.alexander) → review+
Comment on attachment 336870 [details] [diff] [review]
Proposed patch

>+
>+      //////////////////////////////////////////////////////////////////////////
>+      // textarea name
>+
>+      // textarea's name should have the value, 
>+	  // which initially is specified by a text child.
>+	  testName("textareawithchild", "Story: Foo");

it seems you have tabs here still, use spaces only

>+      var elem = document.getElementById("textareawithchild");
>+	  // new textarea name should reflect the value change. 
>+	  elem.value = "Bar";
>+
>+	  testName("textareawithchild", "Story: Bar");
> 

here too

r=me with these nits fixed
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(In reply to comment #9)
> (In reply to comment #6)
> > if it should reflect the value change, why doesn't it reflect? :)
> 
> I'm not sure I understand this question. I wrote "should" because that is what
> the result of the test "should" be. :)

that's all ok, I misread the code
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: