Closed Bug 357583 Opened 14 years ago Closed 13 years ago

xpcom a11y clients cannot activate ally code properly

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(3 files, 5 obsolete files)

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.
Ah, I understand now.  So, nsIPresShell::IsAccessibilityActive() won't work.
Blocks: keya11y
Component: DOM Inspector → Disability Access APIs
Keywords: access
Product: Other Applications → Core
Summary: domi doesn't activate ally code properly → xpcom a11y clients cannot activate ally code properly
Version: unspecified → 1.0 Branch
Assignee: dom-inspector → aaronleventhal
QA Contact: timeless → accessibility-apis
Duplicate of this bug: 377288
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.
Blocks: 377783
Version: 1.0 Branch → Trunk
Attached patch UntestedSplinter Review
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?
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.
Attachment #261965 - Flags: superreview?(roc)
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.
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.
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?
Attachment #262000 - Flags: superreview?(roc)
Attachment #262000 - Flags: review?(surkov.alexander)
Attachment #261965 - Attachment is obsolete: true
Attachment #261965 - Flags: superreview?(roc)
Attachment #261965 - Flags: review?(surkov.alexander)
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 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.
Attachment #262000 - Flags: review?(surkov.alexander)
(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.
Attachment #262000 - Attachment is obsolete: true
Attachment #262124 - Flags: review?(surkov.alexander)
Attachment #262000 - Flags: superreview?(roc)
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?
Attachment #262124 - Flags: review?(surkov.alexander) → review+
Attachment #262124 - Flags: superreview?(roc)
static variables are automatically initialized to 0, so we don't need to. 
Will this work in embedding situations?

How about using nsIObserverService to broadcast accessibility activation notifications? Presshells already listen for nsIObserver notifications.
This should work in embedding anyway, but is there another reason you prefer changing it to nsIObserver?
It sounds simpler that this GetOnePresShell thing.
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.
Attachment #262124 - Attachment is obsolete: true
Attachment #263633 - Flags: superreview?(roc)
Attachment #263633 - Flags: review?(surkov.alexander)
Attachment #262124 - Flags: superreview?(roc)
+    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.
Attached patch Use real string for aData (obsolete) — Splinter Review
Attachment #263633 - Attachment is obsolete: true
Attachment #263645 - Flags: superreview?(roc)
Attachment #263645 - Flags: review?(roc)
Attachment #263633 - Flags: superreview?(roc)
Attachment #263633 - Flags: review?(surkov.alexander)
That's because I'm an idiot and ran the batch file that checks it in instead of creating a diff.
Attached patch Correct patchSplinter Review
Attachment #263645 - Attachment is obsolete: true
Attachment #263647 - Flags: superreview?(roc)
Attachment #263647 - Flags: review?(roc)
Attachment #263645 - Flags: superreview?(roc)
Attachment #263645 - Flags: review?(roc)
Attachment #263647 - Flags: superreview?(roc)
Attachment #263647 - Flags: superreview+
Attachment #263647 - Flags: review?(roc)
Attachment #263647 - Flags: review+
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
It looks like this dropped Tp and Tp2 on Linux significantly (10% or so).  Was this expected?
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?
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?
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.
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.
Yes, that could be it.
Seems like we should reopen this then.

Boris, is there something else in layout which would be a better place to hold this info?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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?
Note that leaking all presshells would be consistent with the Tp impact, by the way.
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.
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.
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 on attachment 263722 [details] [diff] [review]
Add RemoveObserver() and restore gIsAccessibilityActive = PR_TRUE in HandleEventInternal()

Looks ok.
Attachment #263722 - Flags: superreview?(bzbarsky)
Attachment #263722 - Flags: superreview+
Attachment #263722 - Flags: review?(bzbarsky)
Attachment #263722 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.