604391 - Expose placeholder as name if name is otherwise empty

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

I'll move the patch over from the tracker.

Comment 1

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

Attachment: patch (has surkov r+)

Carried over from bug 545817.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to bug 545817 comment #20)
> (In reply to bug 545817 comment #19)
> 
> > it sounds we should have name empty check here as well. If we prefer aria and
> > label names then we should  do the same for placeholder stuffs. Do you agree?
> > 
> > if we have tests for thsi then we should add test for placehodler, though I
> > doubt we have them.
> > 
> > r=me with fixed/addressed
> 
> The patch tests this :)

Sorry I was unclear. I meant tests for the case of mContent->GetBindingParent(), where we would check that aria-label (for example) is preferred over accessible name from parent.

Comment 3

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

Given bug 459640, I think it is safest to use the placeholder text if it exists, before checking the binding parent.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Given bug 459640, I think it is safest to use the placeholder text if it
> exists, before checking the binding parent.

It might be not since "search using google has placeholder but it's exposed with different name now. Try NVDA, I expect it will say "google" but it should "search using google" or something simialr, more descriptive.

Comment 5

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

Maybe I should just check that we are HTML.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Maybe I should just check that we are HTML.

who's we?

either fix bug 459640 or move placeholder after parent name in the case of binding presence, sure I prefer the former.

Comment 7

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

I already have the latter in my local patch, I'm just unsure whether I'll regret it :)

Comment 8

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

Attachment: patch (gives binding parent precedence)

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 483958 [details] [diff] [review]
patch (gives binding parent precedence)

r=me, this looks good, thanks!

Comment 10

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

Comment on attachment 483958 [details] [diff] [review]
patch (gives binding parent precedence)

