894485 - Don't rely on roles and click listeners for default doAction()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Problem 1: It is hard to determine 100% that an item is clickable or not, because:
 (a) No role was provided by the website author.
 (b) No click listener is directly registered with the element, instead the container listens for bubbled clicks.
 (c) Sometimes, no click events are added at all (see problem #2).

Proposed solution for problem 1:
doAction(0) should always synthesize events, regardless of actionCount. I don't think that is very disruptive, since doAction(0) on an accessible with an actionCount of 0 does not raise an error, and is currently simply a no-op.

Problem 2: Items on some mobile sites and apps don't use click listeners at all, instead, they rely entirely on touch events (good idea? probably not). So they listen for taps (touchstart->touchend) instead of clicks.

Proposed solution for problem 2: Send the exact series of events that is expected of a mobile UA, if touch events are supported: touchstart,mousedown,touchend,mouseup,click.

Comment 1

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #0)
> Proposed solution for problem 1:
> doAction(0) should always synthesize events, regardless of actionCount. I
> don't think that is very disruptive, since doAction(0) on an accessible with
> an actionCount of 0 does not raise an error, and is currently simply a no-op.
> 

Actually, it does raise an error. But I still think this is the best way forward.

Comment 2

[alexander :surkov (:asurkov)]

Jamie, your feedback is welcome

Comment 3

[alexander :surkov (:asurkov)]

exposing accessible event on every accessible is semantically wrong. If we exposed a click action on focusable items would it make a trick?

it'd be great if you put a small example of crazy things people do in the wild.

Comment 4

[alexander :surkov (:asurkov)]

alternatively we could do a magic value to make a click action on any thing (like doAction(-1)). It doesn't affect on existing actions. So if AT wants to click then it clicks, if it wants actions then it gets them.

concerning to tap events I don't mind, actually I'm surprised they aren't part of generic click routine and aren't maintained automatically.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #3)
> exposing accessible event on every accessible is semantically wrong.

Agreed. Also, this does not do away with correctly exposing actionCount and getActionName, since that helps ATs provide better usability.

> If we
> exposed a click action on focusable items would it make a trick?

Not really, mobile sites don't care about focus and are less likely to use tabindex.

> 
> it'd be great if you put a small example of crazy things people do in the
> wild.

If you go to mobile.southwest.com with a UA string of iOS Safari. I copied it here so you don't have to:
http://people.mozilla.com/~eisaacson/southwest.html

Comment 6

[Marco Zehe (:MarcoZ)]

Also, if you go to http://mobile.twitter.com with Firefox for Android or Firefox OS, or make desktop Firefox emulate a mobile browser, then log in, the tweets themselves are also touchable and open detail views, user profiles etc., and none of these items are focusable. This is purely designed for touch input which does not care about keyboard focus at all.

Comment 7

[James Teh [:Jamie]]

The general idea certainly makes sense.

Note that longdesc gets mapped to an action and I'm guessing you still want this "force activate" action to be usable in this case. This means there are two options:
1. Map "force activate" (just a placeholder name) to action 0, expose a name for it and include it in the action count. The name cannot be "click", since some ATs use this to determine whether to report "clickable" and we don't want everything reporting as clickable. Making it a discoverable action might seem weird, but if you don't, things get nasty with longdesc. (I'm guessing you'll want longdesc to be 1, not 0, but you can't exclude action 0 from the count and not give it a name in this case.)

2. Use a negative action index. Note that IA2 already has some negative action indexes, so mapping to IA2 will require special code unless this is accounted for.

Mick and I are leaning towards option 1. However, it could be argued that having a discoverable action which may not actually be valid or do anything at all is wrong. If you do go for option 1, the name must clearly indicate that this isn't something the object is choosing to expose. We can probably do better than "force activate", but that's all I could come up with for now.

One concern is what happens if events are synthesised on an element which is obscured or off-screen.

Comment 8

[alexander :surkov (:asurkov)]

Wouldn't option 1 lead to extra noise from AT? Or do AT tend to ignore unknown actions? If we go with 1st option then I wouldn't make it 0 action since it becomes a default action, probably it'd be strange to see "force activate" instead "expand" or "collapse" on tree item.

Personally I think I prefer 2nd option, negative index approach seems to applied well over IA2 negative actions.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #8)
> Wouldn't option 1 lead to extra noise from AT? Or do AT tend to ignore
> unknown actions? If we go with 1st option then I wouldn't make it 0 action
> since it becomes a default action, probably it'd be strange to see "force
> activate" instead "expand" or "collapse" on tree item.
> 
> Personally I think I prefer 2nd option, negative index approach seems to
> applied well over IA2 negative actions.

What about using the action count as the index? If there are other actions defined then doAction(0) would perform the default. If there are no actions defined than doAction(0) will force activate. If you want to always force activate you do acc.doAction(acc.actionCount).

Or if you want to keep it semantically correct, *all* accessibles would have "force activate" appended to the end of their given action list. So that you would always have an actionCount of 1 or more.

Side note: yeah, "force activate" is probably a bad name, it should be simply "click". And what is now known as "click" should be renamed "activate". The current action names confuse input and intent - you 'click' (input) a mouse button to 'activate' (intent) a control. This is inconsistent with links, for example where you 'jump' (intent).

There are probably historical reasons for this, and we are not going to change that now, but it would allow us to be more accurate as to what the "force" action is all about.

Comment 10

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #8)
> Wouldn't option 1 lead to extra noise from AT? Or do AT tend to ignore
> unknown actions?
NVDA doesn't tell the user about the action unless they perform it. The only exception is "click", where NVDA will inform the user that it is clickable.

> If we go with 1st option then I wouldn't make it 0 action
> since it becomes a default action, probably it'd be strange to see "force
> activate" instead "expand" or "collapse" on tree item.
I was envisioning "force activate" wouldn't be present if there was already an activate action such as "expand", "jump", etc. In other words, if it's anything other than "longdesc", there's no "force activate".

> What about using the action count as the index? If there are other actions
> defined then doAction(0) would perform the default.
I'd argue longdesc is never a default action where some sort of activation is possible. I guess I see the default action as some sort of activation.

> Side note: yeah, "force activate" is probably a bad name, it should be
> simply "click".
Yes, but according to your proposal, "force activate" will also generate touch events, so "click" is wrong.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #10)

> > If we go with 1st option then I wouldn't make it 0 action
> > since it becomes a default action, probably it'd be strange to see "force
> > activate" instead "expand" or "collapse" on tree item.
> I was envisioning "force activate" wouldn't be present if there was already
> an activate action such as "expand", "jump", etc. In other words, if it's
> anything other than "longdesc", there's no "force activate".

I can live with this but implementation might be tricky.

What is downside of negative action index introduction like

IA2_ACTION_FORCEACTIVATE = -4

I can see only one issue if it's rejected by IA2 group and other IA2 at -4 index is introduced. On the other hand it's easy to implement also and it seems I like it more.

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #10)
> > What about using the action count as the index? If there are other actions
> > defined then doAction(0) would perform the default.
>
> I'd argue longdesc is never a default action where some sort of activation
> is possible. I guess I see the default action as some sort of activation.
>

Longdesc could be the exception to that rule.
 
> > Side note: yeah, "force activate" is probably a bad name, it should be
> > simply "click".
> Yes, but according to your proposal, "force activate" will also generate
> touch events, so "click" is wrong.

Not really, if an object is not semantically an interactive object, there is no intent when you tap/click it. It is the same case as a sighted user who encounters a poorly designed UI, they will helplessly click on any object without knowing what it is, and what it does, is that a button? checkbox? link? just text?

Actually, my new proposal for a name instead of "force activate" is "poke" :) That describes better what a user is attempting to do in an input-agnostic way.

So a new proposal: Any non interactive item (including images with longdesc) gets an action at index 0 called "poke".

tbh, I'm happy with any of these proposals so far.

Comment 13

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #11)
> I can live with this but implementation might be tricky.
I guess the idea is that there would always be an action 0 which tries to activate the object. If the object can be activated for certain, then 0 is "click", "jump", etc. If it can't, then it's "poke" or whatever.

> What is downside of negative action index introduction like
> IA2_ACTION_FORCEACTIVATE = -4
This means the action would never be treated as the default action for an object. Whether or not this is a good thing is controversial. :)

