374711 - ARIA state change events not working in ATK

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

When an ARIA state liked checked or expanded changes, we should fire the correct ATK state change event.

Comment 1

[Aaron Leventhal]

Attachment: Set the stateData that goes with the state change event

Test cases:
http://www.mozilla.org/access/dhtml/checkbox (checked)
http://www.mozilla.org/access/dhtml/tree (expanded)

Comment 2

[Aaron Leventhal]

Ginn, I didn't test the patch but it should work. Will you try it for checkbox and tree?

Comment 3

[Ginn Chen]

Comment on attachment 259167 [details] [diff] [review]
Set the stateData that goes with the state change event

aaron, this doesn't work because stateData will be gone after the timer.
We cannot use &stateData in this case.

Comment 4

[Aaron Leventhal]

Yes that was very stupid of me! It reminded me of all the things we need to do for the event system in general, so I filed bug 374790 on that.

For this bug perhaps the answer is to pass the actual struct around instead of a pointer to it. Unfortunately each kind of event data is a different struct. So we probably need to turn it into a class and have each kind of event data be a different class, which inherits from a base event data class.

Does that make sense?

Comment 5

[Aaron Leventhal]

Alexander, if you get a chance to look at this and bug 374790 it would be great.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Alexander, if you get a chance to look at this and bug 374790 it would be
> great.
> 

Yes, I will. I'll try to do soon.

Comment 7

[Aaron Leventhal]

Surkov, when you get to this, can you fix bug 255088 at the same time? I think the best thing is just to fire the STATE_CHANGE with event data for STATE_SENSITIVE, when the disabled attribute changes no matter what namespace the attribute is in. That will catch XUL, ARIA and HTML disabled state changes.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[Aaron Leventhal]

Comment on attachment 261669 [details] [diff] [review]
patch2

I have a feeling that new event constructor is too early to detect if state is set, because attribute change hasn't occured yet.
I think it was why we used FireDelayedToolkitEvent() instead of FireToolkitEvent() (which we should comment about).
Test it, because I might be wrong. You can use http://www.mozilla.org/access/dhtml/checkbox with with accevent from MSAA SDK to test.

Comment 10

[Aaron Leventhal]

Comment on attachment 261669 [details] [diff] [review]
patch2

Actually I tested on Linux and this works perfectly, so ignore my previous comment. But, test and make sure that events on Windows still work.

mIsEnabled = (mIsExtraState ? extraState : state) & mState;
This needs to be 
mIsEnabled = 0 != ((mIsExtraState ? extraState : state) & mState);
Or 
!!(mIsExtraState ? extraState : state) & mState)
or saomething like that, otherwise the boolean gets an int.

Optimization for ARIAAttributeChanged(), pull this out to the top of the method, before the block of if's:
+    nsCOMPtr<nsIAccessibleEvent> event;
In each attribute check, just set the event, don't fire or return.
Then at the end of the method, do:
if (event) {
  FireDelayedAccessibleEvent(event);
}

For disabled, we also need to fire state change for sensitive. Also, we need to do that one for HTML and XUL namespace as well. See  bug 255088

Comment 11

[Aaron Leventhal]

Comment on attachment 261669 [details] [diff] [review]
patch2

Actually you can have + if you deal with my comments in the checkin or in another bug.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3 (for checkin)

The patch has one problem related with delayed events. Sometimes accessible is shutdowned before delayed event is proccessed. I'll file new bug for this.

Comment 13

[alexander :surkov (:asurkov)]

checked in

Comment 14

[Aaron Leventhal]

It doesn't look like the 2nd part of comment 10 was checked in, right?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> It doesn't look like the 2nd part of comment 10 was checked in, right?
> 

Do you mean this?

(In reply to comment #10)

> In each attribute check, just set the event, don't fire or return.
> Then at the end of the method, do:
> if (event) {
>   FireDelayedAccessibleEvent(event);
> }

Yes, I missed. Sorry. Should I fix it? (I'm just not big fan of if/else statements).