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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [aes+][JAWS])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
We need to call UpdateSystemCaretFor for those events like we do in AccessibleWrap::HandleAccEvent.
(Assignee)

Comment 1

a year ago
Created attachment 8867392 [details] [diff] [review]
Make proxied CARET_MOVE and FOCUS events update the win32 system caret

JAWS works a lot better with this patch. :-)
Attachment #8867392 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 2

a year ago
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.
Attachment #8867392 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Comment 3

a year ago
Created attachment 8867863 [details] [diff] [review]
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.
Attachment #8867392 - Attachment is obsolete: true
Attachment #8867863 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

a year ago
Attachment #8867863 - Flags: review?(tbsaunde+mozbugs) → review?(eitan)
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?
Attachment #8867863 - Flags: review?(eitan) → review+
(Assignee)

Comment 5

a year ago
(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?
Flags: needinfo?(eitan)
(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.
Flags: needinfo?(eitan)
(Assignee)

Comment 7

a year ago
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
https://hg.mozilla.org/mozilla-central/rev/f9b2d4a39bc3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.