(In reply to Eitan Isaacson [:eeejay] from comment #12)
> Not really, if an object is not semantically an interactive object, there is
> no intent when you tap/click it.
That wasn't my point. I was saying that "click" is specific to mouse input; you can't "click" an object on a touch screen. Yes, I'm being pedantic. Are you surprised? :)

> Actually, my new proposal for a name instead of "force activate" is "poke"
> :) That describes better what a user is attempting to do in an
> input-agnostic way.
Sort of, although poke doesn't lead me to think it might activate something. I think of poking as a more investigative action that might trigger a small (but not massive) reaction. That said, it's still probably better than "force activate". :)

> So a new proposal: Any non interactive item (including images with longdesc)
> gets an action at index 0 called "poke".
That works for me. However, the negative index also works for me if that's preferred.

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #13)
> (In reply to Eitan Isaacson [:eeejay] from comment #12)
> > Not really, if an object is not semantically an interactive object, there is
> > no intent when you tap/click it.
> That wasn't my point. I was saying that "click" is specific to mouse input;
> you can't "click" an object on a touch screen. Yes, I'm being pedantic. Are
> you surprised? :)

Not the least.

> > Actually, my new proposal for a name instead of "force activate" is "poke"
> > :) That describes better what a user is attempting to do in an
> > input-agnostic way.
> Sort of, although poke doesn't lead me to think it might activate something.
> I think of poking as a more investigative action that might trigger a small
> (but not massive) reaction. That said, it's still probably better than
> "force activate". :)

