1504963 - nsIAccessible::accLocation() for OBJID_CARET does not return caret rect of Gecko

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Overlapping caret with layered window application sample (both binary and src)

This bug is reported by a Japanese IME developer, co-san.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=6947

STR:
1. execute sample application in the attachment
2 [review]. click an editor, e.g., <textarea> in bugzilla, or awesomebar.
3. Type some characters.

ER:
Red window which looks like a red caret overlaps caret of Gecko always.

AR:
Red window stays somewhere.

The app does:

void CALLBACK WinEventProc(
	HWINEVENTHOOK hHook,
	DWORD dwEvent,
	HWND hWnd,
	LONG idObject,
	LONG idChild,
	DWORD dwEventThread,
	DWORD dwmsEventTime)
{
	IAccessible* pAcc;
	VARIANT v;
	HRESULT hr;
	RECT rc;

	if (idObject != OBJID_CARET)
		return;

	hr = ::AccessibleObjectFromEvent(hWnd, idObject, idChild, &pAcc, &v);

	hr = pAcc->accLocation(&rc.left, &rc.top, &rc.right, &rc.bottom, v);

	ShowRect(&rc);

	UNREFERENCED_PARAMETER(hHook);
	UNREFERENCED_PARAMETER(dwEventThread);
	UNREFERENCED_PARAMETER(dwmsEventTime);
}

So, just moving the red window over caret rect which is computed by IAccessible::accLocation().

Currently, we don't create Windows native caret in TSF mode unless active IME requires native caret such as old ATOK which is Japanese 3rd party's IME. Even if I use such ATOK, red window is positioned at wrong point.

If we need to create native caret when this kind of a11y tools are active, let me know, I can make TSFTextStore create native caret and keep updating the rect. However, anyway, we need to fix the wrong position issue first.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

aklotz: Do you have some idea?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: Neither Edge nor Chrome works.

Comment 3

[Marco Zehe (:MarcoZ)]

Redirecting the NI to Jamie, as our team is probably more familiar with what should happen and this part of the code than aklotz is. :) masayuki, since this is something visual that neither Jamie nor I can see, would you mind taking a stab at this? You're probably looking at code in accessible/windows/msaa, or the base classes in accessible/generic.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I'm not familiar with a11y APIs and design, and I don't have much time due to working on editor/events a lot... If accessible/ just needs native caret, I could write a patch for TSFTextStore though.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> FYI: Neither Edge nor Chrome works.

Probably, this is my mistake. Chrome looks like works fine.

> Currently, we don't create Windows native caret in TSF mode unless active
> IME requires native caret such as old ATOK which is Japanese 3rd party's IME.

And maybe this too. When I move focus to, for example, the search bar, the red window does not moved immediately. However, after a while, the red caret starts to sync its position with Gecko's caret.

> Even if I use such ATOK, red window is positioned at wrong point.

And this is perhaps bug of the test app, since it's not aware of HiDPI environment.

Comment 6

[James Teh [:Jamie]]

Accessibility creates an invisible native caret (and maintains it) when accessibility is enabled:
https://searchfox.org/mozilla-central/source/accessible/windows/msaa/AccessibleWrap.cpp#1640
We don't support OBJID_CARET directly. However, Windows itself handles OBJID_CARET by querying the native caret.

The question is whether accessibility is enabled in this case. Accessibility only gets enabled if we receive a WM_GETOBJECT with OBJID_CLIENT (e.g. if a client calls AccessibleObjectFromWindow or AccessibleObjectFromEvent with OBJID_CLIENT). If the app doesn't need accessibility except for OBJID_CARET, it probably won't ask for OBJID_CLIENT and thus accessibility won't get enabled. You can check whether accessibility is enabled in about:support.

I have a couple of questions/concerns here:

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #0)
> Currently, we don't create Windows native caret in TSF mode unless active
> IME requires native caret such as old ATOK which is Japanese 3rd party's
> IME.

So that suggests that we have two places where a native caret gets created: one in a11y and one in TSF. That's somewhat worrying. We should probably unify that.

