Closed Bug 420863 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Disability Access APIs, defect, major)

x86
Windows Vista
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: MarcoZ, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

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   
Flags: blocking1.9?
Attached file 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.
may be fixed in bug 421066
Attached patch patch (obsolete) — Splinter Review
expose actions if element has registered click event handler by default
Assignee: aaronleventhal → surkov.alexander
Status: NEW → ASSIGNED
Attachment #308824 - Flags: review?(aaronleventhal)
Marco, could you morph your testcase to mochitest?
Flags: in-testsuite?
Comment on attachment 308824 [details] [diff] [review]
patch

r+ but you only need to calculate isOnclick if aIndex == 0
Attachment #308824 - Flags: review?(aaronleventhal) → review+
Comment on attachment 308824 [details] [diff] [review]
patch

I will fix this before checking in
Attachment #308824 - Flags: approval1.9?
Can we get that mochitest?  Waiting on approval until we can get a test if at all possible.
Attached patch patch2Splinter Review
with Aaron's comments and mochitest
Attachment #308824 - Attachment is obsolete: true
Attachment #309325 - Flags: approval1.9?
Attachment #308824 - Flags: approval1.9?
Thanks for adding theMochitest, Alexander! Here at CSUN, tthings are pretty busy, so I appreciate the help!
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?
(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.
(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.
But it's common thing: action is performed when widget is clicked, right?
Comment on attachment 309325 [details] [diff] [review]
patch2

Nice!  Really appreciate the test.  :)

a1.9+=damons
Attachment #309325 - Flags: approval1.9? → approval1.9+
/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

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Marco please open new bug to track mousedown issue
patch contains mochitests, in-testsuite+
Flags: in-testsuite? → in-testsuite+
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.