353503 - Move ATK-specific event-handling code, and implement focus events on mac

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Aaron Leventhal]

Events are the most important part of getting Firefox to work with a screen reader -- especially focus events.

In nsRootAccessible::HandleEventWithTarget() we map DOM events to MSAA & ATK events. This needs to be changed to handle Mac as well.

Unfortunately there are differences between what events we want to fire, and a 2 huge #ifdef blocks evolved in that method. This was never really intended. So, some refactoring will be required. The common code should stay in nsRootAccessible, but each accessibility toolkit (msaa/atk/mac) should also have an nsRootAccessibleWrap::HandleEventWithTarget() so that platform-specific code can cleanly live there. That method will map the DOM events to nsIAccessibleEvent events.

You'll also have to implement nsDocAccessibleWrap::FireToolkitEvent which handles the actual platform mechanics of firing the events.

As stated before, the focus event is the most important to get right. If you get that working you can file following bugs to get the rest. After the focus event is implemented, Voice Over should speak something as the user tabs around.

Comment 1

[Håkan Waara]

Attachment: WIP v1

This patch moves ATK-only code to src/atk (and lets mac/msaa use the same code), but other than that, there should be no behavioral differences.

Ginn/Evan: Can any of you just test to see if it seems to still work + compile on your side?

Thanks.

Comment 2

[Håkan Waara]

Comment on attachment 239637 [details] [diff] [review]
WIP v1

Actually, just by skimming the ATK-moved code, I notice a bunch of undefined references.

I'll try to catch them all manually, and upload a new patch.

Comment 3

[Håkan Waara]

Attachment: WIP v2

Ok, try this instead. Make sure events (fired from gecko -> atk) still work.

If it doesn't compile, just look at what the remaining parts of nsRootAccessible::HandleEventWithTarget does, that your root accessible wrap doesn't. It should be pretty easy.

Comment 4

[Håkan Waara]

Attachment: Move ATK-only code to nsRootAccessibleWrap (WIP v3)

Found some more minor errors. This should hopefully either compile (and work) or be very close to compiling.

Still need testing/compiling help on Linux. I've looked closely at the code, and there should be no difference in behavior before and after this patch.

Comment 5

[Aaron Leventhal]

So, to start with, Mac events will mirror MSAA events exactly?

Comment 6

[Håkan Waara]

(In reply to comment #5)
> So, to start with, Mac events will mirror MSAA events exactly?

No, but I will do this in steps, to make it reviewable and not regress things wildly.

Comment 7

[Aaron Leventhal]

Comment on attachment 239989 [details] [diff] [review]
Move ATK-only code to nsRootAccessibleWrap (WIP v3)

This has problems in nsRootAccessibleWrap.cpp:
1) It needs #include "nsIDOMXULMultSelectCtrlEl.h"
2) s/targetNode/aTargetNode
3) A lot of code is missing from the start of the method, including defining eventShell, accService, handling events like pagehide, etc.

I stopped looking after that.

Comment 8

[Håkan Waara]

Comment on attachment 239989 [details] [diff] [review]
Move ATK-only code to nsRootAccessibleWrap (WIP v3)

Apart from the (minor) errors you mentioned, the reason the behavior was missing was that I forgot to do:

nsRootAccessible::HandleEventWithTarget(aEvent, aTargetNode);

at the top of the method. That should give the same expected behavior as before.

If you want, I'll make a patch tomorrow with those changes.

Comment 9

[Aaron Leventhal]

I'll look for the new patch tomorrow then.

I don't think your upcalls need to be prefixed with nsRootAccessible:: -- it should just go to the right place.

Comment 10

[Håkan Waara]

Attachment: ATK move, v4

Changes:

* Added the call to the xp-event handling code that I had accidently forgotten
* s/targetNode/aTargetNode
* Added the missing include
* Omitted the IID-removing change in nsRootAccessible.h until we know what it does

Comment 11

[Håkan Waara]

Attachment: Focus patch v1

Here are the mac-specific parts to start listening for, and firing native focus events, when focus changes.

It's not a big patch, but I will summarize each change in detail.

Summary:

* Implement FireToolkitEvent, that tells the focused accesible's native mozAccessible that is has received focus. The mozAccessible will use the OS API NSAccessibilityPostNotification() to notify the OS about it.

