The default bug view has changed. See this FAQ.

Add reason of change to pivot change notifications

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
Attachment #633688 - Flags: review?(dbolter)
Can you give me the "killer use case" concrete example?
(Assignee)

Comment 4

5 years ago
(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+
(Assignee)

Comment 8

5 years ago
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
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+
(Assignee)

Comment 10

5 years ago
Created attachment 635060 [details] [diff] [review]
Added reason to pivot change notifications. r=davidb

Nits addressed.
Attachment #634669 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
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.