357583 - xpcom a11y clients cannot activate ally code properly

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

When I switch on 'Show accessible nodes' option then ally code is not initialized like it happens for regular 3d party application that act via MSAA/ATK. It means PresShell::HandleEventInternal doesn't called for NS_ACCESSIBLE_EVENT event. Partially this leads accessible subtree isn't invalidated.

Comment 1

[Aaron Leventhal]

Ah, I understand now.  So, nsIPresShell::IsAccessibilityActive() won't work.

Comment 3

[Aaron Leventhal]

If the accessibilityService is initialized in any way, it needs to tell JS that
it's active.

Also IsAccessibilityActive() should use a static global to track this, so that
different presShells all agree on whether accessibility is active or not.

Comment 4

[Aaron Leventhal]

Attachment: Untested

Comment 5

[Ginn Chen]

On Linux, what's the behavior if desktop is not a11y enabled, but I selected "Show accessible nodes" in DOM inspector?
Does it relate to this bug?

Comment 6

[Aaron Leventhal]

Ginn, that's right. 

On Linux: 
You should be able to show accessible nodes in DOM Inspector or use nsIAccessible in Javascript even if it's not enabled in the desktop

On Windows:
You should be able to show accessible nodes in DOM Inspector or use nsIAccessible in Javascript even if MSAA is not used (and thus no WM_GETOBJECT message is received)

I believe you were able to do that, but layout wasn't notified that accessibility was active, so important layout changes didn't invalidate the a11y cache.

Comment 7

[Aaron Leventhal]

Attachment: Tested -- works. Notice that we only turn it on if a root accessible is created, because root accesible is required for a11y events anyway.

Comment 8

[alexander :surkov (:asurkov)]

It doesn't look API friednly. And it looks the patch doesn't help to DOMi (how can I pass nsIPresShell from js?). Probably ideally ally should be activated when ally service is created (is it possible?) and if ally service has a method to get root accessible.

Comment 9

[Aaron Leventhal]

You don't pass presshell from JS. You just start creating accessibles. If you happen to create a root accessible the a11y service will start generating events as well. If it does start generating events, it will get all of them now.

Comment 10

[Aaron Leventhal]

I did think of having it happen when a11y service is created, but it's really hard to get a presshell from there. I'm not sure how I'd do it, but nsIWindowWatcher might be a possibility. Get the active window and then turn that into a presshell. But, what if there is no active window?

Comment 11

[Aaron Leventhal]

Attachment: More clever -- do it in InitXPAccessibility() which is called once as accessibility service is created

Comment 12

[alexander :surkov (:asurkov)]

You should unitinialize gIsAccessibilityActive in gIsShutdownXPAccessibility too. Not sure I'm fan to have two gIsAccessibilityActive in nsAccessNode and in nsPresShell. Can we have uniquie one?

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 262000 [details] [diff] [review]
More clever -- do it in InitXPAccessibility() which is called once as accessibility service is created

canceling until questions are addressed.

Comment 14

[Ginn Chen]

