875665 - doAction(0) in <textarea/> accessible raises error

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Go to http://facebook.com and log in if you aren't already.
2. Find and double-tap the "What are you doing?" text field. Note that TalkBack speaks it as a text field and not a textarea.

Result:
1. TalkBack says "Editing", and I assume the keyboard becomes visible, too.
2. Half a second later or so, TalkBack says "Navigating", and the keyboard disappears again.

Facebook has replaced the text field with a whole set of widgets including post and cancel buttons above, and a couple of unlabeled other buttons below, a newly appeared text area where one can enter a status.

Continuing with the STR:
3. Find that text area and double-tap.

Expected: Keyboard should come up so I can enter my status.
Actual: Nothing happens.

Comment 1

[Marco Zehe (:MarcoZ)]

Another place where this reproduces is Twitter:
1. Go to http://mobile.twitter.com and log in.
2. Double-tap "New Tweet".
3. Find and double-tap the text area for the tweet.

Same result: Keyboard does not come up.

Comment 2

[Marco Zehe (:MarcoZ)]

A place where this does not reproduce is http://alpha.app.net. When logged in, the textarea to post a status can be double-tapped and the keyboard comes up.

Wild guess: contentEditable mapped to a multi-line text field via ARIA on Facebook and Twitter, versus a real textarea element on app.net?

Comment 3

[Eitan Isaacson [:eeejay]]

This is failing for me:
doing doAction(0) on a textarea, even when the actionCount is 1 ('activate') brings up an NS_ERROR_FAILURE for me. I'll investigate further, and come up with either a mochitest or a patch.

Comment 4

[Marco Zehe (:MarcoZ)]

Interesting! Other screen readers probably just set focus and don't use the actions for this type of control.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Failing test case for textarea

Here is a test. Anyone could pick this up. Or I'll get to it next week.

Comment 6

[Marco Zehe (:MarcoZ)]

CC'ing Surkov in case he has a quick idea.

Comment 7

[alexander :surkov (:asurkov)]

because textarea isn't HTMLInputElement (http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/HTMLFormControlAccessible.cpp#456)

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Fix activate action for <textarea/> and add action tests for text input elements.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 755058 [details] [diff] [review]
Fix activate action for <textarea/> and add action tests for text input elements.

Review of attachment 755058 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +458,5 @@
>        return element->Focus();
>  
> +    nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea(do_QueryInterface(mContent));
> +    if (textArea)
> +      return textArea->Focus();

I'd rather added FromContent to HTMLElement class which implements nsIDOMHTMLElement defining Focus() method (for example see http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.h#64)

::: accessible/tests/mochitest/actions.js
@@ +9,3 @@
>  
>  const CLICK_EVENTS = MOUSEDOWN_EVENT | MOUSEUP_EVENT | CLICK_EVENT;
> +const ALL_EVENTS = CLICK_EVENTS | COMMAND_EVENT | FOCUS_EVENT;

I'd rename it to XUL_EVENTS since "command" event belongs to XUL world. In opposite to XUL_EVENTS you can have HTML_EVENTS.

We don't have tests where we invoke an action twice (thus no focus event in second case) and probably we don't have tests where we invoke a link action that opens a new window so having FOCUS_EVENT is ALL_EVENTS works but I really wouldn't include it. If you like then you could have HTML_CONTROLS_EVENTS or simply use HTML_EVENTS | FOCUS_EVENT.

::: accessible/tests/mochitest/actions/test_controls.html
@@ +61,5 @@
> +        },
> +        {
> +          ID: "textinput",
> +          actionName: "activate",
> +          events: FOCUS_EVENT

then it's not clear ALL_EVENTS change

@@ +103,5 @@
>    </fieldset>
> +
> +  <textarea id="textarea" placeholder="What's happening?"></textarea>
> +
> +  <input id="textinput" type="text"/>

you don't need to have trailing '/' to close tag since you are in HTML file

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #9)
> Comment on attachment 755058 [details] [diff] [review]
> Fix activate action for <textarea/> and add action tests for text input
> elements.
> 
> Review of attachment 755058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/html/HTMLFormControlAccessible.cpp
> @@ +458,5 @@
> >        return element->Focus();
> >  
> > +    nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea(do_QueryInterface(mContent));
> > +    if (textArea)
> > +      return textArea->Focus();
> 
> I'd rather added FromContent to HTMLElement class which implements
> nsIDOMHTMLElement defining Focus() method (for example see
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> nsGenericHTMLElement.h#64)
> 

I a don't understand exactly what you mean. I could use FromContent(), of nsGenericHTMLElement or nsGenericHTMLFormElement, but they do not implement or inherit from nsIDOMHTMLElement, so Focus() is not available (at least not the one that returns an nsresult).

I took the liberty of simply calling Accessible::TakeFocus(), since we don't really check the element type in the other action methods anyway. For example, ActionCount() always returns one no matter the type.

> ::: accessible/tests/mochitest/actions.js
> @@ +9,3 @@
> >  
> >  const CLICK_EVENTS = MOUSEDOWN_EVENT | MOUSEUP_EVENT | CLICK_EVEN    aName.AssignLiteral("activate");
    return NS_OK;
  }T;
> > +const ALL_EVENTS = CLICK_EVENTS | COMMAND_EVENT | FOCUS_EVENT;
> 
> I'd rename it to XUL_EVENTS since "command" event belongs to XUL world. In
> opposite to XUL_EVENTS you can have HTML_EVENTS.
> 

I renamed it to XUL_EVENTS (even though it is not related to this bug).

> We don't have tests where we invoke an action twice (thus no focus event in
> second case) and probably we don't have tests where we invoke a link action
> that opens a new window so having FOCUS_EVENT is ALL_EVENTS works but I
> really wouldn't include it. If you like then you could have
> HTML_CONTROLS_EVENTS or simply use HTML_EVENTS | FOCUS_EVENT.
> 
> ::: accessible/tests/mochitest/actions/test_controls.html
> @@ +61,5 @@
> > +        },
> > +        {
> > +          ID: "textinput",
> > +          actionName: "activate",
> > +          events: FOCUS_EVENT
> 
> then it's not clear ALL_EVENTS change

I added it to ALL_EVENTS because the name suggested it should be added :) I renamed it to XUL_EVENTS and removed it.

> 
> @@ +103,5 @@
> >    </fieldset>
> > +
> > +  <textarea id="textarea" placeholder="What's happening?"></textarea>
> > +
> > +  <input id="textinput" type="text"/>
> 
> you don't need to have trailing '/' to close tag since you are in HTML file

Right...

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Bug 875665 - Fix activate action for <textarea/> and add action tests for text input elements.

I hope this is better.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 756030 [details] [diff] [review]
Bug 875665 - Fix activate action for <textarea/> and add action tests for text input elements.

Review of attachment 756030 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #10)

> > I'd rather added FromContent to HTMLElement class which implements
> > nsIDOMHTMLElement defining Focus() method (for example see
> > http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> > nsGenericHTMLElement.h#64)
> > 
> 
> I a don't understand exactly what you mean. I could use FromContent(), of
> nsGenericHTMLElement or nsGenericHTMLFormElement, but they do not implement
> or inherit from nsIDOMHTMLElement, so Focus() is not available (at least not
> the one that returns an nsresult).

I meant you can implement FromContent on HTMLElement class, that class implements nsIDOMHTMLElement which you need.

Comment 14

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5ab810a451

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2a5ab810a451