Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

      No description provided.

	Trevor Saunders (:tbsaunde)
	Comment 2 • 9 years ago

Comment on attachment 8612994 [details] [diff] [review]
ProxyEvent, ProxyCaretMoveEvent inform mozAccessible

>+#if defined(__OBJC__)
>+  nsresult HandleProxyOrAccEvent(mozAccessible* aNativeAcc, uint32_t aEventType);

mostly just commenting to silence bugzilla, but when you get a chance lets make this return null as we talked about last week.

	Trevor Saunders (:tbsaunde)
	Comment 5 • 9 years ago

Comment on attachment 8614209 [details] [diff] [review]
ProxyEvent, ProxyCaretMoveEvent inform mozAccessible

># HG changeset patch
># User Lorien Hu <lorien@lorienhu.com>
># Date 1432921826 14400
>#      Fri May 29 13:50:26 2015 -0400
># Node ID e7f9095a1cf055dfef88b44e1daa9afe85e2f135
># Parent  9eae3880b132898a96a80d497cba2b9523e049a4
>Bug 1169701 - ProxyEvent, ProxyCaretMoveEvent inform mozAccessible

I'd say a better commit message is something like

bug 1169701 - fire native OSX accessibility events for proxied accessibles

>+#if defined(__OBJC__)
>+  void HandleProxyOrAccEvent(mozAccessible* aNativeAcc, uint32_t aEventType);

hrm, I should have caught this earlier, but that isn't my favorite name, I'd probably choose FireNativeEvent maybe?

>   mozAccessible *nativeAcc = nil;
>   accessible->GetNativeInterface((void**)&nativeAcc);
>+
>   if (!nativeAcc)

no reason to add whitespace.

>-ProxyEvent(ProxyAccessible*, uint32_t)
>+ProxyEvent(ProxyAccessible* aProxy, uint32_t aEventType)
> {
>+  // ignore everything but focus-changed, value-changed, caret and selection
>+  // events for now.
>+  if (aEventType != nsIAccessibleEvent::EVENT_FOCUS &&
>+      aEventType != nsIAccessibleEvent::EVENT_VALUE_CHANGE &&
>+      aEventType != nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED &&
>+      aEventType != nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED)

I'm not really convinced it wouldn't be better to save the dupplication and always call HandleProxyOrNativeEvent, but meh

>+    return;
>+
>+  mozAccessible* wrapper =
>+    reinterpret_cast<mozAccessible*>(aProxy->GetWrapper());

seems like a wrapper function to handle the cast would be nice.

I think you need a null check here, but I'm not really sure.

> void
> ProxyCaretMoveEvent(ProxyAccessible* aTarget, int32_t aOffset)
> {
>+  mozAccessible* wrapper =
>+    reinterpret_cast<mozAccessible*>(aTarget->GetWrapper());
>+  [wrapper selectedTextDidChange];

same

	Lorien Hu (:lsocks) Assignee
	Comment 6 • 9 years ago

Carry r=tbsaunde

	Trevor Saunders (:tbsaunde)
	Comment 9 • 9 years ago

Comment on attachment 8615386 [details] [diff] [review]
Wrap getting mozAccessibles from proxies

>+GetNativeFromProxy(mozilla::a11y::ProxyAccessible* aProxy)
>+{
>+  return aProxy ? reinterpret_cast<mozAccessible*>(aProxy->GetWrapper()) :
>+                  nullptr;

so, all the existing callers should never pass null, so why is this checking for it?

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1df389a3ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ada2f2d007

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/2a1df389a3ed
https://hg.mozilla.org/mozilla-central/rev/55ada2f2d007