729384 - Need a special interface for virtual cursor change events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

A special interface for vc change events would be very helpful. This will eliminate the need to keep a reference to the previous vc position.

In the screen reader case, it should be announced that we are entering a table or list. In order to do that we need to compare the lineage of the previous vc position to know that we just stepped into the list.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Created an interface for virtual cursor changed events.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 599444 [details] [diff] [review]
Created an interface for virtual cursor changed events.

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

please add testing to virtualCursorChangedChecker of pivot.js

patch is ok but many nits, I'd take a look at next version to make sure I didn't missed anything

::: accessible/public/nsIAccessibleEvent.idl
@@ +41,5 @@
>  
>  interface nsIAccessible;
>  interface nsIAccessibleDocument;
>  interface nsIDOMNode;
> +interface nsIAccessiblePivot;

why do you need it here?

@@ +571,5 @@
>     */
>    readonly attribute long numRowsOrCols;
>  };
>  
> +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]

comment it please
extra new line (two empty lines between interfaces)

@@ +572,5 @@
>    readonly attribute long numRowsOrCols;
>  };
>  
> +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> +interface nsIAccessibleVirtualCursorChangeEvent: nsISupports

space before :

::: accessible/src/base/AccEvent.cpp
@@ +379,5 @@
>    NS_IF_ADDREF(event);
>    return event;
>  }
>  
> +////////////////////////////////////////////////////////////////////////////////

extra empty line before this comment (two empty lines between classes impl)

@@ +387,5 @@
> +AccVirtualCursorChangeEvent::
> +  AccVirtualCursorChangeEvent(nsAccessible* aAccessible,
> +                              nsIAccessible* aOldAccessible,
> +                              PRInt32 aOldStart, PRInt32 aOldEnd) :
> +    AccEvent(nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, aAccessible),

do ::nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, there were compilation problems on some platforms

@@ +396,5 @@
> +already_AddRefed<nsAccEvent>
> +AccVirtualCursorChangeEvent::CreateXPCOMObject()
> +{
> +  nsAccEvent* event = new nsAccVirtualCursorChangeEvent(this);
> +  NS_IF_ADDREF(event);

NS_ADDREF

::: accessible/src/base/AccEvent.h
@@ +402,5 @@
>  
> +/**
> + * Accessible virtual cursor change event.
> + */
> +class AccVirtualCursorChangeEvent : public AccEvent

maybe name it AccVCChnageEvent to keep it shorter (compare to, for example, AccSelChangeEvent)

@@ +408,5 @@
> +public:
> +  AccVirtualCursorChangeEvent(nsAccessible* aAccessible,
> +                              nsIAccessible* aOldAccessible,
> +                              PRInt32 aOldStart, PRInt32 aOldEnd);
> +

please add virtual destructor to be consistent with other classes here

@@ +421,5 @@
> +
> +  // AccTableChangeEvent
> +  nsIAccessible* GetOldAccessible() const { return mOldAccessible; }
> +  PRInt32 GetOldStartOffset() const { return mOldStart; }
> +  PRInt32 GetOldEndOffset() const { return mOldEnd; }

remove "Get" prefix from these

@@ +424,5 @@
> +  PRInt32 GetOldStartOffset() const { return mOldStart; }
> +  PRInt32 GetOldEndOffset() const { return mOldEnd; }
> +
> +private:
> +  nsRefPtr<nsIAccessible> mOldAccessible;

that's not friendly to dexpcomination but virtual cursor is not friendly to that at all, so it's not wanted but nothing nice comes to my mind

::: accessible/src/xpcom/nsAccEvent.cpp
@@ +260,5 @@
> +NS_IMPL_ISUPPORTS_INHERITED1(nsAccVirtualCursorChangeEvent, nsAccEvent,
> +                             nsIAccessibleVirtualCursorChangeEvent)
> +
> +NS_IMETHODIMP
> +nsAccVirtualCursorChangeEvent::GetOldStartOffset(PRInt32 *aOldStartOffset)

PRInt32*

@@ +270,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsAccVirtualCursorChangeEvent::GetOldEndOffset(PRInt32 *aOldEndOffset)

same

@@ +280,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsAccVirtualCursorChangeEvent::GetOldAccessible(nsIAccessible** aOldAccessible)

it'd be nice to keep it the same order as it's defined in idl

::: accessible/src/xpcom/nsAccEvent.h
@@ +180,5 @@
> +
> +private:
> +  nsAccVirtualCursorChangeEvent();
> +  nsAccVirtualCursorChangeEvent(const nsAccVirtualCursorChangeEvent&);
> +  nsAccVirtualCursorChangeEvent& operator =(const nsAccVirtualCursorChangeEvent&);

add MOZ_DELETE, not necessary thing but it seems to be mozilla style

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Bug 729384 - Created an interface for virtual cursor changed events.

Added mochitests too

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Mochitest for nsIAccessibleVirtualCursorChangeEvent

Oops, the mochitests atr here.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 599444 [details] [diff] [review]
> Created an interface for virtual cursor changed events.
> 
> Review of attachment 599444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please add testing to virtualCursorChangedChecker of pivot.js
> 
> patch is ok but many nits, I'd take a look at next version to make sure I
> didn't missed anything
> 
> ::: accessible/public/nsIAccessibleEvent.idl
> @@ +41,5 @@
> >  
> >  interface nsIAccessible;
> >  interface nsIAccessibleDocument;
> >  interface nsIDOMNode;
> > +interface nsIAccessiblePivot;
> 
> why do you need it here?

Don't, removed.

> 
> @@ +571,5 @@
> >     */
> >    readonly attribute long numRowsOrCols;
> >  };
> >  
> > +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> 
> comment it please
> extra new line (two empty lines between interfaces)
> 

