381888 - Caret tracking broken with Windows screen readers [Cairo-related]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Window-Eyes cannot track the caret in Firefox 3. As you arrow through text (e.g .the location bar), it reads incorrect characters, or none at all. JAWS usually works fine, but I have heard some reports that is having some problems in Firefox 3 text fields.

1.  You can see the problem in any Firefox 3 build (e.g. http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/) 
2.  It broke for the first time in the Firefox 2/23/2006 build. It appears to be the introduction of Cairo. However, I have not been able to find the exact lines of code in Firefox that caused the problem. 
3. Sometimes it seems to report a character that's several character positions away, or the same character several times in a row 

We could ask Window-Eyes and JAWS to update their code to either use caret move events with IAccessibleText, or to adapt to our new caret drawing code, but that would not help users with current versions of those screen readers.

It would be better to try and fix Firefox.

Comment 1

[Aaron Leventhal]

We might be able to fix this by using win32's SetCaretPos -- I was working on a patch for that for bug 244119 anyway (for Windows XP tablet edition handwriting recognition).

Comment 2

[William Smith]

When I was searching for differences between Firefox 2 and Firefox 3, that were relevant to Window-Eyes, this is what I found:

What is different between Firefox 2 and 3  is that in 2 we get repeated  PatBlts as the caret flashes.    In Firefox 3 we only get one of either a stretchblt or bitblt (not sure which) each time the caret moves.  Once the caret is no longer moving, firefox 3 doesn't update the screen in a way that Window-Eyes sees.

Without repetition of the blts, we think the caret has flashed on and then gone away.  

Comment 3

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

We could probably do some dummy PatBlt operations or something to fool screenreaders ... but this is a really crappy way of tracking the caret.

Comment 4

[Aaron Leventhal]

I have a patch that fixes this and bug 244119 that replaces our MSAA caret with an invisible system caret (only for Windows, and only when a11y is active).

>  but this is a really crappy way of tracking the caret.
It's all there used to be, and IAccessible2 is also being implemented for Firefox 3 as a replacement, but only future screen readers will use that. We still want to be backwards-compatible with screen readers that already worked with us. Otherwise the users are forced to upgrade in order to use Firefox 3.

Comment 5

[Aaron Leventhal]

We need to choose betweeen several possibilities here, to maintain backward compatibility:

(I have a patch for #2 that I'm getting ready)

1) On Windows-only, use the system caret. Do not have Gecko render the caret. 
Pros: fixes the problem completely and is probably the right fix (less hacky than other alternatives). When the Gecko caret is turned off and only the system caret is used, everything just works. Also fixes bug 244119 Windows Tablet Edition issues.

Cons: danger of regressions in an otherwise stable area

2) Have an invisible Windows system caret follow the Gecko caret around.
Pros: already have a patch. Works fine with JAWS, works 95% with other ATs, no danger of regressions when accessibility is not active. Patch is ready. Also fixes bug 244119 Windows Tablet Edition issues.

Cons: a bit of a hack. Having the Gecko caret at the same time seems to cause some problems. For example, it does not work perfectly with caret browsing and rich text editing in ZoomText. Occasional beep instead of a read character in Windows-Eyes when there is a 1 pixel-wide letter like i or l followed by a larger letter.

3) Roc's idea "we could probably do some dummy PatBlt operations or something to fool screenreaders ... but this is a really crappy way of tracking the caret."
Pros: possibly the smallest change that will work.
Cons: does not fix Windows Tablet PC Edition issues, no patch yet. We do not know if this will work or what it could break.

Comment 6

[Aaron Leventhal]

The fact that #2 doesn't work 100% perfectly without some tweaks from the ATs is frustrating, but it works well enough so that users can get by. The biggest problems are not deal killers:
1) Rich text editing in Window-Eyes and ZoomText. The ATs need to really support IA2 for that, which will come in later releases of their products.
2) Occasional beeps in Window-Eyes. Hopefully they can help us with this once they have a build with this support.
3) ZoomText saying "blank" for some characters. Hopefully they can help us with this once they have a build with this support.

