756502 - Add reason of change to pivot change notifications

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

I'm thinking that it is important for an observer to know how a pivot was moved. So to
nsIAccessiblePivotObserver.onPivotChanged, I would add:
  void onPivotChanged(in nsIAccessiblePivot aPivot,
                      in nsIAccessible aOldAccessible,
                      in long aOldStart, in long aOldEnd,
                      in PivotChangedReason aReason);

where PivotMovedReason could be:
PIVOT_CHANGED_REASON_ASSIGNED - the position was set directly (i.e. pivot.position = foo)
PIVOT_CHANGED_REASON_MOVED_NEXT - moveNext()
PIVOT_CHANGED_REASON_MOVED_PREV - movePrevious()
PIVOT_CHANGED_REASON_MOVED_FIRST - moveFirst()
PIVOT_CHANGED_REASON_MOVED_LAST - moveLast()
PIVOT_CHANGED_REASON_MOVED_POINT - movePoint()

That might be too high granularity, really I am just interested in assigned, moved in order and moved to point. But if we are doing this, why not...

nsIAccessibleVirtualCursorChangeEvent would get a new equivalent field as well. maybe
nsIAccessibleVirtualCursorChangeEvent.reason

Comment 1

[Eitan Isaacson [:eeejay]]

Another idea might be to provide a move id on a successful move. Kind of like how setTimeout returns a handle. This will make it easier to associate move operations with their subsequent notifiers.

So instead of the move functions returning true or false, they would return unsigned long. 0 on failure or anything else on success.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Added reason to pivot change notifications

The presenter may choose to display a cursor movement differently depending on the type of movement. For example, in Android explore by touch emits different events than d-pad navigation.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Can you give me the "killer use case" concrete example?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #3)
> Can you give me the "killer use case" concrete example?

When receiving a pivot previous/next move, send an Android TYPE_VIEW_FOCUSED event. When receiving a pivot move instigated by MoveToPoint, send an Android TYPE_VIEW_HOVER_ENTER event.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #3)
> Can you give me the "killer use case" concrete example?

it seems like this is within reason if you believe using events  in the first place is reasonable.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> (In reply to David Bolter [:davidb] from comment #3)
> > Can you give me the "killer use case" concrete example?
> 
> When receiving a pivot previous/next move, send an Android TYPE_VIEW_FOCUSED
> event. When receiving a pivot move instigated by MoveToPoint, send an
> Android TYPE_VIEW_HOVER_ENTER event.

OK.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 633688 [details] [diff] [review]
Added reason to pivot change notifications

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

r=me with nits addressed please with a cherry on top.

Do you have tests in github for this stuff?

::: accessible/public/nsIAccessiblePivot.idl
@@ +6,5 @@
>  
>  #include "nsISupports.idl"
>  
>  typedef short TextBoundaryType;
> +typedef short PivotMoveReason;

My personal preference would be to not bother with the typedef.

::: accessible/src/base/AccEvent.h
@@ +390,5 @@
>    // AccTableChangeEvent
>    nsIAccessible* OldAccessible() const { return mOldAccessible; }
>    PRInt32 OldStartOffset() const { return mOldStart; }
>    PRInt32 OldEndOffset() const { return mOldEnd; }
> +  PRInt32 Reason() const { return mReason; }

You probably meant the return type to be PRInt16?

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 756502 - Added reason to pivot change notifications

Added mochitests. Started try run to make tuse this works on all platforms.
https://tbpl.mozilla.org/?tree=Try&rev=e608fd9f7709

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 634669 [details] [diff] [review]
Bug 756502 - Added reason to pivot change notifications

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

::: accessible/tests/mochitest/pivot.js
@@ +85,5 @@
>        SimpleTest.ok(false, "Does not support correct interface: " + e);
>      }
>  
> +    SimpleTest.is(
> +      event.reason,

nit: why the newline already?

@@ +136,5 @@
>  {
>    return VCChangedChecker.prevPosAndOffset[aPivot];
>  };
>  
> +VCChangedChecker.methodReasonMap = {

I wonder if there is some way to ensure this is synced with nsIAccessiblePivot.idl reason constants?

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Added reason to pivot change notifications. r=davidb

Nits addressed.

Comment 11

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/60468d1cc060

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60468d1cc060

Comment 13

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

Comment on attachment 635060 [details] [diff] [review]
Added reason to pivot change notifications. r=davidb

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

Missed uuid revs...