Closed Bug 1074869 Opened 6 years ago Closed 6 years ago

Make atk deal with proxied focus events

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(2 files)

I imagine at some point we may need to do something smarter because of multiple windows and what not, but it seems to me this patch is better than nothing.
Summary: Make deal with proxied focus events → Make atk deal with proxied focus events
Comment on attachment 8497534 [details] [diff] [review]
make atk deal with proxied focus events

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

ok
(note to readers: requires bug 982842)

::: accessible/ipc/DocAccessibleParent.cpp
@@ +118,5 @@
> +    return true;
> +  }
> +
> +  ProxyAccessible* proxy = e->mProxy;
> +  ProxyEvent(proxy, aEventType);

Should we assert for proxy here? I think ProxyEvent fails silently otherwise. Not sure how I feel about that.
Attachment #8497534 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 8497534 [details] [diff] [review]
> make atk deal with proxied focus events
> 
> Review of attachment 8497534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok
> (note to readers: requires bug 982842)
> 
> ::: accessible/ipc/DocAccessibleParent.cpp
> @@ +118,5 @@
> > +    return true;
> > +  }
> > +
> > +  ProxyAccessible* proxy = e->mProxy;
> > +  ProxyEvent(proxy, aEventType);
> 
> Should we assert for proxy here? I think ProxyEvent fails silently
> otherwise. Not sure how I feel about that.

probably, though I'm not really sure why we need to fail silently, its kind of coppied from AccessibleWrap::HandleEvent
Comment on attachment 8519306 [details] [diff] [review]
make atk deal with proxied focus events

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

::: accessible/atk/AccessibleWrap.cpp
@@ +1309,5 @@
>  
> +void
> +a11y::ProxyEvent(ProxyAccessible* aTarget, uint32_t aEventType)
> +{
> +  AtkObject* wrapper = GetWrapperFor(aTarget);

Why did you remove the !wrapper check?

::: accessible/base/EventQueue.cpp
@@ +481,5 @@
>  }
>  
> +void
> +EventQueue::SendIPCEvent(AccEvent* aEvent) const
> +{

Good idea.

@@ +497,5 @@
> +      break;
> +
> +    case nsIAccessibleEvent::EVENT_REORDER:
> +      // reorder events on the application acc aren't necessary to tell the parent
> +      // about new top level documents.

I'm curious what happens if we do?
Attachment #8519306 - Flags: review?(dbolter) → review+
> ::: accessible/atk/AccessibleWrap.cpp
> @@ +1309,5 @@
> >  
> > +void
> > +a11y::ProxyEvent(ProxyAccessible* aTarget, uint32_t aEventType)
> > +{
> > +  AtkObject* wrapper = GetWrapperFor(aTarget);
> 
> Why did you remove the !wrapper check?

I don't see how it can possibly be true

> @@ +497,5 @@
> > +      break;
> > +
> > +    case nsIAccessibleEvent::EVENT_REORDER:
> > +      // reorder events on the application acc aren't necessary to tell the parent
> > +      // about new top level documents.
> 
> I'm curious what happens if we do?

we get an assertion because the parent process doesn't know about the accessible for the childs Application acc.
https://hg.mozilla.org/mozilla-central/rev/f991f366f66e
Assignee: nobody → tbsaunde+mozbugs
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.