I think it is a good parallel to bad UI with limited perceptibility, visual or not. You end up poking it to elicit any kind of reaction, good or bad. The consequences may even be dire. I'm being hyperbolic. Are you surprised? :)

It goes without saying that web apps that end up having interactive controls with "poke" actions should do a better job and mark the type of object it is with a role.

Comment 15

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> I think it is a good parallel to bad UI with limited perceptibility, visual
> or not. You end up poking it to elicit any kind of reaction, good or bad.
> The consequences may even be dire. I'm being hyperbolic. Are you surprised?
Touche. If we're being hyperbolic, why not "catastrophic"? :) I'm happy enough with the name "poke".

> It goes without saying that web apps that end up having interactive controls
> with "poke" actions should do a better job and mark the type of object it is
> with a role.
Agreed. This really is a "give the user a chance to cope with poorly authored web pages" function.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #13)
> (In reply to alexander :surkov from comment #11)
> > I can live with this but implementation might be tricky.
> I guess the idea is that there would always be an action 0 which tries to
> activate the object. If the object can be activated for certain, then 0 is
> "click", "jump", etc. If it can't, then it's "poke" or whatever.

I have two concerns

1) "poke" is not usual action since when you invoke nothing may happen. It's not a characteristic of the object, it's rather hit testing
2) keeping it as default action is not backward compatible in general, probably not a big deal but something we should keep in mind making the change

> > What is downside of negative action index introduction like
> > IA2_ACTION_FORCEACTIVATE = -4
> This means the action would never be treated as the default action for an
> object. Whether or not this is a good thing is controversial. :)

Right but do we really need to have "hit somewhere at the object's area" as default action?

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Have touch events precede mouse events to simulate touch devices.

This solves the issue where a website will listen to touch events and not mouse events.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 778715 [details] [diff] [review]
Have touch events precede mouse events to simulate touch devices.

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

