Last Comment Bug 756502 - Add reason of change to pivot change notifications
: Add reason of change to pivot change notifications
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks: 757721
  Show dependency treegraph
 
Reported: 2012-05-18 10:15 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-06-22 03:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Added reason to pivot change notifications (15.24 KB, patch)
2012-06-15 15:01 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Bug 756502 - Added reason to pivot change notifications (19.32 KB, patch)
2012-06-19 17:19 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Added reason to pivot change notifications. r=davidb (19.50 KB, patch)
2012-06-20 14:05 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-05-18 10:15:08 PDT
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] 2012-06-12 16:33:16 PDT
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] 2012-06-15 15:01:27 PDT
Created attachment 633688 [details] [diff] [review]
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] 2012-06-18 11:01:10 PDT
Can you give me the "killer use case" concrete example?
Comment 4 Eitan Isaacson [:eeejay] 2012-06-18 14:43:35 PDT
(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) 2012-06-19 06:57:54 PDT
(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] 2012-06-19 07:00:57 PDT
(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] 2012-06-19 07:23:16 PDT
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] 2012-06-19 17:19:35 PDT
Created attachment 634669 [details] [diff] [review]
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] 2012-06-20 06:56:16 PDT
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] 2012-06-20 14:05:43 PDT
Created attachment 635060 [details] [diff] [review]
Added reason to pivot change notifications. r=davidb

Nits addressed.
Comment 12 Ed Morley [:emorley] 2012-06-21 04:04:35 PDT
https://hg.mozilla.org/mozilla-central/rev/60468d1cc060
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-06-22 03:28:26 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.