I'm just a little too afraid of complications with option #1 (replacing Gecko caret on Windows with system caret, and option #3 doesn't fix Windows XP Tablet Edition).

So I'm going to post my patch for option #2 and suggest we go forward with that barring any objections.

Comment 7

[Aaron Leventhal]

Attachment: Solution #2, details below. Fixes most caret tracking a11y regressions from Cairo support. Other will need to come via help from AT vendors (IA2 support, point releases or hints to us)

Changes:
* No need for nsIAccessibleCaret, or for nsCaretAccessible to be an "accessible object", because the MSAA caret is now automatically exposed by Windows itself, based on the system caret. At this point it's just a helper classed owned by the nsRootAccessible to track caret and selection changes. We were also able to get rid of the OBJID_CARET support in widget/src/windows
* Fix role for Midas areas to be ROLE_DOCUMENT, so that JAWS does not continually read "about blank" when the user navigates in it
* Call UpdateSystemCaret() whenever system caret moves. Destroy the caret when it becomes invisible (e.g. user tabs out of a textbox).

Comment 8

[Aaron Smith]

I have to disagree that #2 is the solution. #1 might potentially break something somewhere, whereas #2 definitely breaks AT products, meaning that users of Window-Eyes are going to have less functionality in FF3 than they have with FF2. I think using the system caret is the most logical solution: it works with all AT, and solves an additional bug in FF.

Comment 9

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

If you rely on the system caret for display, and hide the Gecko caret, the caret will not be displayed correctly in many situations. See bug 167801 and all its duplicates, which you will regress. If you limit this to blind users, I guess that's OK.

Comment 10

[Aaron Leventhal]

> If you limit this to blind users, I guess that's OK.
Hmmm ... we can't do this only for totally blind users -- low vision users (ZoomText etc.) need both the caret tracking with text-to-speech as well as correct visual rendering.

> I think using the system caret is the most logical solution: it works
with all AT, and solves an additional bug in FF.
Aaron, #2 does use the system caret and solves the other bug as well. There are some minor things broken with the way Window-Eyes handles it, but I think people can live with it until it's fixed in Window-Eyes. Maybe if we're lucky a build with this solution will lead us toward a fix of the occasional caret/beep problem.

I've actually spent 7 coding days to get this working. I want to get past this and work on future stuff, like ARIA and IAccessible2 support. Really Window-Eyes should just pick up our IAccessible2 support for text fields -- that will allow full support of rich text editing.

Comment 11

[Aaron Leventhal]

On a phone call with GW Micro it was decided the current approach is the right one. They will fix the remaining issues once they can get a test build.

Comment 12

[Aaron Leventhal]

Attachment: Updated to trunk and removed end-of-line 1 px fudge factor for Window-Eyes

Comment 13

[Aaron Leventhal]

Comment on attachment 267291 [details] [diff] [review]
Updated to trunk and removed end-of-line 1 px fudge factor for Window-Eyes

Roc has approved the general approach via email.  So I will start with a module review request.

Comment 14

[Aaron Leventhal]

Attachment: Fix a memory management / shutdown issue for nsCaretAccessible

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 267970 [details] [diff] [review]
Fix a memory management / shutdown issue for nsCaretAccessible

>+nsCaretAccessible::nsCaretAccessible( nsRootAccessible *aRootAccessible):
>+mLastCaretOffset(-1), mLastTextAccessible(nsnull),
>+mCurrentControl(nsnull), mRootAccessible(aRootAccessible)
>+{
>+}

nit: iirc members should be initialized in that order they are declared because it may add compile warnings. Also there is no initialization of mCurrentControlSelection.
 
