567713 - accessibleEvent view should show changed state rather than current state of accessible

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[[e-mail]]

Sorry, I don't understand this bug. Can you provide an example?

Comment 2

[alexander :surkov (:asurkov)]

Sorry I didn't provide a bug description. So the scenario is. When accessible state is changed then a11y fires state change event where changed state and its value are stored, so when I inspect accessible event (by accessibleEvent view) then I should see the changed state and its value, now I see the current all states of target accessible of inspected event.

Comment 3

[[e-mail]]

Comment on attachment 447025 [details] [diff] [review]
patch

>+    var state = 0, extraState = 0;
>+    if (this.mAccEventSubject.isExtraState()) {
>+      extraState = this.mAccEventSubject.state;
>+    }
>+    else {
>+      state = this.mAccEventSubject.state;
>+    }
[Don't know what local {} style is but the if you deleted didn't have them.]

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

I'm really not good to review accessibility stuff. I can do general inspector internals and DOM stuff, but I don't have a decent understanding of the accessibility APIs. I especially don't have a nuanced understanding where I can catch mistakes and point out caveats about the interfaces you're using and other stuff you should be able to get during review.

Since I don't know anything about what "extra state" means, and the IDL is unhelpful, what are the circumstances where isExtraState will be false? I guess the for loop is there for that case, since the list returned from getStringStates should only have one element when isExtraState will be true?

This seems okay as far as language use and interface availability goes, though. If Neil says it looks good, r+.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 447025 [details] [diff] [review]
patch

Looks right.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Note hg log should mention r=crussell since his conditional r+ passed.

Comment 7

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/dom-inspector/rev/fec2ea76f90f