* Stop using some constants that are only defined in the OS X 10.4 SDK. I redefine them instead, so we can still support them.

* Stop saying that we support the description attribute (for individual element's description) until we actually do.

* Implement the RoleDescription attribute. This is the "general" description of a role. It is also localized. For example "Button", or "Window". 

* Mark elements as "ignored" if they have the unknown role. This makes the acc hierarchy much cleaner, because we only show elements that we understand (right now). We will make more and more roles not use the AXUnknown role, when we can implement their function.

With these changes, google.com speaks to me when I tab around!

Comment 12

[Aaron Leventhal]

I don't see these things addressed:
> 3) A lot of code is missing from the start of the method, including defining
> eventShell, accService, handling events like pagehide, etc.
eventShell and accService are still undefined :b

And comment 9:
> I don't think your upcalls need to be prefixed with nsRootAccessible:: -- it
> should just go to the right place.

There's an extra else for Linux builds now which makes it break in nsRootAccessible::HandleEventWithTarget()

Also we're now going out and attemping to create accessibles twice.

I'll post a patch with something that compiles in a minute.

Comment 13

[Aaron Leventhal]

Attachment: ATK move, v5. Compiles. Focus events work. No other testing done.

Hakan, go ahead and request r= from Nian Liu if you think it's ready, because the others will not be around much.

> -NS_DEFINE_STATIC_IID_ACCESSOR(nsRootAccessible, NS_ROOTACCESSIBLE_IMPL_CID)
Was that change still supposed to be in the patch? What does it have to do with this bug?

Comment 14

[Håkan Waara]

Comment on attachment 240155 [details] [diff] [review]
ATK move, v5. Compiles. Focus events work. No other testing done.

Yes, please review this Nian.

When you've applied this patch, add back the two lines Aaron mentioned above -- they were only accidently removed.

If possible, please test that events fire just like before. There should be no changes in behavior (the code is just in a different place).

Thanks!

Comment 15

[Nian Liu(n/a in a long time)]

Comment on attachment 240155 [details] [diff] [review]
ATK move, v5. Compiles. Focus events work. No other testing done.

wow, a big move.

seems fine

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 240150 [details] [diff] [review]
Focus patch v1


>+// These are defined first in OS X SDK 10.4+, so we re-define them here, to be able
>+// to respond to them.

Probalby something like "Some constants are defined in OS X SDK 10.4+ only, therefore we define own constants  to make possible using them for previous OS X SDK versions." I'm not big english man, so it's up to you.

>+const NSString *kInstanceDescriptionAttribute = @"AXDescription";     // NSAccessibilityDescriptionAttribute
>+const NSString *kTopLevelUIElementAttribute = @"AXTopLevelUIElement"; // NSAccessibilityTopLevelUIElementAttribute

>+  // is this expensive perf-wise?

What is it? If it's question you don't know answer then I guess you should use XXX

>+                                                           // kInstanceDescriptionAttribute,

Please use here XXX section if description attribute support is missed right now.

>+  /*if ([attribute isEqualToString:kInstanceDescriptionAttribute])
>+    return [self customDescription];*/

The same.

>+// The root accessible will make sure to call this, when it hears
>+// that the focus has changed to us.

Probably "The root accessible should call this when focused node is changed."?

Comment 17

[Håkan Waara]

Clarify bug summary to what actually happened.

Comment 18

[timeless]

hwaara: this broke my tree. it's on MozillaTest. you didn't stick around and fix it.

nian.liu: i'm not sure who you are, but i'm pretty close to demanding that you guys go back to real sr's.

Comment 19

[Håkan Waara]

(In reply to comment #18)
> hwaara: this broke my tree. it's on MozillaTest. you didn't stick around and
> fix it.
> 
> nian.liu: i'm not sure who you are, but i'm pretty close to demanding that you
> guys go back to real sr's.

timeless, I see green trees. It's also hard to help you unless you actually provide the compiler error; what do you really expect us to do?

Comment 20

[Ginn Chen]

Haakan, the tree was recovered.

Comment 21

[timeless]

i expect your reviewers to pay attention to ifdefs. and when you check in, i expect you to pay attention to trees.

it should not be my job to clean up after you (or anyone else).

which isn't to say that i didn't clean up after vlad and you this morning, because i did. i just don't like it.