It probably doesn't make sense to enable accessibility if a client only wants the caret. That would suggest TSF might be the right place to unify this. That said, when does TSF mode get enabled? We still need this to work for a11y clients as well, regardless of whether the user is using an IME.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)
> When I move focus to, for example, the search bar, the
> red window does not moved immediately. However, after a while, the red caret
> starts to sync its position with Gecko's caret.

Can you clarify "after a while"? Do you have to wait a few seconds, move the cursor, etc.?

> > Even if I use such ATOK, red window is positioned at wrong point.
> And this is perhaps bug of the test app, since it's not aware of HiDPI
> environment.

Theoretically, oleacc should translate the coordinates to the DPI mode being used by the app, so even an app that isn't DPI aware should behave properly. That said, the coordinates can still be a bit wrong; the translation isn't perfect. I'd suggest having the test app call SetProcessDPIAware() at startup to eliminate this factor. (It's recommended to set awareness in the manifest, but for testing, the SetProcessDPIAware call is fine.)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to James Teh [:Jamie] from comment #6)
> Accessibility creates an invisible native caret (and maintains it) when
> accessibility is enabled:
> https://searchfox.org/mozilla-central/source/accessible/windows/msaa/
> AccessibleWrap.cpp#1640
> We don't support OBJID_CARET directly. However, Windows itself handles
> OBJID_CARET by querying the native caret.
> 
> The question is whether accessibility is enabled in this case. Accessibility
> only gets enabled if we receive a WM_GETOBJECT with OBJID_CLIENT (e.g. if a
> client calls AccessibleObjectFromWindow or AccessibleObjectFromEvent with
> OBJID_CLIENT).

Thank you for the information, this is really helpful information for my other pending jobs (not related to this bug though).

> If the app doesn't need accessibility except for OBJID_CARET,
> it probably won't ask for OBJID_CLIENT and thus accessibility won't get
> enabled. You can check whether accessibility is enabled in about:support.

Thanks. After I reboot my computer and logged into the test machine locally (not with RDP), I see different symptom. The red caret of the test application works only when I type Japanese text with old ATOK which requires native caret (i.e., TSFTextStore creates native caret for it). And currently, about:support says "Activated" false and "Accessible Handler" is true. I guess that RDP or something enabled a11y when I tested yesterday.

> I have a couple of questions/concerns here:
> 
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #0)
> > Currently, we don't create Windows native caret in TSF mode unless active
> > IME requires native caret such as old ATOK which is Japanese 3rd party's
> > IME.
> 
> So that suggests that we have two places where a native caret gets created:
> one in a11y and one in TSF. That's somewhat worrying. We should probably
> unify that.
> 
> It probably doesn't make sense to enable accessibility if a client only
> wants the caret. That would suggest TSF might be the right place to unify
> this. That said, when does TSF mode get enabled? We still need this to work
> for a11y clients as well, regardless of whether the user is using an IME.

Yeah, sounds good. However, basically, TSFTextStore and IMMHandler require native caret creation only for legacy IMEs, and always creates (hidden) native caret at same position/size of Gecko's caret. So, if a11y module always creates caret while a11y is activated, and if other modules can get the state simply, I believe that when a11y module is activated, TSFTextStore and IMMHandler should stop touching native caret. Then, the changing cost must be minimized.

(The main purpose of IMMHandler creating native caret is for IME on Flash Player. So, if a11y module does not care Flash Player, we need to make IMMHandler keep doing it only when plugin has focus, though.)

> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)
> > When I move focus to, for example, the search bar, the
> > red window does not moved immediately. However, after a while, the red caret
> > starts to sync its position with Gecko's caret.
> 
> Can you clarify "after a while"? Do you have to wait a few seconds, move the
> cursor, etc.?

I couldn't make the symptom clearly. Additionally, now, after rebooting my computer, I cannot see the previous symptom.

