448660 - Turn off new visual Ctrl+tab switching when screen reader present

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Aaron Leventhal]

The new feature specifically interferes with JAWS, which is an expensive screen reader to upgrade. As things stand now, we'd need to convince JAWS to change their code in time for the next release, users would need to find out about the change and then users would have to get a paid upgrade. Marco & I think this feature should turned off when the screen reader flag is present. In general it's not a useful feature for blind users. 

You have to use eMetric_IsScreenReaderActive from C++ to turn off the pref that controls whether this feature is used. This can be done in some init phase of Firefox. I suggest asking Neil Rashbrook (CC'd) or one of the platform guys where we should patch.

As an example, we do something similar here with theme switching:
http://mxr.mozilla.org/seamonkey/source/chrome/src/nsChromeRegistry.cpp#592

Comment 1

[Dão Gottwald [:dao]]

(In reply to comment #0)
> In general it's not a useful feature for blind users.

I disagree with this, which I guess I didn't make clear when we met up.
Apart from the problem with JAWS, I don't see why navigating between a bunch of most recently used tabs would be harder for blind users. In fact, not seeing the tab bar might make switching in tab bar order less useful for blind users than it is for sighted ones.

Comment 2

[Dão Gottwald [:dao]]

The pref's name is browser.ctrlTab.mostRecentlyUsed, btw.

Comment 3

[James Teh [:Jamie]]

Personally, I don't mind whether I use this new feature or not. However, while I understand the concern here with regard to JAWS, it seems somewhat crippling to insist that any Windows screen reader cannot use this functionality, even if it does work fine with some screen readers. (Aside from this specific case, it sets a bad precedent.) Also, this does not fix the situation if a user launches JAWS *after* Firefox has been loaded, which, while a somewhat less common use case, is certainly plausible.

Comment 4

[Aaron Leventhal]

Jamie, in general I agree but this is a rare case. We won't do this often, but sometimes we have to just take a practical approach. It's a big deal for us to confuse our current set of JAWS users and make them pay money for something that's not even really helpful. Also, there will be other new features coming into Firefox for navigating to tabs that will serve screen reader users better. For example, they are going to allow a user to type a substring in a quick search and navigate through a list of matching open tabs.

The launch order is a good point. We can deal with that by putting the check here:
http://mxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsAccessNodeWrap.cpp#580
That also makes sure the code doesn't bloat the rest of Gecko. It's cleanly put in the a11y module.

If you really think this feature is important for NVDA users, catch me offline and we can discuss how to make that happen.

Comment 5

[Aaron Leventhal]

Dao, as I explained to Jamie, this is really a practical issue for us. You need to understand what we've had to go through to get *any* screen reader users into the world of Firefox. Things either need to work right out of the box for the current tools or we are creating a problem. They won't know they need to upgrade their screen reader, and they will assume it's a Firefox bug. Many people won't want to pay hundreds of dollars just for this, even if they do find out they need to upgrade. Think about the cost/benefit here. Sorry, it's pretty dead obvious to me.

Comment 6

[Dão Gottwald [:dao]]

I understand this, I just want to make sure that this bug is only about the fact that JAWS is unable to deal with the Ctrl+Tab panel rather than the feature in itself being useless to blind users.

Comment 7

[James Teh [:Jamie]]

Do other ATs which set the screen reader flag have problems with this functionality? If we disable it based on the screen reader flag, this will affect all other ATs. Does ZoomText set the screen reader flag? If it does, this will affect ZoomText users as well. Perhaps other ATs need to be tested with this functionality once bug 447580 is fixed.

From a principle standpoint, if this issue is specific to JAWS, then perhaps the check needs to be made specific to JAWS (perhaps check for jfw.exe). I realise this is being pedantic, but this bug is essentially suggesting a hack to fix an issue for a particular screen reader, so it could be argued that the hack should be made specific to that screen reader.

In addition, users of commercial screen readers need to take some responsibility. I strongly desire that application developers provide the best accessibility possible, but in this case, the bug is clearly and entirely on the AT side. There is even a work around: provide instructions for the user to set the pref. Should we not encourage users to be aware that this is an AT issue and request their AT vendor to fix such issues, rather than hoping that application developers will always transparently work around them? This really does set a bad precedent.

For me, this isn't really about the practical outcome as to whether I can use this particular feature or not; I have no idea whether it will be useful. Yes, I'm being overly pedantic. However, these principle objections do need to be raised.

Comment 8

[Aaron Leventhal]

Jamie, I would prefer we talk about it offline when Marco and I get back. Bugzilla really isn't the best place for controversial discussions. FYI, Marco says that Window-Eyes has the same problem.

In general Firefox adheres to the principle of access to the same UI for everyone. We have no place where we've had to do the screen reader check before. However, there is a time and place for everything. We have many JAWS and Window-Eyes users who are not very technical and don't have a lot of money. Many of these users have enough problems just coping with the basics and we can't afford to scare them off.

I would not worry about precedence. This is obviously a last resort. We have to do it because it causes a regression related to the keyboard and there is no way around it.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Does that only happen on Windows or are also other OS affected?

Comment 10

[timeless]

there is a screen reader for Linux, it'd be interesting to know how well it handles this (and similarly how well NVDA handles it on windows).

However, as for precedent, we've added jaws7.1 specific code to prevent Firefox from crashing when it encounters this. And I support such code.

fwiw, the first thing I thought when i saw aaronl's suggestion of a startup thing was exactly that it's not "correct".

i'll offer an alternative:
use a pref which is not in all.js, and check for its existence at startup of accessible. if it isn't set, set it to disabled. in tab startup, read that pref to determine whether ctrl-tab activates the visual.

if a user has a screen reader which handles ctrl-tab visuals well, then let the user switch the pref to enabled. it'll appear in about:config for any such user.

Comment 11

[James Teh [:Jamie]]

(In reply to comment #10)
> it'd be interesting to know how well ... NVDA handles it on windows
Currently, it only partially works, though it should work well once bug 447580 is fixed.

> However, as for precedent, we've added jaws7.1 specific code to prevent Firefox
> from crashing when it encounters this. And I support such code.
See my argument above; this is not so bad. My argument was against using the screen reader flag for this purpose.

Comment 12

[Aaron Leventhal]

Yes, now that we know how to test for specific screen reader versions, let's just disable this feature for JAWS and Window-Eyes, by default. The solution should possibly allow the user to override.

I think we should have a bug for an alternative UI for navigating through tabs that simply allows arrowing through the list of tabs sorted how the user wants -- by when it was visited or grouped by parent tab.

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Patch

1. Detect if either JAWS's jhook or Window-Eyes's gwm32inc modules are injected.
2. If so, check if the pref "browser.ctrlTab.disallowForScreenReaders" exists already.
3. If so, simply do nothing, respect whatever setting is there.
4. if not, set its value to TRUE and set it.
5. In the browser chrome, before initializing tabpreview and tabs, check to see if the pref is there or is not set to TRUE.

Tests performed:
1. if JAWS or Window-Eyes is running, and I start Firefox with this patch, the pref is set if it doesn't exist, and CTRL+Tab now behaves like it does in Firefox 3.0.
2. If NVDA is running, nothing is done.
3. If the pref is already there, whatever setting it has is repsected regardless of the screen reader. So even someone with JAWS can turn it on (for example to test a bug fix on the JAWS end for this), or someone with NVDA can turn it off.

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 335047 [details] [diff] [review]
Patch

Requesting review from Mano for the browser chrome part.

Comment 15

[Aaron Leventhal]

Comment on attachment 335047 [details] [diff] [review]
Patch

I'm okay with the general idea but have someone sr= it. I'm not sure if we want a #define or to check for the jhook process twice as we're doing.

Comment 16

[Aaron Leventhal]

If someone changes the pref in either direction during the running of Firefox can that cause init or uninit problems?

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 335047 [details] [diff] [review]
Patch

>+  if (!gPrefService.getBoolPref("browser.ctrlTab.disallowForScreenReaders")) {
>+    tabPreviews.init();
>+    if (gPrefService.getBoolPref("browser.ctrlTab.mostRecentlyUsed"))
>+      ctrlTab.init();
>+  }

tabPreviews.init(); will still be needed.

> function BrowserShutdown()
> {
>-  tabPreviews.uninit();
>-  ctrlTab.uninit();
>+  if (!gPrefService.getBoolPref("browser.ctrlTab.disallowForScreenReaders")) {
>+    tabPreviews.uninit();
>+    ctrlTab.uninit();
>+  }

You can call uninit regardless.

Comment 18

[[e-mail]]

Comment on attachment 335047 [details] [diff] [review]
Patch

>+    PRBool disallowCtrlTabPreviewForScreenReaders = PR_FALSE;
If this variable wasn't already unreasonably long enough I'd suggest you call it hasDisallow...ForScreenReadersPref. (Although the pref isn't obviously specific to screen readers.)

>+    static void TurnOffNewTabSwitchingForJawsAndWe();
We means Window-Eyes, right? (The abbreviation looks odd.)

>+  if (!gPrefService.getBoolPref("browser.ctrlTab.disallowForScreenReaders")) {
This will fail if the preference isn't set, which is the case if you don't have a build with the Windows accessibility code. Fortunately this is browser code so you'll have to fix it to get your browser r+ anyway, so I'm still marking sr+.

> function BrowserShutdown()
> {
>-  tabPreviews.uninit();
>-  ctrlTab.uninit();
>+  if (!gPrefService.getBoolPref("browser.ctrlTab.disallowForScreenReaders")) {
>+    tabPreviews.uninit();
>+    ctrlTab.uninit();
>+  }
I'm not a browser peer but this change looks unnecessary.

Comment 19

[Marco Zehe (:MarcoZ)]

(In reply to comment #18)
> >+    PRBool disallowCtrlTabPreviewForScreenReaders = PR_FALSE;
> If this variable wasn't already unreasonably long enough I'd suggest you call
> it hasDisallow...ForScreenReadersPref. (Although the pref isn't obviously
> specific to screen readers.)

:-) I can certainly shorten it a bit since it's only being used locally anyway.

> >+    static void TurnOffNewTabSwitchingForJawsAndWe();
> We means Window-Eyes, right? (The abbreviation looks odd.)

Well, strictly spoken it's "WE", so I could change it to TurnOffNewTabSwitchingForJawsAndWE();
Does this look better?
> 
> >+  if (!gPrefService.getBoolPref("browser.ctrlTab.disallowForScreenReaders")) {
> This will fail if the preference isn't set, which is the case if you don't have
> a build with the Windows accessibility code. Fortunately this is browser code
> so you'll have to fix it to get your browser r+ anyway, so I'm still marking
> sr+.

Ah OK, I'll check if the pref exists in addition.

Thanks!

Comment 20

[[e-mail]]

(In reply to comment #19)
> (In reply to comment #18)
> > >+    static void TurnOffNewTabSwitchingForJawsAndWe();
> > We means Window-Eyes, right? (The abbreviation looks odd.)
> Well, strictly spoken it's "WE", so I could change it to
> TurnOffNewTabSwitchingForJawsAndWE();
> Does this look better?
Yes, definitely.

Comment 21

[Marco Zehe (:MarcoZ)]

Attachment: Address Neil's comments and change logic in browser.js.

In browser.js, now also check whether the pref exists at all. Put both nested ifs into one big statement. Also remove ifs from uninit, as mentioned by both Neil and Dao.

Comment 22

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/b9fefe5ed9e2

Comment 23

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080825031951 Minefield/3.1a2pre

Comment 24

[Marcia Knous [:marcia]]

Marco: Can you create a Litmus test case for this?