164763 - no enough arguments for ATK text-change and state-change events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Kyle Yuan]

needs to pass AtkStateChange/AtkTextChange data to the event handler.

Comment 1

[Kyle Yuan]

Attachment: fix nsRootAccessible::HandleEvent()

1) passed more arguments to mListener when EVENT_STATE_CHANGE and
EVENT_ATK_TEXT_CHANGE were fired;
2) remove EVENT_ATK_SELECTION_CHANGE event, because gnopernicus doesn't care
about that, it uses focus event instead;
3) fire a focus event when received "ListitemStateChange"
4) fire EVENT_REORDER when document was changed. at-poke need this event to
update its acc object tree.

All changes made within #ifdef MOZ_ACCESSIBILITY_ATK/#endif block

Seeking r=

Comment 2

[Aaron Leventhal]

Comment on attachment 98391 [details] [diff] [review]
fix nsRootAccessible::HandleEvent()

What do you mean, // currently,  we don't need to fire
EVENT_ATK_SELECTION_CHANGE here
Is everything based on Gnopernicus' needs?
Is Gnopernicus the only assistive technolgy you want to be compatible with?
Why not fire the event and let the assistive technology decide if it needs it?

-	 if (selectElement) // it's a HTML <select>
-	  
mListener->HandleEvent(nsIAccessibleEventListener::EVENT_ATK_SELECTION_CHANGE,
accessible, nsnull);
+	 if (selectElement) { // it's a HTML <select>
+	   // currently,  we don't need to fire EVENT_ATK_SELECTION_CHANGE here
+	 }

Is this going to change sometime in the future? 
Shouldn't we be calling some method on accessible text objects to get textData?
It doesn't look right.
+	 textData.start = 0;
+	 textData.length = 0;
+	 textData.add = PR_TRUE;

Above comment applies to childrenData.

This won't set the correct value for true (1). enable is a boolean.
If they check stateData.enable == TRUE, it's really comparing to 1.
stateData.enable = stateData.state & STATE_CHECKED;
I think you want
stateData.enable = (stateData.state & STATE_CHECKED) != 0

Don't you need to remove these lins from the "focus" handler, since tree items
get "select" now?
	  nsAccessibilityService::GetShellFromNode(targetNode,
getter_AddRefs(weakShell));
	  accessible = new nsXULTreeitemAccessible(accessible, targetNode,
weakShell, treeIndex);
Perhaps that code should be moved up in the method, next to
GetFocusedOptionNode(), so that it
doesn't need to be repeated in the 2 ATK and MSAA areas of this method.
Or, maybe we should combine that kind of stuff into 1 method,
GetEventAccessible() or something.

Comment 3

[Kyle Yuan]

> Why not fire the event and let the assistive technology decide if it needs it
Yes, Gnopernicus doesn't handle this kind of event yet. And in a near future, 
it's the only AT application in *nix platform. But I still can put the original 
code back for future use.

> Is this going to change sometime in the future? 
> Shouldn't we be calling some method on accessible text objects to get 
> textData?
> It doesn't look right.
It's hard to me to get which part of text was changed in that moment, because 
the content already changed. 

> Above comment applies to childrenData.
childrenData is different. childrenData.index = 0 and childrenData.child = 0 
means the all children were changed. Maybe I should assign -1 to 
childrenData.index.

> Don't you need to remove these lins from the "focus" handler, since tree items
> get "select" now?
Sorry for my low quality code. I'll simplify it.

Comment 4

[Kyle Yuan]

Attachment: new patch per aaron's suggestion

changed per aaron's suggestion, reordered some code.

seeking r=.

Comment 5

[Kyle Yuan]

Attachment: sorry, should return NS_OK instead of NS_ERROR_FAILURE even if GetAccessibleFor failed

Comment 6

[Aaron Leventhal]

Comment on attachment 98557 [details] [diff] [review]
sorry, should return NS_OK instead of NS_ERROR_FAILURE even if GetAccessibleFor failed

r=aaronl
Thanks Kyle.

Please add in a comment to explain this code, something like
// XXX [e-mail] uture work, put correct values for text change data

+      textData.start = 0;
+      textData.length = 0;
+      textData.add = PR_TRUE;

No new patch necessary for the comment.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 98557 [details] [diff] [review]
sorry, should return NS_OK instead of NS_ERROR_FAILURE even if GetAccessibleFor failed

sr=jst

Comment 8

[Kyle Yuan]

checked in!