fire caret move and state change events for proxies[

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8605942 [details] [diff] [review]
move AccessibleWrap::FireStateChangeEvent to be a member of MaiAtkObject
Attachment #8605942 - Flags: review?(dbolter)
(Assignee)

Comment 2

3 years ago
Created attachment 8605945 [details] [diff] [review]
Make DocAccessibleParent::GetAccessible return itself when appropriate
Attachment #8605945 - Flags: review?(dbolter)
(Assignee)

Comment 3

3 years ago
Created attachment 8605946 [details] [diff] [review]
fire useful state change and caret move events for proxies
Attachment #8605946 - Flags: review?(dbolter)
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+
(Assignee)

Comment 6

3 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/d9afa8ff148e
https://hg.mozilla.org/mozilla-central/rev/661f089f73b5
https://hg.mozilla.org/mozilla-central/rev/e5649b9714d1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.