Closed Bug 1164976 Opened 6 years ago Closed 6 years ago

fire caret move and state change events for proxies[

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(3 files)

No description provided.
Comment on attachment 8605942 [details] [diff] [review]
move AccessibleWrap::FireStateChangeEvent to be a member of MaiAtkObject

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

ok r=me. We have a comment somewhere that "MaiAtkObject is a thin wrapper"; at some point we might want to edit that.

::: accessible/atk/AccessibleWrap.cpp
@@ +1128,5 @@
>  
>      switch (type) {
>      case nsIAccessibleEvent::EVENT_STATE_CHANGE:
> +      {
> +        AccStateChangeEvent* event = downcast_accEvent(aEvent);

We used to NS_ENSURE_TRUE here which was our pattern when using QueryInterface to downcast... should we keep it?

@@ +1130,5 @@
>      case nsIAccessibleEvent::EVENT_STATE_CHANGE:
> +      {
> +        AccStateChangeEvent* event = downcast_accEvent(aEvent);
> +        MAI_ATK_OBJECT(atkObj)-> FireStateChangeEvent(event->GetState(),
> +                                                      event->IsStateEnabled());

nit: unnecessary space after the initial dereference '->'
Attachment #8605942 - Flags: review?(dbolter) → review+
Attachment #8605945 - Flags: review?(dbolter) → review+
Comment on attachment 8605946 [details] [diff] [review]
fire useful state change and caret move events for proxies

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

r=me with nit and question.

::: accessible/base/EventQueue.cpp
@@ +503,5 @@
>          ipcDoc->SendEvent(id, aEvent->GetEventType());
>        break;
> +    case nsIAccessibleEvent::EVENT_STATE_CHANGE: {
> +        AccStateChangeEvent* event = downcast_accEvent(aEvent);
> +        ipcDoc->SendStateChangeEvent(id, event->GetState(),

Do we need to ensure event or is the downcast guaranteed?

@@ +510,5 @@
> +      }
> +    case nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED: {
> +                                                       AccCaretMoveEvent* event = downcast_accEvent(aEvent);
> +                                                       ipcDoc->SendEvent(id, event->GetCaretOffset());
> +                                                       break;

The indenting here went haywire.
Attachment #8605946 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #4)
> Comment on attachment 8605942 [details] [diff] [review]
> move AccessibleWrap::FireStateChangeEvent to be a member of MaiAtkObject
> 
> Review of attachment 8605942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok r=me. We have a comment somewhere that "MaiAtkObject is a thin wrapper";
> at some point we might want to edit that.
> 
> ::: accessible/atk/AccessibleWrap.cpp
> @@ +1128,5 @@
> >  
> >      switch (type) {
> >      case nsIAccessibleEvent::EVENT_STATE_CHANGE:
> > +      {
> > +        AccStateChangeEvent* event = downcast_accEvent(aEvent);
> 
> We used to NS_ENSURE_TRUE here which was our pattern when using
> QueryInterface to downcast... should we keep it?

It should never fail and if does I think its a bug so crashing seems better than pretending its ok.


> @@ +503,5 @@
> >          ipcDoc->SendEvent(id, aEvent->GetEventType());
> >        break;
> > +    case nsIAccessibleEvent::EVENT_STATE_CHANGE: {
> > +        AccStateChangeEvent* event = downcast_accEvent(aEvent);
> > +        ipcDoc->SendStateChangeEvent(id, event->GetState(),
> 
> Do we need to ensure event or is the downcast guaranteed?

same
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.