::: accessible/src/base/nsCoreUtils.cpp
@@ +117,5 @@
>    DispatchMouseEvent(NS_MOUSE_BUTTON_DOWN, cnvdX, cnvdY,
>                       tcContent, tcFrame, presShell, rootWidget);
>  
>    DispatchMouseEvent(NS_MOUSE_BUTTON_UP, cnvdX, cnvdY,
>                       tcContent, tcFrame, presShell, rootWidget);

you need to fix these to make XUL trees working

@@ +121,5 @@
>                       tcContent, tcFrame, presShell, rootWidget);
>  }
>  
> +void
> +nsCoreUtils::DispatchClickEvent(nsIPresShell *aPresShell, nsIContent *aContent)

nit: type* name

but it seems like you can move the function body into caller directly

@@ +178,5 @@
> +  nsTouchEvent event(true, aEventType, aRootWidget);
> +
> +  event.time = PR_IntervalNow();
> +
> +  // XXX: Touch has an identifier of -1 to hint that it is synthesized.

why is XXX?

Comment 19

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #18)
> Comment on attachment 778715 [details] [diff] [review]
> Have touch events precede mouse events to simulate touch devices.
> 
> Review of attachment 778715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsCoreUtils.cpp
> @@ +117,5 @@
> >    DispatchMouseEvent(NS_MOUSE_BUTTON_DOWN, cnvdX, cnvdY,
> >                       tcContent, tcFrame, presShell, rootWidget);
> >  
> >    DispatchMouseEvent(NS_MOUSE_BUTTON_UP, cnvdX, cnvdY,
> >                       tcContent, tcFrame, presShell, rootWidget);
> 
> you need to fix these to make XUL trees working

I didn't touch DispatchMouseEvent() so it should just work fine, no?

> 
> @@ +121,5 @@
> >                       tcContent, tcFrame, presShell, rootWidget);
> >  }
> >  
> > +void
> > +nsCoreUtils::DispatchClickEvent(nsIPresShell *aPresShell, nsIContent *aContent)
> 
> nit: type* name
> 
> but it seems like you can move the function body into caller directly

I'm not sure what you mean. All this should go into Accessible::DispatchClickEvent()?

> 
> @@ +178,5 @@
> > +  nsTouchEvent event(true, aEventType, aRootWidget);
> > +
> > +  event.time = PR_IntervalNow();
> > +
> > +  // XXX: Touch has an identifier of -1 to hint that it is synthesized.
> 
> why is XXX?

When receiving a touch event in content, there is no way to know what the input source was (or if the event was synthesized). In Mouse events it is possible with mozInputSource.

So instead I give it a negative identifier (which is completely legal). That way, when we capture the event in AccessFu, we let it through to the content and don't cancel it like we do with real touch events.

So, the XXX suggests this is just a hack. When we migrate to pointer events, we will have an attribute in the PointerEvent interface called pointerType[1] that takes a string, and will allow us to differentiate the sources of events.

http://www.w3.org/TR/pointerevents/#widl-PointerEventInit-pointerType

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #19)
> (In reply to alexander :surkov from comment #18)

> > you need to fix these to make XUL trees working
> 
> I didn't touch DispatchMouseEvent() so it should just work fine, no?

ah, so you don't need touch events for XUL trees, correct? Though it might be good to have them for consistency.

> > 
> > @@ +121,5 @@
> > >                       tcContent, tcFrame, presShell, rootWidget);
> > >  }
> > >  
> > > +void
> > > +nsCoreUtils::DispatchClickEvent(nsIPresShell *aPresShell, nsIContent *aContent)
> > 
> > nit: type* name
> > 
> > but it seems like you can move the function body into caller directly
> 
> I'm not sure what you mean. All this should go into
> Accessible::DispatchClickEvent()?

right, because Accessible::DispatchClickEvent() is simple wrapper around nsCoreUtils::DispatchClickEvent and nsCoreUtils method isn't used anywhere except Accessible.