> > > Even if I use such ATOK, red window is positioned at wrong point.
> > And this is perhaps bug of the test app, since it's not aware of HiDPI
> > environment.
> 
> Theoretically, oleacc should translate the coordinates to the DPI mode being
> used by the app, so even an app that isn't DPI aware should behave properly.
> That said, the coordinates can still be a bit wrong; the translation isn't
> perfect. I'd suggest having the test app call SetProcessDPIAware() at
> startup to eliminate this factor. (It's recommended to set awareness in the
> manifest, but for testing, the SetProcessDPIAware call is fine.)

Thank you for the information. First, I'd ask the test app developer why does the app not calling AccessibleObjectFromWindow  nor AccessibleObjectFromEvent with OBJID_CLIENT.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I discussed this issue with the original reporter.

I still don't know whether Gecko works fine if a11y is activated, though, but he said, Gecko should enable (at least) when WM_GETOBJECT requests OBJID_CARET because Chrome and IE work with that. And also he said, "if it's possible, a11y module should be activated if it's same as or more than 0xFFFFFFF5. However, for example, OBJID_CURSOR is not usually used by MSAA apps, OBJID_SOUND must not be supported by any browsers, so, I'm not sure what's the best". And also he thinks that OBJID_CARET is used a lot by MSAA apps. So, it makes sense that Gecko enables a11y module when receiving WM_GETOBJECT with OBJID_CARET.

What do you think?

Comment 9