>diff --git a/accessible/src/base/nsAccessibilityAtomList.h b/accessible/src/base/nsAccessibilityAtomList.h
>--- a/accessible/src/base/nsAccessibilityAtomList.h
>+++ b/accessible/src/base/nsAccessibilityAtomList.h
>@@ -191,16 +191,17 @@ ACCESSIBILITY_ATOM(longDesc, "longdesc")
> ACCESSIBILITY_ATOM(max, "max") // XUL
> ACCESSIBILITY_ATOM(maxpos, "maxpos") // XUL
> ACCESSIBILITY_ATOM(minpos, "minpos") // XUL
> ACCESSIBILITY_ATOM(_moz_menuactive, "_moz-menuactive") // XUL
> ACCESSIBILITY_ATOM(multiline, "multiline") // XUL
> ACCESSIBILITY_ATOM(name, "name")
> ACCESSIBILITY_ATOM(onclick, "onclick")
> ACCESSIBILITY_ATOM(popup, "popup")
>+ACCESSIBILITY_ATOM(placeholder, "placeholder")
> ACCESSIBILITY_ATOM(readonly, "readonly")
> ACCESSIBILITY_ATOM(scope, "scope") // HTML table
> ACCESSIBILITY_ATOM(seltype, "seltype") // XUL listbox
> ACCESSIBILITY_ATOM(simple, "simple") // XLink
> ACCESSIBILITY_ATOM(src, "src")
> ACCESSIBILITY_ATOM(selected, "selected")
> ACCESSIBILITY_ATOM(summary, "summary")
> ACCESSIBILITY_ATOM(tabindex, "tabindex")
>diff --git a/accessible/src/html/nsHTMLFormControlAccessible.cpp b/accessible/src/html/nsHTMLFormControlAccessible.cpp
>--- a/accessible/src/html/nsHTMLFormControlAccessible.cpp
>+++ b/accessible/src/html/nsHTMLFormControlAccessible.cpp
>@@ -407,26 +407,35 @@ nsresult
> nsHTMLTextFieldAccessible::GetNameInternal(nsAString& aName)
> {
>   nsresult rv = nsAccessible::GetNameInternal(aName);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (!aName.IsEmpty())
>     return NS_OK;
> 
>-  if (!mContent->GetBindingParent())
>+  if (mContent->GetBindingParent()) {
>+    // XXX: bug 459640
>+    // There's a binding parent.
>+    // This means we're part of another control, so use parent accessible for
>+    // name. This ensures that a textbox inside of a XUL widget gets an
>+    // accessible name.
>+    nsAccessible* parent = GetParent();
>+    if (parent) {
>+      GetName(aName);
>+    }
>+  }
>+
>+  if (!aName.IsEmpty())
>     return NS_OK;
> 
>-  // XXX: bug 459640
>-  // There's a binding parent.
>-  // This means we're part of another control, so use parent accessible for name.
>-  // This ensures that a textbox inside of a XUL widget gets
>-  // an accessible name.
>-  nsAccessible* parent = GetParent();
>-  return parent ? parent->GetName(aName) : NS_OK;
>+  // text inputs and textareas might have useful placeholder text
>+  mContent->GetAttr(kNameSpaceID_None, nsAccessibilityAtoms::placeholder, aName);
>+
>+  return NS_OK;
> }
> 
> NS_IMETHODIMP nsHTMLTextFieldAccessible::GetValue(nsAString& _retval)
> {
>   PRUint32 state;
>   nsresult rv = GetStateInternal(&state, nsnull);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>diff --git a/accessible/tests/mochitest/test_name.html b/accessible/tests/mochitest/test_name.html
>--- a/accessible/tests/mochitest/test_name.html
>+++ b/accessible/tests/mochitest/test_name.html
>@@ -171,16 +171,24 @@
>       testName("checkbox", "Play the Haliluya sound when new mail arrives");
> 
>       testName("comboinend", "This day was sunny");
>       testName("combo6", "This day was");
> 
>       testName("textboxinend", "This day was sunny");
>       testName("textbox2", "This day was");
> 
>+      // placeholder
>+      testName("ph_password", "a placeholder");
>+      testName("ph_text", "a placeholder");
>+      testName("ph_textarea", "a placeholder");
>+      testName("ph_text2", "a label");
>+      testName("ph_textarea2", "a label");
>+      testName("ph_text3", "a label");
>+
>       SimpleTest.finish();
>     }
> 
>     SimpleTest.waitForExplicitFinish();
>     addA11yLoadEvent(doTest);
>   </script>
> 
> </head>
>@@ -192,16 +200,21 @@
>      title="mochitest for accessible name calculating">
>     Mozilla Bug 444279
>   </a><br>
>   <a target="_blank"
>      href="https://bugzilla.mozilla.org/show_bug.cgi?id=530081"
>      title="Clean up our tree walker ">
>     Mozilla Bug 530081
>   </a>
>+  <a target="_blank"
>+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=604391"
>+     title="Use placeholder as name if name is otherwise empty">
>+    Mozilla Bug 604391
>+  </a>
>   <p id="display"></p>
>   <div id="content" style="display: none"></div>
>   <pre id="test">
>   </pre>
> 
>   <!-- aria-label, simple label -->
>   <span id="btn_simple_aria_label" role="button" aria-label="I am a button"/>
>   <br/>
>@@ -416,10 +429,23 @@
>       </select></label>
> 
>     <label id="textboxinend">
>       This day was
>       <input id="textbox2" value="sunny">
>     </label>
>   </form>
> 
>+  <!-- placeholder  -->
>+  <input id="ph_password" type="password" value="" placeholder="a placeholder" />
>+  <input id="ph_text" type="text" placeholder="a placeholder" />
>+  <textarea id="ph_textarea" cols="5" placeholder="a placeholder"></textarea>  
>+
>+  <!-- placeholder does not win -->
>+  <input id="ph_text2" type="text" aria-label="a label" placeholder="meh" />
>+  <textarea id="ph_textarea2" cols="5" aria-labelledby="ph_text2" 
>+            placeholder="meh"></textarea>
>+
>+  <label for="ph_text3">a label</label>
>+  <input id="ph_text3" placeholder="meh" />
>+
> </body>
> </html>

Comment 11

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

Man I wish I could undo that!

Comment 12

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

Note the latest patch attached has a bug:
>+      GetName(aName);

Should be parent->GetName(aName);

Fixed locally, but there are other problems. Seems I've uncovered some mutual recursion.

Comment 13

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

Attachment: patch 2 (gives priority to binding parent)

Oh the mutual recursion was just from the bug I mentioned (forgot to rebuild toolkit).

We can fix bug 459640 when we have less high priority bugs I think.

Alexander do you mind giving this a final look?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 485460 [details] [diff] [review]
patch 2 (gives priority to binding parent)


>   if (!aName.IsEmpty())
>     return NS_OK;
> 
>-  if (!mContent->GetBindingParent())
>+  if (mContent->GetBindingParent())
>+  {

nit: no { on new line

>+    // XXX: bug 459640
>+    // There's a binding parent.
>+    // This means we're part of another control, so use parent accessible for name.
>+    // This ensures that a textbox inside of a XUL widget gets
>+    // an accessible name.

nit: please make sure you use 80 chars per line, no more than 80, no less than 80 (with word wrapping precision).

>+    nsAccessible* parent = GetParent();
>+    parent->GetName(aName);

if AT gets name for for hidden accessible (unattached from the tree) then you get a crash.

>+  }
>+
>+  if (!aName.IsEmpty())
>     return NS_OK;

it doesn't make sense to check name twice

with all of these fixed r=me

Comment 15

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

> 
> >+    nsAccessible* parent = GetParent();
> >+    parent->GetName(aName);
> 
> if AT gets name for for hidden accessible (unattached from the tree) then you
> get a crash.

Thanks!

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/3eddc7a81ee8