Comment 21

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #20)
> (In reply to Eitan Isaacson [:eeejay] from comment #19)
> > (In reply to alexander :surkov from comment #18)
> 
> > > you need to fix these to make XUL trees working
> > 
> > I didn't touch DispatchMouseEvent() so it should just work fine, no?
> 
> ah, so you don't need touch events for XUL trees, correct? Though it might
> be good to have them for consistency.
> 

I'll add it. But since I have no experience with XUL, I don't know if it is the right thing to do. FWIW, there is no XUL in our mobile offerings.

> > > 
> > > @@ +121,5 @@
> > > >                       tcContent, tcFrame, presShell, rootWidget);
> > > >  }
> > > >  
> > > > +void
> > > > +nsCoreUtils::DispatchClickEvent(nsIPresShell *aPresShell, nsIContent *aContent)
> > > 
> > > nit: type* name
> > > 
> > > but it seems like you can move the function body into caller directly
> > 
> > I'm not sure what you mean. All this should go into
> > Accessible::DispatchClickEvent()?
> 
> right, because Accessible::DispatchClickEvent() is simple wrapper around
> nsCoreUtils::DispatchClickEvent and nsCoreUtils method isn't used anywhere
> except Accessible.

Done.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #21)
> (In reply to alexander :surkov from comment #20)
> > (In reply to Eitan Isaacson [:eeejay] from comment #19)
> > > (In reply to alexander :surkov from comment #18)
> > 
> > > > you need to fix these to make XUL trees working
> > > 
> > > I didn't touch DispatchMouseEvent() so it should just work fine, no?
> > 
> > ah, so you don't need touch events for XUL trees, correct? Though it might
> > be good to have them for consistency.
> > 
> 
> I'll add it. But since I have no experience with XUL, I don't know if it is
> the right thing to do. FWIW, there is no XUL in our mobile offerings.

ok, then maybe comment why we don't want touch events here is enough

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Bug 894485 - Have touch events precede mouse events to simulate touch devices.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 779987 [details] [diff] [review]
Bug 894485 - Have touch events precede mouse events to simulate touch devices.

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

::: accessible/src/base/nsCoreUtils.cpp
@@ +143,5 @@
>  
> +void
> +nsCoreUtils::DispatchTouchEvent(uint32_t aEventType, int32_t aX, int32_t aY,
> +                                nsIContent *aContent, nsIFrame *aFrame,
> +                                nsIPresShell *aPresShell, nsIWidget *aRootWidget)

nit: type* name

::: accessible/src/base/nsCoreUtils.h
@@ +67,5 @@
> +   * @param aEventType   [in] an event type (see nsGUIEvent.h for constants)
> +   * @param aX           [in] x coordinate in dev pixels
> +   * @param aY           [in] y coordinate in dev pixels
> +   * @param aContent     [in] the element
> +   * @param aRootWidget  [in] the root widget of the element

presshell is missed in arguments description

@@ +71,5 @@
> +   * @param aRootWidget  [in] the root widget of the element
> +   */
> +  static void DispatchTouchEvent(uint32_t aEventType, int32_t aX, int32_t aY,
> +                                 nsIContent *aContent, nsIFrame *aFrame,
> +                                 nsIPresShell *aPresShell, nsIWidget *aRootWidget);

nit: type* name

::: accessible/src/generic/Accessible.cpp
@@ +2286,5 @@
>                                     nsIPresShell::ScrollAxis(),
>                                     nsIPresShell::ScrollAxis(),
>                                     nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
>  
> +  nsIFrame *frame = aContent->GetPrimaryFrame();

nit: type* name

Comment 25

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6192b162ba2

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6192b162ba2

Comment 27

[Eitan Isaacson [:eeejay]]

Oops, this was supposed to stay opened for solving problem #1 in the summary.

Comment 28

[Eitan Isaacson [:eeejay]]

I'm going to mark this as fixed for now since this hasn't been touched in 4 years. If we want to disassociate roles from actions we could do that in a future bug.