Closed Bug 538530 Opened 14 years ago Closed 10 years ago

a11y too easy to enable (win7 tablet pc components)

Categories

(Tech Evangelism Graveyard :: English US, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: vlad, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [3.6.x])

Attachments

(1 file, 1 obsolete file)

Win7 Tablet PC components trigger a11y enabling, leading to significantly reduced performance esp. in DOM manipulations (see bug 531850).  Given that a lot of Windows 7 machines are shipping with some form of touch input, those users are by default getting a seriously degraded Firefox experience (in a simple innerHTML testcase that I ran into, it goes from negligible milliseconds to 2 slow script dialogs, taking 10+ seconds total!).

At the very least, we need a pref to hard-disable a11y.  That doesn't really solve things for users though, where we really want "a11y is disabled unless you really need it", not "until some random system service pokes us".  Adding a default-off pref probably won't work here, because people who -do- need a11y then wouldn't be able to flip it :/

One solution might be to set a Mozilla registry key for Accessibility, and then ship a small pure-win32 (and therefore accessible) app for toggling it on/off?  Screen readers and other tools can also learn to toggle it on during their own install/enabling as well.

Nominating this for blocking 1.9.2.x (ideally .1), because this really hurts.
Flags: blocking1.9.2?
Note that fundamentally doing N DOM mutations is an O(N^2) or O(N^3) (depending on how lucky or unlucky you get) operation in our current acessibility code due to the event coalescing, with a reasonably high constant.  It's trivial to get tens of thousands of DOM mutation (the page vlad ran into this on just appended to the innerHTML of an element 110-some times to get about 12k DOM mutations), and 12000^3 is a very big number of operations even for a modern processor.  Even 12000^2 operations is something that will take you at least 50ms if each operation is only one processor cycle on a 3GHz processor.  Did I mention that there's a high constant in this case?  :(

How feasible would it be to either drop the coalescing, or make it not be a huge algorithmic slowdown, or reduce the constant significantly (e.g. stop using nsIDOMNode!) or all of the above?  Is it easier to do that than to implement vlad's solution?  (I doubt it, fwiw.)
(In reply to comment #1)

> How feasible would it be to either drop the coalescing,

on the another hand to fire all thousands of the events to AT

> or make it not be a
> huge algorithmic slowdown, or reduce the constant significantly (e.g. stop
> using nsIDOMNode!) 

I think we do and will do but that's not quick process and it won't be accomplished in 1.9.2  timeframe I think.
(In reply to comment #1)
> Is it easier to do that than to
> implement vlad's solution?  (I doubt it, fwiw.)

I think it's worth to grab Marco's opinion (as AT user) on Vlad's suggestion.
This works well for me and fixes the problem, if I set the pref to false -- I'm not sure if this is the best way to disable a11y fully, but I didn't see another obvious spot.
Marking for 1.9.2.x ...
Flags: blocking1.9.2? → blocking1.9.2-
Whiteboard: [3.6.x]
(In reply to comment #4)
> Created an attachment (id=420691) [details]
> add a pref to force-disable a11y
> 
> This works well for me and fixes the problem, if I set the pref to false -- I'm
> not sure if this is the best way to disable a11y fully, but I didn't see
> another obvious spot.

That's correct place I think. If we need to disable a11y on Windows then we shouldn't make a respond on WM_GETOBJECT event. That's what do you do in the end.
What Windows preferences do we use to trigger our a11y support? How are they set?

Perhaps what we should be doing is asking the user for a confirmation that they want to have Firefox interact with the a11y tools.

Also: what are the chances that by disabling a11y, we end up not being able to use the components needed by Tablet PCs?
(In reply to comment #7)
> What Windows preferences do we use to trigger our a11y support? How are they
> set?

A11y support is triggered by an assistive technology like a screen reader, sending a WM_GETOBJECT to the application, in this case, Firefox. We receive that message, and in response, turn on our Microsoft Active Accessibility support (MSAA) plus what else we do like IAccessible2 etc.

So it is not a preference, it is a window message we respond to.

The rule is that *any* software that needs *any* service provided by MSAA will need to send a WM_GETOBJECT message to the application in question, and that application either responds and implements MSAA, or does not respond and therefore does not support it.

> Perhaps what we should be doing is asking the user for a confirmation that they
> want to have Firefox interact with the a11y tools.

The problem is that we cannot turn on MSAA support partially. It is either on or off, and once on, we need to make sure we can respond to everything we support. Moreover, we do not know off-hand whether a certain assistive technology or other tool like the Tablet PC components requires our services.

> Also: what are the chances that by disabling a11y, we end up not being able to
> use the components needed by Tablet PCs?

I believe there will be broken functionality, since Tablet PCs use a combination of MSAA and Text Services to  do their magic.

MSAA was originally implemented for screen readers, but because its services are useful for voice dictation and other technologies, this technology is now being leveraged by a much broader spectrum.
(In reply to comment #4)
> Created an attachment (id=420691) [details]
> add a pref to force-disable a11y
> 
> This works well for me and fixes the problem, if I set the pref to false -- I'm
> not sure if this is the best way to disable a11y fully, but I didn't see
> another obvious spot.

Yes or perhaps in nsAccessibilityService::GetAccessibilityService.

On a related note: I filed bug 527003 for optionally shutting a11y down at runtime.

We probably also need an evangelism bug to find out specifically what API the tablets are using, and maybe provide a more direct API for that. Anyone got a contact?

(In reply to comment #8)
> (In reply to comment #7)
> > Perhaps what we should be doing is asking the user for a confirmation that they
> > want to have Firefox interact with the a11y tools.
> 
> The problem is that we cannot turn on MSAA support partially. It is either on
> or off, and once on, we need to make sure we can respond to everything we
> support. Moreover, we do not know off-hand whether a certain assistive
> technology or other tool like the Tablet PC components requires our services.

Right, but we could always enable accessibility for the confirmation, and then shut it down if the user doesn't want it. (depends on bug 527003).

> > Also: what are the chances that by disabling a11y, we end up not being able to
> > use the components needed by Tablet PCs?
> 
> I believe there will be broken functionality, since Tablet PCs use a
> combination of MSAA and Text Services to  do their magic.
> 
> MSAA was originally implemented for screen readers, but because its services
> are useful for voice dictation and other technologies, this technology is now
> being leveraged by a much broader spectrum.

Longer term, we also need better desktop platform API where we can have some idea what events and properties are really required. It is unfortunate that we have to compute everything all or nothing. I think we should drive this (along with some KISS ideas I have).
Depends on: 527003
(In reply to comment #1)
> How feasible would it be to either drop the coalescing, or make it not be a
> huge algorithmic slowdown, or reduce the constant significantly (e.g. stop
> using nsIDOMNode!) or all of the above?  Is it easier to do that than to
> implement vlad's solution?  (I doubt it, fwiw.)

I think we need to address this performance hit regardless of this bug, but I think we will need a solution like Vlad's for this and related bugs.
What if we take Beltzner's user confirmation idea, and Vlad's patch, and a solution for bug 527003? Would that cover us? Am I missing something?
Attached patch coalesce less frequently (obsolete) — — Splinter Review
What would this break?
Attachment #420735 - Flags: review?(surkov.alexander)
(In reply to comment #12)
> Created an attachment (id=420735) [details]
> coalesce less frequently
> 
> What would this break?

Our tests (for example, recently disabled events_coalesce.html I think).
(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=420735) [details] [details]
> > coalesce less frequently
> > 
> > What would this break?
> 
> Our tests (for example, recently disabled events_coalesce.html I think).

OK I spun off bug 538594 to reduce noise here. I'll tryserver it with that test enabled and report there.
Depends on: a11yperfcoalesce
Attachment #420735 - Flags: review?(surkov.alexander)
Note that such a fix would help, but either way there would still be a significant non-zero performance penalty.  For cases when the user doesn't need a11y, I don't think that's acceptable.  I'll test the tablet pc stuff when I get to the office.
Yes, absolutely. In the long run accessibility suffers when it hurts users that don't need it. We have every reason to fix this in the direction you've taken, striving for a 0 perf hit.

Thanks for testing; I hope a user pref based accessibility.force_disabled doesn't break tablet usage. Note it might break things like Kaspersky (bug 490794) but that isn't necessarily a bad thing... since we are even considering blocklisting that.
If I force disable a11y, I don't get prompted to open up the keyboard when I click in an input field (e.g. urlbar or searchbox).  That seems to be the only difference that I can tell, though I only spent a few minutes playing with it.  That makes sense to me though; the tablet pc stuff is going to want to know about text input fields/areas, and probably not much else.
(In reply to comment #17)
...
> That makes sense to me though; the tablet pc stuff is going to want to know
> about text input fields/areas, and probably not much else.

Yeah this seems to be the case with Kaspersky as well. We should probably provide specific API for this kind of thing.

I wonder if we can detect tablet usage before prompting users to disable a11y.
(In reply to comment #17)
> If I force disable a11y, I don't get prompted to open up the keyboard when I
> click in an input field (e.g. urlbar or searchbox).  That seems to be the only
> difference that I can tell, though I only spent a few minutes playing with it. 
> That makes sense to me though; the tablet pc stuff is going to want to know
> about text input fields/areas, and probably not much else.

The normal way Windows Tablets do this is through TSF. It would be interesting to see if accessibility is turned on in a fallback code path when TSF is not available; does changing intl.enable_tsf_support to true fix it? (This was implemented in a summer of code project in 2008, but is still not on by default: bug 478029.)
Looks like a11y is the main path -- but if I force disable a11y and enable TSF, then I still get the pen input keyboard stuff without the a11y dom slowdown.  However, if I leave a11y enabled, it gets triggered first, and I get the slowdown regardless of whether TSF is enabled or not.
As much as I'm all for less events being fired to ATs, the real question is: does coalescing slow things down more than just firing all of the events? If so, then I'm tempted to say that coalescing should be avoided until its perf can be improved. Can we test the perf in this case with coalescing completely disabled?

As a blind user, having to enable accessibility via some separate application would be very unsatisfactory. For a start, you can no longer just waltz up to another computer, run a portable screen reader and have a11y working. Having a confirmation dialog every time we want to enable accessibility would be even worse and probably enough to make me stop using Firefox. Having a one-time dialog doesn't solve this. Think public access computers. Also, what if I am using a portable screen reader on a friend's computer? All of a sudden, a11y is permanently enabled. The whole point of portable screen readers is to leave no trace.

One of the things that makes Firefox so good for a11y is that users don't have to deal with enabling accessibility or the like; it just *works*. If you take this away, I think this is a huge step backward for Firefox's excellent a11y story.

In addition, this will probably affect applications other than assistive technologies in the future. Obviously, it doesn't seem to affect tablet. However, when it does affect something, the affected user is probably not going to be familiar with the term "accessibility" at all, so they have no chance of understanding the problem. Granted, this might be fixable by making the message for the dialog or enabler very detailed.
Hi Jamie, thanks for your thoughts. Note that Vlad's patch has accessibility.force_disabled as false by default.
(In reply to comment #22)
> Note that Vlad's patch has
> accessibility.force_disabled as false by default.
Granted. However, as Vlad points out, the pref isn't useful for most users. For example, most users of tablet PCs won't know to set this to improve perf.
The other option is to just fix a11y; there's no reason it should be this much of a performance hit at all.  But I think one of the two things must happen for the next release of firefox (well, maybe release after next) -- either a11y gets fixed so that the perf impact is negligible, or we take other action to mitigate the perf imact.  I'm all for Firefox remaining one of the best accessible browsers out there; but I don't think this should come at a massive performance cost.
If we just stop to coalesce events it won't help the perf. I believe some cases will win, some will lose.

Obviously we need get fixed bug 527003 - if no AT any more then no running a11y.

We did few good perf fixes for Firefox 3.7 and we need more. How much time do we have before to "take other action to mitigate the perf impact"?
Accessibility performance should clearly be our first priority.
(In reply to comment #0)
> Win7 Tablet PC components trigger a11y enabling, leading to significantly
> reduced performance esp. in DOM manipulations (see bug 531850).  Given that a
> lot of Windows 7 machines are shipping with some form of touch input, those
> users are by default getting a seriously degraded Firefox experience (in a
> simple innerHTML testcase that I ran into, it goes from negligible milliseconds
> to 2 slow script dialogs, taking 10+ seconds total!).

Vlad do you still have those test cases, or a link to them?
Comment on attachment 420691 [details] [diff] [review]
add a pref to force-disable a11y

Would be helpful to get this in so that we have a reproducible way to test if a problem is caused by a11y or not
Attachment #420691 - Flags: review?(surkov.alexander)
I'm fine with it landing since it defaults to off, and is a good diagnostic aid.
Comment on attachment 420691 [details] [diff] [review]
add a pref to force-disable a11y

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -170,6 +170,8 @@ pref("gfx.color_management.rendering_int
> 
> pref("gfx.downloadable_fonts.enabled", true);
> 
>+pref("accessibility.force_disabled", false);

force_disabled is not good name since it doesn't say it prevents Windows AT to work only. Gecko-based and Linux AT will continue the work in any case.

>+  static int accForceDisable = -1;
>+

I really would like comment for this code block: why we need this and what we do.

>+  if (accForceDisable == -1) {
>+    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);

it's kind unusual when you need to restart the browser to pick up new pref value but it's possibly ok.

r=me
Attachment #420691 - Flags: review?(surkov.alexander) → review+
Checked in with review comments addressed -- I changed the pref to "accessibility.win32.force_disabled".

http://hg.mozilla.org/mozilla-central/rev/5f4acba26bdc

Not sure if we should close this bug out though, as that patch is just a bandaid for the actual problem.
OK I hope we can get rid of that pref one day. Note we have a few bugs filed for performance improvements and I've made this one block our meta/tracker.

I think this should become an evangelism bug (I'm setting this now), while technical solutions can continue on the tracker, and on dependency bug 527003.
Assignee: nobody → english-us
Blocks: a11yperf
Component: Disability Access APIs → English US
Flags: blocking1.9.2-
Product: Core → Tech Evangelism
QA Contact: accessibility-apis → english-us
Target Milestone: --- → Future
Version: Trunk → unspecified
Do we want to back-port this to mozilla-1.9.2?
We could, though it doesn't change any behaviour unless users actually change the pref.  If that's useful I can do the patch.
Pretty sure this should be resolved-fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Tech Evangelism → Tech Evangelism Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: