65158 - Need to attach accessibility event handler to all windows in one go

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Aaron Leventhal]

For accessibility purposes, we need to be able to monitor events that happen in
any content *or* chrome window - keyboard events, focus events, load events etc.

I have done this by adding a function in dom/src/base/nsGlobalWindow.cpp called
AttachAccessibleEventHandlers, which looks for a service called
@mozilla.org/accessproxy;1 with the interface nsIAccessProxy and attaches event
handlers to it for keypress, keyup, click, load and focus events.

Comment 1

[Aaron Leventhal]

Further explanation: this bug allows one to add text-to-speech capability
directly to the Mozilla browser. Eventually we will add a pref so that the user
can use Mozilla with whatever text-to-speech engine they have on their system.
This is intended to be used with the accessibility.browsewithcaret preference
(see bug 49508)

Comment 2

[Aaron Leventhal]

Attachment: Patch to nsGlobalWindow.cpp to attach accessibility handler

Comment 3

[Aaron Leventhal]

This won't affect performance - the accessibility handler is only attached if
the pref accessibility.usetexttospeech or accessibility.usebrailledisplay are
set to a a non-null string. Eventually those string's will correspond to the
name of an engine or speech component.

Comment 4

[Aaron Leventhal]

Attachment: Proper -u patch (oops) for attach accessible handler bug

Comment 5

[Aaron Leventhal]

Attachment: Got rid of heinous objectionable unneccessary NS_RELEASE

Comment 6

[Aaron Leventhal]

Attachment: Whitespace fixes & a comment added

Comment 7

[Aaron Leventhal]

Attachment: Fixed leaks by using nsXPIDLCString and getter_Copies(blah) with CopyCharPref

Comment 8

[Aaron Leventhal]

Attachment: Fixed more leaks and null pointer check. nsXPIDLCstring is your friend.

Comment 9

[Aaron Leventhal]

Attachment: implements nsIAccessProxy called for each global event, please create dir for it in modules/accessibility/accessproxy

Comment 10

[Aaron Leventhal]

Eventually I need to replace
"@mozilla.org/accessproxy;1" with NS_ACCESSPROXY_CONTRACTID
once everything's in place.
However, I'd like to get these changes checked in first, but I promise to change
it once we have these checkins finalized.

Comment 11

[Aaron Leventhal]

Attachment: Diff's for everything needed, including new directory with accessproxy

Comment 12

[Aaron Leventhal]

Waiting for jst to ask Vidur about this patch.
We might want to make the component that is used for the handler a pref, so
instead of hardcoding "@mozilla.org/accessproxy;1" we could have
the pref accessibility.globlalhandler="@mozilla.org/accessproxy;1"
jst is the reviewer for this, vidur is probably the super-reviewer.
I'll need 3 reviewers and their signatures to get CVS access - this will be my
first checkin.

Comment 13

[Aaron Leventhal]

Attachment: Don't need to hook nsGlobalWindow.cpp anymore! Inits as nsIWebProgressListener

Comment 14

[Aaron Leventhal]

Jag, got rid of the kludginess you didn't like, via Vidur & Jst's help.
Jst, ready for r= ?

Comment 15

[Johnny Stenback (:jst)]

I had a look at the last diff in this bug and here's a few things that should be
fixed before this is checked in:

- Since the code is MPL'ed the comment header in nsAccessProxy.cpp,
nsAccessProxy.h, nsIAccessProxy.idl should be the standard MPL comment (the one
you have in there has either #'es on every line, or missing *'s on every line),
grab a copy of the comment block from an existing source file in mozilla.

- nsAccessProxy::GetInstance() is never used, could it be removed or was it left
in for futher callers?

- nsAccessProxy::mInstance should be private, right?

- All those printf's in the nsAccessProxy::OnXXX methods could generate quite
alot of noice, could you wrap them in #ifdef NS_DEBUG?

- in nsIAccessProxy.idl, could all the nsIDOMEvent stuff you have in there be
replaced with one simple "interface nsIDOMEvent;" forward declaration?

- Any good reason for nsIAccessProxy::handleEvent() being [noscript]?

Also, please fix the indentation in the .cpp file, bee consistent, I see 1, 2
and 4 spaces used for indentation, stay with 2 like most of the mozilla code
does, or if you disagree with that atleast make it consistent, that also goes
for indentation of method argument lists that are broken up on multiple lines.

I assume there's more to this than this one patch, right? Will the rest of this
(i.e. the implementation of the event listener) be checked in in a different
checkin?

Please correct those issues and attach a new diff and I'll have one more look.

Comment 16

[Johnny Stenback (:jst)]

Oh, and in nsAccessProxy::Initialize() you don't need the extra docLoader
pointer, just do progress(do_GetServ...) and you needs a null check before
referencing 'progress'.

Comment 17

[Aaron Leventhal]

Attachment: Johnny's suggestions implemented

Comment 18

[Johnny Stenback (:jst)]

I had a look at the last diff and there's still some license comment block
weidness, just formatting differences between the ones used here and other
license comment blocks in the rest of the code in mozilla, so please fix that
when checking in, simply grab a copy of a license comment block, and don't edit
it, leave it as is so that these files don't cause grief when doing automatic
updates of the licenses in the source repository. And now the code is dual
licensed, MPL and GPL, I'm assuming that's intentional.

I noticed that the diffs contain the actual unix Makefile's, make sure you don't
check those in, just check in Makefile.in's.

Also, as part of the lisence comment there's an emacs more line, it should be:

/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

to match the indentation used in the files (the modelines in the patch say 4 in
stead of 2).

In nsAccessProxy.cpp you probably don't need the #include's "plstr.h", "stdio.h"
(which should be <stdio.h> if it's needed) and "prprf.h".

In nsAccessProxy::OnStateChange() you don't need the rv variable, simply
checking if you got a non-null window pointer is enough since you don't return
the falure code or anything there, and the ending square brackets are not
aligned with the opening ones.

Ok, I'm done here, I'd say that with those changes the code looks nice and
clean, r=jst.

Comment 19

[Aaron Leventhal]

Okay,
I've made jst's changes - the license is fixed, indent comment is fixed for
emacs, OnStateChange is fixed, the extra #include's are removed, and I won't
check in the Makefile 's, just Makefile.in
Vidur, can you sr= ?
Aaron

Comment 20

[vidur (gone)]

Aaron, I'm not sure what the current conventional wisdom is for where to put
contract ids, but I'd recommend putting the AccessProxy one in
nsIAccessProxy.idl, within a %{ C++ %} block. Other than that, sr=vidur.

Comment 21

[Aaron Leventhal]

Vidur, done - I need 2 other superreviews so I can eventually get CVS access

Comment 22

[Aaron Leventhal]

Part 1 checked in.
Next we'll ctually use the data we're getting from these events.

Comment 23

[Aaron Leventhal]

This was checked in a while ago, marking fixed