420863 - If an HTML element has an onClick attribute, expose its click action on the element rather than its child text leaf node.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This morning, Jamie of NV Access and I discussed OnClick attributes on HTML elements. In his example, an onclick handler was present on an html:td element. He then discovered the following (copied from IRC log):

<Jamie> MarcoZ: hmm... ok... here's the scoop on that clickable thing   
<Jamie> MarcoZ: The "click" action is exposed on the text node, but for links, it's on the link. this means we have to drill down to the text node in order ot trigger the action   
<Jamie> for a clickable table cell   
<Jamie> This makes for inconsistency in terms of code, but also, we're no longer using the IAccessible text child nodes when we move around the document   

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Testcase

This testcase shows an onclick handler on the first table cell, and nothing on the second. If you look at the hierarchy using accProbe, you'll see that the text leaf child of the table cell with the onclick handler has an action of "click", but the table cell itself does not. So every screen reader is required to drill down to the text leaf accessible to seee that the td has an onclick handler.

Comment 2

[alexander :surkov (:asurkov)]

may be fixed in bug 421066

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

expose actions if element has registered click event handler by default

Comment 4

[alexander :surkov (:asurkov)]

Marco, could you morph your testcase to mochitest?

Comment 5

[Aaron Leventhal]

Comment on attachment 308824 [details] [diff] [review]
patch

r+ but you only need to calculate isOnclick if aIndex == 0

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 308824 [details] [diff] [review]
patch

I will fix this before checking in

Comment 7

[Damon Sicore (:damons)]

Can we get that mochitest?  Waiting on approval until we can get a test if at all possible.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

with Aaron's comments and mochitest

Comment 9

[Marco Zehe (:MarcoZ)]

Thanks for adding theMochitest, Alexander! Here at CSUN, tthings are pretty busy, so I appreciate the help!

Comment 10

[Marco Zehe (:MarcoZ)]

A question: In the code block

> +  PRBool isOnclick = nsAccUtils::HasListener(content,
> +                                             NS_LITERAL_STRING("click"));

Would it be too difficult to add a check for MouseDown as well? Gmail uses that instead of "click", and it would allow NVDA to recognize those as clickables as well. Or should this go into a separate bug?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> A question: In the code block
> 
> > +  PRBool isOnclick = nsAccUtils::HasListener(content,
> > +                                             NS_LITERAL_STRING("click"));
> 
> Would it be too difficult to add a check for MouseDown as well? Gmail uses that
> instead of "click", and it would allow NVDA to recognize those as clickables as
> well. Or should this go into a separate bug?
> 

It's not hard but it seems to me it's not correct. Let see button for example, in XUL they register handlers on command event, command event is fired after mousedown/mouseup. It means action is executed after mousedown and if followin up mouseup event is handled.

Comment 12

[Marco Zehe (:MarcoZ)]

(In reply to comment #11)
> (In reply to comment #10)
> It's not hard but it seems to me it's not correct. Let see button for example,
> in XUL they register handlers on command event, command event is fired after
> mousedown/mouseup. It means action is executed after mousedown and if followin
> up mouseup event is handled.

I realize this is not how XUL does it, but it is what would allow screen readers to better access gmail and other similar web apps. But I now think this should go into a separate bug.

Comment 13

[alexander :surkov (:asurkov)]

But it's common thing: action is performed when widget is clicked, right?

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 309325 [details] [diff] [review]
patch2

Nice!  Really appreciate the test.  :)

a1.9+=damons

Comment 15

[alexander :surkov (:asurkov)]

/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.363; previous revision: 1.362
done
Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.8; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/accessible/tests/mochitest/test_bug420863.html,v
done
Checking in accessible/tests/mochitest/test_bug420863.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_bug420863.html,v  <--  test_bug420863.html
initial revision: 1.1
done

Comment 16

[alexander :surkov (:asurkov)]

Marco please open new bug to track mousedown issue

Comment 17

[alexander :surkov (:asurkov)]

patch contains mochitests, in-testsuite+