Closed Bug 756502 Opened 7 years ago Closed 7 years ago

Add reason of change to pivot change notifications

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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.
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.
Attachment #633688 - Flags: review?(dbolter)
Can you give me the "killer use case" concrete example?
(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.
(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.
(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 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?
Attachment #633688 - Flags: review?(dbolter) → review+
Added mochitests. Started try run to make tuse this works on all platforms.
https://tbpl.mozilla.org/?tree=Try&rev=e608fd9f7709
Attachment #633688 - Attachment is obsolete: true
Attachment #634669 - Flags: review?(dbolter)
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?
Attachment #634669 - Flags: review?(dbolter) → review+
Nits addressed.
Attachment #634669 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/60468d1cc060
Assignee: nobody → eitan
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/60468d1cc060
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 757721
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...
You need to log in before you can comment on or make changes to this bug.