Done, but there are no double newlines anywhere else in the file.

> @@ +572,5 @@
> >    readonly attribute long numRowsOrCols;
> >  };
> >  
> > +[scriptable, uuid(370e8b9b-2bbc-4bff-a9c7-16ddc54aea21)]
> > +interface nsIAccessibleVirtualCursorChangeEvent: nsISupports
> 
> space before :
> 

Done, but this is not the style in the rest of the file.

> ::: accessible/src/base/AccEvent.cpp
> @@ +379,5 @@
> >    NS_IF_ADDREF(event);
> >    return event;
> >  }
> >  
> > +////////////////////////////////////////////////////////////////////////////////
> 
> extra empty line before this comment (two empty lines between classes impl)
>

Done.
 
> @@ +387,5 @@
> > +AccVirtualCursorChangeEvent::
> > +  AccVirtualCursorChangeEvent(nsAccessible* aAccessible,
> > +                              nsIAccessible* aOldAccessible,
> > +                              PRInt32 aOldStart, PRInt32 aOldEnd) :
> > +    AccEvent(nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, aAccessible),
> 
> do ::nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, there were compilation
> problems on some platforms
> 
> @@ +396,5 @@
> > +already_AddRefed<nsAccEvent>
> > +AccVirtualCursorChangeEvent::CreateXPCOMObject()
> > +{
> > +  nsAccEvent* event = new nsAccVirtualCursorChangeEvent(this);
> > +  NS_IF_ADDREF(event);
> 
> NS_ADDREF
> 

Done.

> ::: accessible/src/base/AccEvent.h
> @@ +402,5 @@
> >  
> > +/**
> > + * Accessible virtual cursor change event.
> > + */
> > +class AccVirtualCursorChangeEvent : public AccEvent
> 
> maybe name it AccVCChnageEvent to keep it shorter (compare to, for example,
> AccSelChangeEvent)
> 

Done.


> @@ +408,5 @@
> > +public:
> > +  AccVirtualCursorChangeEvent(nsAccessible* aAccessible,
> > +                              nsIAccessible* aOldAccessible,
> > +                              PRInt32 aOldStart, PRInt32 aOldEnd);
> > +
> 
> please add virtual destructor to be consistent with other classes here
> 

Done.

> @@ +421,5 @@
> > +
> > +  // AccTableChangeEvent
> > +  nsIAccessible* GetOldAccessible() const { return mOldAccessible; }
> > +  PRInt32 GetOldStartOffset() const { return mOldStart; }
> > +  PRInt32 GetOldEndOffset() const { return mOldEnd; }
> 
> remove "Get" prefix from these
> 

Done.

> @@ +424,5 @@
> > +  PRInt32 GetOldStartOffset() const { return mOldStart; }
> > +  PRInt32 GetOldEndOffset() const { return mOldEnd; }
> > +
> > +private:
> > +  nsRefPtr<nsIAccessible> mOldAccessible;
> 
> that's not friendly to dexpcomination but virtual cursor is not friendly to
> that at all, so it's not wanted but nothing nice comes to my mind
> 

Didn't do anything :)

> ::: accessible/src/xpcom/nsAccEvent.cpp
> @@ +260,5 @@
> > +NS_IMPL_ISUPPORTS_INHERITED1(nsAccVirtualCursorChangeEvent, nsAccEvent,
> > +                             nsIAccessibleVirtualCursorChangeEvent)
> > +
> > +NS_IMETHODIMP
> > +nsAccVirtualCursorChangeEvent::GetOldStartOffset(PRInt32 *aOldStartOffset)
> 
> PRInt32*
> 

Done.

> @@ +270,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsAccVirtualCursorChangeEvent::GetOldEndOffset(PRInt32 *aOldEndOffset)
> 
> same
> 

Done,

> @@ +280,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsAccVirtualCursorChangeEvent::GetOldAccessible(nsIAccessible** aOldAccessible)
> 
> it'd be nice to keep it the same order as it's defined in idl
> 

Done.

> ::: accessible/src/xpcom/nsAccEvent.h
> @@ +180,5 @@
> > +
> > +private:
> > +  nsAccVirtualCursorChangeEvent();
> > +  nsAccVirtualCursorChangeEvent(const nsAccVirtualCursorChangeEvent&);
> > +  nsAccVirtualCursorChangeEvent& operator =(const nsAccVirtualCursorChangeEvent&);
> 
> add MOZ_DELETE, not necessary thing but it seems to be mozilla style

Done.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 599812 [details] [diff] [review]
Bug 729384 - Created an interface for virtual cursor changed events.

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

r=me

Trevor had a question about nsRefPtr/nsCOMPtr usage, I leave this issue to him

::: accessible/public/nsIAccessibleEvent.idl
@@ +573,5 @@
>  
> +
> +/*
> + * An interface for virtual cursor changed events.
> + * Passes previous cursor position and text offsets.

passes -> provides, don't put new sentence on new line

@@ +584,5 @@
> +   */
> +  readonly attribute nsIAccessible oldAccessible;
> +
> +  /**
> +   * Previous start offset of pivot. -1 if none.

sometimes you use virtual cursor, sometimes pivot in idl file, please be consistent

::: accessible/src/base/AccEvent.cpp
@@ +389,5 @@
> +  AccVCChangeEvent(nsAccessible* aAccessible,
> +                   nsIAccessible* aOldAccessible,
> +                   PRInt32 aOldStart, PRInt32 aOldEnd) :
> +    AccEvent(::nsIAccessibleEvent::EVENT_VIRTUALCURSOR_CHANGED, aAccessible),
> +    mOldAccessible(aOldAccessible), mOldStart(aOldStart), mOldEnd(aOldEnd)

we use two spaces offset for members construction

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 599813 [details] [diff] [review]
Mochitest for nsIAccessibleVirtualCursorChangeEvent

redirecting review to Marco since it might be I can't accomplish it today

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 599813 [details] [diff] [review]
Mochitest for nsIAccessibleVirtualCursorChangeEvent

r=me. I find it a bit confusing that the prevPosition gets event.accessible as the comparison. But that's just my thinking of position in coordinate terms rather than object terms.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec7f2468d66
https://hg.mozilla.org/integration/mozilla-inbound/rev/c53de99a72cb

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7ec7f2468d66
https://hg.mozilla.org/mozilla-central/rev/c53de99a72cb