>+nsCaretAccessible::~nsCaretAccessible()
> {
>+  Shutdown();

You call shutdown here and explicitly in another places. Should you call it explicitly?

>+  caretRect.y      = presContext->AppUnitsToDevPixels(caretRect.y + offsetFromWidget.y);

nit: there is no needed offset before = sign.

> 
>+  /**
>+   * System caret support:
>+   * The system caret works more universally than the MSAA caret
>+   * For example, Window-Eyes, JAWS, ZoomText and Windows Tablet Edition use it
>+   * We will use an invisible system caret.
>+   * Gecko is still responsible for drawing its own caret
>+   */
>+  void UpdateSystemCaret();  // Update Windows caret position.

nit: please merge these comments

Comment 16

[Aaron Leventhal]

Actually for nsCOMPtr<>'s we don't need to initialize them to null in the constructor becauese nsCOMPtr does that for us automatically.

Comment 17

[Aaron Leventhal]

> You call shutdown here and explicitly in another places. Should you call it
> explicitly?

It fixed a crash when I used a Shutdown() method instead of doing things in the destructor. Unfortunately I don't remember the exact steps to reproduce the crash, and can't seem to make it happen.

I suppose it looks a bit redundant to have Shutdown() in both nsRootAccessible() and from the destructor. 

Comment 18

[Aaron Leventhal]

Tomorrow's build will have this.

I noticed some new Firefox 3 problems with JAWS in caret tracking, where the wrong character or "blank" is read for a character. However, it's not at all affected by this patch. It's going to be an issue that we need to ask Freedom Scientific about.

Surkov, I decided to keep the explicit Shutdown() because I know that I added that last week for a consistent crash we were getting, although I can't reproduce it now. I'd rather not spend half a day trying to get rid of an extra Shutdown() command, unless you see a problem it could cause.

Comment 19

[Aaron Leventhal]

Here's the message I sent to the mozilla.dev.accessibility newsgroup:

Firefox 2 worked pretty much perfectly with the caret and Window-Eyes, JAWS and ZoomText, but unfortunately that all changed with Firefox 3, because of the new rendering architecture.

We have new IAccessible2/IAccessibleText interfaces that can resolve it once and for all but that does not address older screen readers that currently work with Firefox.

Today I checked in a patch to fix most of the Firefox 3 problems (the 6/15 build will have the fix), by having an invisible system caret follow the Gecko caret around.

Here's the current status of each AT:

Window-Eyes:
Firefox 2, caret worked completely
Before patch, caret totally broken
After patch, two problems:
1) caret sometimes beeps for a character after a single pixel "i" or "l"
2) caret sometimes repeats last letter on line, make it appear to occur twice (pqr seems like pqrr)

ZoomText:
Firefox 2, caret worked completely
Before patch, caret totally broken
After patch, visual caret tracking completely fixed, but audio caret echo sometimes says "blank" when user presses right arrow at end of line and then arrows backwards

JAWS:
Firefox 2, caret worked completely
Before patch, caret sometimes (not often) says "blank" or the wrong thing on a given character. Eventually the caret corrects itself after several left/right key presses.
After patch, same error -- caret sometimes says "blank" or the wrong thing on a given character. Eventually the caret corrects itself after several left/right key presses.

The remaining problems are small, but important. We'll have to address the last problem with each AT vendor separately. Feel free to help us test and provide succinct, clear information to the vendors.

Ideally the vendors wil start to use IAccessible2, which is much less likely to break than trying to map the visual caret with their offscreen model. In fact, the offscreen model should no longer be necessary for Firefox 3 support.

Comment 20

[Aaron Leventhal]

Attachment: Follow up to simplify GetCaretAccessible()

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268390 [details] [diff] [review]
Follow up to simplify GetCaretAccessible()

You don't need the explicit .get(), do you?

Comment 22

[Aaron Leventhal]

I'll try to remove it.

How about "sr+ but remove the .get() if it's not necessary"

That way I can check it out when it's convenient.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268390 [details] [diff] [review]
Follow up to simplify GetCaretAccessible()

sr=me if you remove the .get().

Comment 24

[Aaron Leventhal]

Checked in the follow-up patch.

Comment 25

[Tim Keenan]

Initial testing on Window-eyes indicates success.

Comment 26

[Tim Keenan]

As Aaron said, periods are missed on a fairly regular basis by Jaws and Window-eyes.  It's a strange problem that I'm sure we'll hear about if we don't fix.  We may have to relnote it for older AT products, if I understood the prior comments correctly.
I'm currently testing to see whether newer AT products use the IAccessible2 interface Aaron mentioned.