Closed Bug 1169701 Opened 4 years ago Closed 4 years ago

ProxyEvent, ProxyStateChangeEvent, ProxyCaretMoveEvent should inform mozAccessible

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attachment #8612994 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8612994 - Flags: review?(tbsaunde+mozbugs)
Attachment #8612994 - Attachment is obsolete: true
Attachment #8614207 - Flags: review?(tbsaunde+mozbugs)
Attachment #8614207 - Attachment is obsolete: true
Attachment #8614207 - Flags: review?(tbsaunde+mozbugs)
Attachment #8614209 - Flags: review?(tbsaunde+mozbugs)
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
Attachment #8614209 - Flags: review?(tbsaunde+mozbugs) → review+
Carry r=tbsaunde
Attachment #8614209 - Attachment is obsolete: true
Attachment #8615377 - Flags: review+
Attachment #8615381 - Flags: review?(tbsaunde+mozbugs)
Attachment #8615381 - Attachment is obsolete: true
Attachment #8615381 - Flags: review?(tbsaunde+mozbugs)
Attachment #8615386 - Flags: review?(tbsaunde+mozbugs)
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?
Attachment #8615386 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/2a1df389a3ed
https://hg.mozilla.org/mozilla-central/rev/55ada2f2d007
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.