(In reply to comment #6)
> Ginn, that's right. 
> 
> On Linux: 
> You should be able to show accessible nodes in DOM Inspector or use
> nsIAccessible in Javascript even if it's not enabled in the desktop

I'm afraid we need do some work to make it happen gracefully.
On Gnome 2.18, atk-bridge couldn't be initialized if desktop a11y is off.
It'll cause some warnings at least.

Maybe we don't need load atk-bridge in this case.
Anyway, I'm not going to fix it in this bug.

Comment 15

[Aaron Leventhal]

Attachment: Address surkov's comments -- store a11y active state in one place

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 262124 [details] [diff] [review]
Address surkov's comments -- store a11y active state in one place


> 
>+static PRBool gIsAccessibilityActive;

Should you initialize it?

Comment 17

[Aaron Leventhal]

static variables are automatically initialized to 0, so we don't need to. 

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Will this work in embedding situations?

How about using nsIObserverService to broadcast accessibility activation notifications? Presshells already listen for nsIObserver notifications.

Comment 19

[Aaron Leventhal]

This should work in embedding anyway, but is there another reason you prefer changing it to nsIObserver?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It sounds simpler that this GetOnePresShell thing.

Comment 21

[Aaron Leventhal]

I see -- so in that case I think we should go back to also tracking gIsAccessibilityActive in our own module, rather than having to ask presShell. Otherwise we can't remove that GetAnyPresShell() method.

Comment 22

[Aaron Leventhal]

Attachment: Use nsIObserver instead of inventing GetAnyPresShell()

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    obsService->NotifyObservers(nsnull, "a11y-init-or-shutdown",
+                                NS_REINTERPRET_CAST(const PRUnichar*, &gIsAccessibilityActive));

I think you should pass a real string here. This looks bad (and could screw up potential nsIObservers in script).

Otherwise looks good.

Comment 24

[Aaron Leventhal]

Attachment: Use real string for aData

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

looks like the same patch

Comment 26

[Aaron Leventhal]

That's because I'm an idiot and ran the batch file that checks it in instead of creating a diff.

Comment 27

[Aaron Leventhal]

Attachment: Correct patch

Comment 28

[Boris Zbarsky [:bzbarsky]]

It looks like this dropped Tp and Tp2 on Linux significantly (10% or so).  Was this expected?

Comment 29

[Aaron Leventhal]

I was worried that it might. It could be the nsIObserver usage. Rather than just telling a presShell directly we're broadcasting.

Robert, maybe the GetAnyPresShell() would have fared better on perf?

Comment 30

[Boris Zbarsky [:bzbarsky]]

No, no.  Look at the graph on tinderbox.  The times _dropped_.  As in, with this patch in, we load the pages faster.

Are we running the accessibility code on those tinderboxes?  Or did we use to and stop with this patch?

Comment 31

[Aaron Leventhal]

Oh, now that is odd. I believe we did and still do run a11y code on those tboxes.

I have no clue why this is happening.

Comment 32

[Boris Zbarsky [:bzbarsky]]

When does NotifyA11yInitOrShutdown run?  It sure looks like it would happen when the nsAccessibilityService is created, no?

And if at that point there are no presshells extant (e.g. it's too early during startup), there will be no one to notify, and we'll end up with the presshell's gIsAccessibilityActive being false all the time, right?

I bet that's what's happening.

Comment 33

[Aaron Leventhal]

Yes, that could be it.

Comment 34

[Aaron Leventhal]

Seems like we should reopen this then.

Boris, is there something else in layout which would be a better place to hold this info?

Comment 35

[Aaron Leventhal]

I'm too tired to back this out right now, but if someone else would do it for me I'd appreciate that. Gotta go to bed now.

Comment 36

[Boris Zbarsky [:bzbarsky]]

I'm not really sure, to be honest.  Probably the simplest thing to do is to have another static boolean indicating whether this one was initialized.  Then the presshell ctor can ask the accessibility service for the current value if a presshell is created and we've never done it before...

It still leaves all presshells listening to this notification, but that's survivable, I suppose.

Though to be honest, I don't really understand this patch and tinderbox's reaction to it.  For example, we never call RemoveObserver.  Why aren't we leaking presshells all over?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Note that leaking all presshells would be consistent with the Tp impact, by the way.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Ah, we wouldn't leak through shutdown, because the observer service would drop all refs when xpcom shuts down.  Still not clear why MH isn't showing a problem in that case, though.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why should broadcasting be a performance problem? We're not starting up or shutting down the accessibility service on a regular basis --- if we are, THAT's the performance problem.

Failing to call RemoveObsever --- my bad, that's easy to fix.

If there are no presshells when accessibility starts up --- that's a problem. Calling nsIServiceManager::isServiceInstantiatedByContractID would work, I think.

Comment 40

[Aaron Leventhal]

I think the easiest fix is to:
1) Add RemoveObserver()
2) Add the line back into nsPresShell.cpp which set isAccessibilityActive = PR_TRUE when NS_ACCESSIBLE_EVENT comes through

That way we still catch the a11y activation the way we used to, but also catch the XPCOM initializations.

Comment 41

[Aaron Leventhal]

Attachment: Add RemoveObserver() and restore gIsAccessibilityActive = PR_TRUE in HandleEventInternal()

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263722 [details] [diff] [review]
Add RemoveObserver() and restore gIsAccessibilityActive = PR_TRUE in HandleEventInternal()

Looks ok.