[James Teh [:Jamie]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> Thanks. After I reboot my computer and logged into the test machine locally
> (not with RDP), I see different symptom. The red caret of the test
> application works only when I type Japanese text with old ATOK which
> requires native caret (i.e., TSFTextStore creates native caret for it). And
> currently, about:support says "Activated" false and "Accessible Handler" is
> true. I guess that RDP or something enabled a11y when I tested yesterday.

So that still suggests that the caret is broken when accessibility is enabled, which is concerning. The hidden native caret created by accessibility should be in the same location as the Gecko caret.

> TSFTextStore and IMMHandler require
> native caret creation only for legacy IMEs, and always creates (hidden)
> native caret at same position/size of Gecko's caret.

That's exactly what accessibility clients want too: a hidden native caret at the same location as Gecko's caret.

> So, if a11y module
> always creates caret while a11y is activated, and if other modules can get
> the state simply, I believe that when a11y module is activated, TSFTextStore
> and IMMHandler should stop touching native caret.

That's one way of doing it. However, it sounds like the native caret created by TSFTextStore has exactly the same goals. Enabling accessibility just so a client can get the caret is pretty heavy. Is it possible to instead use the TSFTextStore native caret when accessibility is enabled? That way, we'd have only one code path for creation of the native caret. Or is the TSFTextStore native caret limited in some way?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> I still don't know whether Gecko works fine if a11y is activated, though,
> but he said, Gecko should enable (at least) when WM_GETOBJECT requests
> OBJID_CARET because Chrome and IE work with that.

If a client only wants OBJID_CARET, it's clearly not interested in most of what Gecko accessibility has to offer (which is all behind OBJID_CLIENT). Thus, I'm reluctant to enable accessibility just to create a native caret. That's why I'm wondering whether TSFTextStore's native caret implementation is more lightweight. Again, note that OBJID_CARET just uses the native caret, so it should also work with TSFTextStore's caret.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay. I chatted with the original reporter after you commented the last one. Then, he thinks that it's enough to create native caret only while an editor has focus. So, we can use the path for IME without big changes. Taking this bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b1519f7be8bfdf029ddb9db2220f9448805ff23

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e89dbf0d007ae86d75bbf04c156f9df658918ab3

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1504963 - part 1: Make IME modules not touch native caret if a11y module handles native caret

If a11y module is active, it observers caret position and size, and when caret
position or size is changed, it creates/moves native caret to overlap with
our caret.

On the other hand, IME module also creates native caret if active IME requires
it.  Therefore, both of them conflicts each other.

This patch makes IME module stop touching native caret if a11y module is active.

Although, a11y module with Flush Player does not work well for IME.  Therefore,
this patch keeps the conflict between them as-is for now.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1504963 - part 2: Make IMEHandler manage whether native caret is created by it

IMEHandler needs to create native caret later (when there is no composition).
Therefore, IMEHandler should manage whether it creates native caret or not
and IMMHandler and TSFTextStore should create/destroy native caret via
IMEHandler.

Note that this patch makes IMMHandler stops managing whether native caret
is created for plugin or not because native caret is created only one instance
and anyway IME handlers should stop managing native caret when they loses
focus.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1504963 - part 3: Make TSFTextStore create native caret when it gets notified of content change

If WM_GETOBJECT for OBJID_CARET is received but a11y module is not active,
IME module should create native caret over our caret because Windows will
handle the request with native caret automatically and we don't need to
enable a11y module only for it.

This patch makes IMEHandler store whether such message has been received and
makes TSFTextStore create native caret when composition, selection or layout
is changed because especially when there is composition, only TSFTextStore
knows correct position to put caret if there is composition or some dispatched
events have not been handled by content process yet.

Note that IMMHandler already does that since some legacy IMEs require native
caret to show its UI and we cannot check active IME strictly.  Therefore, this
patch does not touch IMMHandler.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1504963 - part 4: Make IMEHandler create native caret over our caret if it's necessary

IMMHandler and TSFTextStore are good class to put native caret when they have
enough information.  However, for example, IMMHandler may not have its singleton
instance until first composition of IMM-IME starts.  Therefore, typically,
IMEHandler is a good class to put native caret without composition.

This patch adds IMEHandler::MaybeCreateNativeCaret(), and if it won't create
native caret because not yet received WM_GETOBJCT for OBJID_CARET, we should
fire window event for MSAA applications.  If there is new MSAA application
retrieves OBJID_CARET, we'll receive WM_GETOBJECT for OBJID_CARET
asynchronously.  Then, we should start to put native caret for such
applications.

Note that if we create native caret, some versions of ATOK refers the native
caret and the behavior becomes worse than usual.  Therefore, we need to
keep not using native caret as far as possible.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I tried to fix the bug of caret position which is set by a11y module. However, I couldn't fix it because of not familiar with e10s of a11y module.

Looks like that it does not occur in main process's content such as search bar, XUL dialog, etc. So, here is correct only when its root caller is in the main process.
https://searchfox.org/mozilla-central/rev/69f9d5002c6e3c5c571a348916fb174e6a7b4acd/accessible/windows/msaa/AccessibleWrap.cpp#1559-1566

However, if it comes from content process, aCaretRect needs to be converted to coordinates in client area of the top level window.

Comment 18

[James Teh [:Jamie]]

If I understand correctly, this new code means the IME native caret gets used if a client requests OBJID_CARET but a11y is not enabled. What I don't understand yet is if the IME native caret code is more limited than the a11y native caret in some way. That is, is there any reason we should keep the a11y native caret code at all? Are there some cases where the IME native caret code won't work where the a11y native caret code does?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to James Teh [:Jamie] from comment #18)
> If I understand correctly, this new code means the IME native caret gets
> used if a client requests OBJID_CARET but a11y is not enabled. What I don't
> understand yet is if the IME native caret code is more limited than the a11y
> native caret in some way. That is, is there any reason we should keep the
> a11y native caret code at all? Are there some cases where the IME native
> caret code won't work where the a11y native caret code does?

Unfortunately, yes, it is. IME code can handle native caret only while an editor has focus because only when an editor gets focus, IMEContentObserver is created in content process and it notifies IME handler of selection changes and caret rect.
https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/events/IMEStateManager.cpp#827,857
Therefore, IME code cannot handle caret if it's available outside of editor, e.g., caret browsing mode. (IIRC, read-only editor like <textarea readonly> creates editor, is supported by IME code.)

Comment 20

[James Teh [:Jamie]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> I tried to fix the bug of caret position which is set by a11y module.
> However, I couldn't fix it because of not familiar with e10s of a11y module.

Can you explain how the caret is incorrect for content?

> However, if it comes from content process, aCaretRect needs to be converted
> to coordinates in client area of the top level window.

If I read this right, aCaretRect (as passed to AccessibleWrap::UpdateSystemCaretFor) is supposed to be in screen coordinates. We then convert it to window (relative) coordinates:

    ::GetWindowRect(aCaretWnd, &windowRect);
    ::SetCaretPos(aCaretRect.X() - windowRect.left, aCaretRect.Y() - windowRect.top);

That would suggest that the caret rect passed from the content process isn't in screen coordinates as it should be. However, both content and parent use the same code to get the caret rect: HyperTextAccessible::GetCaretRect:
https://searchfox.org/mozilla-central/source/accessible/generic/HyperTextAccessible.cpp#1417
So that method must be wrong for content somehow.

I'm pretty vague on Mozilla layout stuff (widgets, etc.), but I'm kinda suspicious of this code:

  nsPoint offset;
  // Offset from widget origin to the frame origin, which includes chrome
  // on the widget.
  *aWidget = frame->GetNearestWidget(offset);
  NS_ENSURE_TRUE(*aWidget, LayoutDeviceIntRect());
  rect.MoveBy(offset);

if I understand correctly, in parent, that's going to refer to the top level HWND, since we only have one HWND. In content, however, I guess that's not going to be the real widget (including the chrome), so that offset is going to be wrong?

Eitan, can you help here? Gecko coordinate stuff confuses the hell out of me...

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to James Teh [:Jamie] from comment #20)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> > I tried to fix the bug of caret position which is set by a11y module.
> > However, I couldn't fix it because of not familiar with e10s of a11y module.
> 
> Can you explain how the caret is incorrect for content?

Its x position is a couple of pixels right, y position is almost same as height of titlebar bottom.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Simplest fix only for content

This patch "fixes" the caret position from content process. However, in searchbar or URL bar, caret position is reported above and left of actual position.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Always use toplevel window

This experimental patch forcibly users top level window as caret owner. However, the symptom is same as the previous patch.

It seems that aCaretRect is not correctly converted to screen coordinates?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to James Teh [:Jamie] from comment #20)
> That would suggest that the caret rect passed from the content process isn't
> in screen coordinates as it should be. However, both content and parent use
> the same code to get the caret rect: HyperTextAccessible::GetCaretRect:
> https://searchfox.org/mozilla-central/source/accessible/generic/
> HyperTextAccessible.cpp#1417
> So that method must be wrong for content somehow.

Well, looks like that it subtracts client rect position from screen coordinates.

ContentEventHandler does more complicated for replying to eQueryCaretRect event.
https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/events/ContentEventHandler.cpp#2410-2411,2417,2419-2422
https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/events/ContentEventHandler.cpp#2875,2879-2880,2884,2889,2894-2895

This is also aware of CSS transform.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I confirmed that a11y module is not aware of CSS transform.
https://jsfiddle.net/d_toybox/wcj7gm20/

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Makoto-san: Could you review the part 1 before vacation? I guess that you failed catching the notification since I requested at updating the patch (I also had not realized a request at same case).

Comment 27

[Makoto Kato [:m_kato]]

Ah ,phabricator UI marks "Waiting on Authors", not "Ready to Review"...  I look this soon.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/beecf3381657
part 1: Make IME modules not touch native caret if a11y module handles native caret r=Jamie,m_kato
https://hg.mozilla.org/integration/autoland/rev/67359f421e75
part 2: Make IMEHandler manage whether native caret is created by it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0c8ede3250a6
part 3: Make TSFTextStore create native caret when it gets notified of content change r=Jamie,m_kato
https://hg.mozilla.org/integration/autoland/rev/e3deacdccfe5
part 4: Make IMEHandler create native caret over our caret if it's necessary r=Jamie,m_kato

Comment 29

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/beecf3381657
https://hg.mozilla.org/mozilla-central/rev/67359f421e75
https://hg.mozilla.org/mozilla-central/rev/0c8ede3250a6
https://hg.mozilla.org/mozilla-central/rev/e3deacdccfe5

Comment 30

[Eitan Isaacson [:eeejay]]

Never got a chance to investigate this, but it looks like it is long resolved.