1364544 - Proxied CARET_MOVED and FOCUS events need to update the system caret before firing their WinEvent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[(No longer employed by Mozilla) Aaron Klotz]

We need to call UpdateSystemCaretFor for those events like we do in AccessibleWrap::HandleAccEvent.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Make proxied CARET_MOVE and FOCUS events update the win32 system caret

JAWS works a lot better with this patch. :-)

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8867392 [details] [diff] [review]
Make proxied CARET_MOVE and FOCUS events update the win32 system caret

Grr.. I have to cancel this atm... I need to fix how this integrates with window emulation.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Make proxied CARET_MOVE and FOCUS events update the win32 system caret (r2)

Based on my testing with non-e10s, caret events are hooked up to the main Firefox HWND, not emulated HWNDs. This revision of the patch accounts for that.

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8867863 [details] [diff] [review]
Make proxied CARET_MOVE and FOCUS events update the win32 system caret (r2)

Review of attachment 8867863 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/ipc/DocAccessibleParent.cpp
@@ +272,5 @@
>  
>  mozilla::ipc::IPCResult
> +DocAccessibleParent::RecvCaretMoveEvent(const uint64_t& aID,
> +#if defined(XP_WIN)
> +                                        const LayoutDeviceIntRect& aCaretRect,

Don't love seeing a signature touched with a preprocessor, but its probably not worth adding a whole new function to the windows protocol.

::: accessible/ipc/win/DocAccessibleChild.cpp
@@ +164,5 @@
> +{
> +  Accessible* target;
> +
> +  if (aID) {
> +    target = reinterpret_cast<Accessible*>(aID);

I know that this is how we do it, but casting integers to pointers seems scary! Oh well..

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +1564,5 @@
> +    return;
> +  }
> +
> +  HWND caretWnd = reinterpret_cast<HWND>(widget->GetNativeData(NS_NATIVE_WINDOW));
> +  if (!caretWnd) {

You check for this in UpdateSystemCaretFor(HWND, LayoutDeviceIntRect), you can just pass caretWnd along. no?

@@ +1595,5 @@
>    // Create invisible bitmap for caret, otherwise its appearance interferes
>    // with Gecko caret
> +  nsAutoBitmap caretBitMap(CreateBitmap(1, aCaretRect.height, 1, 1, nullptr));
> +  if (::CreateCaret(aCaretWnd, caretBitMap, 1, aCaretRect.height)) {  // Also destroys the last caret
> +    ::ShowCaret(aCaretWnd);

Why not put ::DestroyCaret() just above this line and remove it from the other overloaded functions?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> Comment on attachment 8867863 [details] [diff] [review]
> Make proxied CARET_MOVE and FOCUS events update the win32 system caret (r2)
> 
> Review of attachment 8867863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ +1564,5 @@
> > +    return;
> > +  }
> > +
> > +  HWND caretWnd = reinterpret_cast<HWND>(widget->GetNativeData(NS_NATIVE_WINDOW));
> > +  if (!caretWnd) {
> 
> You check for this in UpdateSystemCaretFor(HWND, LayoutDeviceIntRect), you
> can just pass caretWnd along. no?

True. I'll fix that.

> 
> @@ +1595,5 @@
> >    // Create invisible bitmap for caret, otherwise its appearance interferes
> >    // with Gecko caret
> > +  nsAutoBitmap caretBitMap(CreateBitmap(1, aCaretRect.height, 1, 1, nullptr));
> > +  if (::CreateCaret(aCaretWnd, caretBitMap, 1, aCaretRect.height)) {  // Also destroys the last caret
> > +    ::ShowCaret(aCaretWnd);
> 
> Why not put ::DestroyCaret() just above this line and remove it from the
> other overloaded functions?

Theoretically I could do one better and just remove the DestroyCaret call completely, since CreateCaret implicitly destroys the previous caret. BUT: My concern is about compat breakage. In the current implementation of UpdateSystemCaretFor, we unconditionally call DestroyCaret (which implicitly generates a system a11y event) before potentially returning due to failure of subsequent tests.

My concern is that, by either moving the DestroyCaret call after the sanity checks, or by eliminating DestroyCaret completely, I might break the generation of caret move a11y events by the system in a way that breaks compat. Thoughts?

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now) from comment #5)
> Theoretically I could do one better and just remove the DestroyCaret call
> completely, since CreateCaret implicitly destroys the previous caret. BUT:
> My concern is about compat breakage. In the current implementation of
> UpdateSystemCaretFor, we unconditionally call DestroyCaret (which implicitly
> generates a system a11y event) before potentially returning due to failure
> of subsequent tests.
> 
> My concern is that, by either moving the DestroyCaret call after the sanity
> checks, or by eliminating DestroyCaret completely, I might break the
> generation of caret move a11y events by the system in a way that breaks
> compat. Thoughts?

Yes, don't do it! Let's keep the API identical as possible to what ATs expect. If that call is actually expensive for each caret move, maybe a spinoff bug would be worthwhile to revisit later.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b2d4a39bc32715a60defbfe57c68e1620431ac
Bug 1364544: Ensure that proxied CARET_MOVED and FOCUS events update the Win32 system caret before firing their WinEvents; r=eeejay

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f9b2d4a39bc3