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

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: aaronlev, Assigned: hwaara)

Tracking

(Blocks: 1 bug, {access})

Trunk
x86
Mac OS X
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Version: 2.0 Branch → Trunk
(Assignee)

Updated

12 years ago
Blocks: 342989
(Assignee)

Comment 1

12 years ago
Created attachment 239637 [details] [diff] [review]
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.
(Assignee)

Comment 2

12 years ago
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.
Attachment #239637 - Attachment is obsolete: true
(Assignee)

Comment 3

12 years ago
Created attachment 239648 [details] [diff] [review]
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.
(Assignee)

Updated

12 years ago
Blocks: 353816
(Assignee)

Comment 4

12 years ago
Created attachment 239989 [details] [diff] [review]
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.
Attachment #239648 - Attachment is obsolete: true
Attachment #239989 - Flags: review?(ginn.chen)
(Reporter)

Comment 5

12 years ago
So, to start with, Mac events will mirror MSAA events exactly?
(Assignee)

Comment 6

12 years ago
(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.
(Reporter)

Comment 7

12 years ago
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.
Attachment #239989 - Flags: review?(ginn.chen) → review-
(Assignee)

Comment 8

12 years ago
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.
(Reporter)

Comment 9

12 years ago
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.
(Assignee)

Comment 10

12 years ago
Created attachment 240114 [details] [diff] [review]
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
Attachment #239989 - Attachment is obsolete: true
Attachment #240114 - Flags: review?(aaronleventhal)
(Assignee)

Comment 11

12 years ago
Created attachment 240150 [details] [diff] [review]
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!
Attachment #240150 - Flags: review?(surkov.alexander)
(Reporter)

Comment 12

12 years ago
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.
(Reporter)

Comment 13

12 years ago
Created attachment 240155 [details] [diff] [review]
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?
Attachment #240114 - Attachment is obsolete: true
Attachment #240114 - Flags: review?(aaronleventhal)
(Assignee)

Comment 14

12 years ago
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!
Attachment #240155 - Flags: review?(nian.liu)
Comment on attachment 240155 [details] [diff] [review]
ATK move, v5. Compiles. Focus events work. No other testing done.

wow, a big move.

seems fine
Attachment #240155 - Flags: review?(nian.liu) → review+
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."?
Attachment #240150 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

12 years ago
Clarify bug summary to what actually happened.
Summary: Map nsIAccessibleEvent events to Universal Access events → Move ATK-specific event-handling code, and implement focus events on mac
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 18

12 years ago
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.
(Assignee)

Comment 19

12 years ago
(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

12 years ago
Haakan, the tree was recovered.

Comment 21

12 years ago
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.
You need to log in before you can comment on or make changes to this bug.