367412 - (statesa11y) Fix Mozilla's usage of states, and refactor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

While investigating STATE_SHOWING I have found a lot of Mozilla's extended states are not implemented or not implemented correctly.

We need these fixes for IAccessible2 support as well.

Comment 1

[Aaron Leventhal]

Attachment: Work in progress -- want to combine TranslateAState and TranslateStates

Comment 2

[Aaron Leventhal]

To do:
Combine TranslateAState and TranslateStates
Clean up comments
Support ATK_STATE_EDITABLE in form controls that aren't readonly, otherwise the AT can't know when they are readonly

Comment 3

[Aaron Leventhal]

Attachment: WIP. More big changes, however still no answer on STATE_EDITABLE on combo boxes. Not 100% clear on TRANSIENT yet either.

Comment 4

[Aaron Leventhal]

Attachment: WIP. Fixes bug Needs to build on Linux. Needs testing. Needs clarity on STATE_TRANSIENT still.

Fixes bug 365683

Comment 5

[Aaron Leventhal]

Attachment: Fix OFFSCREEN/SHOWING and VISIBLE/INVISIBLE. Needs to built/tested on Linux. Still no clarity on STATE_TRANSIENT.

Comment 6

[Aaron Leventhal]

Attachment: Small tweaks. Still same status as before.

Comment 7

[Aaron Leventhal]

Attachment: Builds on Linux. Needs to be tested on Linux.

Comment 8

[Aaron Leventhal]

Basically works, but SHOWING is turning off when something scorlls off, for some reason.

Comment 9

[Aaron Leventhal]

Correction:
SHOWING state is correctly turned off when something scrolls off
VISIBLE state is incorrectly turned off when something scrolls off
SENSITIVE and ENABLED are incorrectly turned on for many items, simply because those items are not DISABLED
FOCUSABLE was incorrectly turned on for a list item down in the page at http://wordlist.sourceforge.net/

Comment 10

[Aaron Leventhal]

Attachment: Everything almost working, some problems with BUSY and MODAL

Comment 11

[Aaron Leventhal]

Attachment: Ready for review

I filed a follow-up bug for the STATE_BUSY problems (which are a regression) -- bug 367653.

Ginn, let me know if there's any code that could use more comments as well.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 252214 [details] [diff] [review]
Ready for review

>
>-  const unsigned long  EXT_STATE_EDITABLE       = 0x00200000;  // Used for XUL/HTML input (type = text,password) element

nit: it's better to say that is for object that implements nsIAccessibleEditableText because XUL hasn't input elements and these doesn't reflect XForms controls.

>+static void TranslateStates(PRUint32 aState, const AtkStateMap *aStateMap,
>+                            AtkStateSet *aStateSet)
>+{

>+  static void TranslateStates(PRUint32 aState, const AtkStateMap *aStateMap,
>+                              void *aAtkStateSet)
>+  {

Why do you need two versions of TranslateStates? It looks like AtkStateMap::TranslateStates isn't used at all.

Comment 13

[Aaron Leventhal]

Attachment: Correct version (nsStateMap.h in last patch was wrong)

Comment 14

[Aaron Leventhal]

The extra TranslateStates() was because of the bad nsStateMap I accidentally posted in the last patch.

Will fix STATE_EDITABLE comment.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 252339 [details] [diff] [review]
Correct version (nsStateMap.h in last patch was wrong)

Amazing work. I have only questions and few nits. I hope ginn will catch problems if I missed them. It's hard patch.

>+  const unsigned long  STATE_INVISIBLE          = 0x00008000;  // Programatically hidden

Does 'programatically hidden' makes difference with 'hidden'. For example, if some
elements is hidden via CSS then will accessible object be destroyed or its
accessible object will have this state?


>-  if (rectVisibility == nsRectVisibility_kVisible ||
>-      (rectVisibility == nsRectVisibility_kZeroAreaRect && frame->GetNextContinuation())) {
>+  if (rectVisibility == nsRectVisibility_kZeroAreaRect ) {
>+    if (frame->GetNextContinuation()) {
>+      // Zero area rects can occur in the first frame of a multi-frame text flow,
>+      // in which case the next frame exists because the text flow is visible
>+      rectVisibility = nsRectVisibility_kVisible;
>+    }
>+    else if (IsCorrectFrameType(frame, nsAccessibilityAtoms::inlineFrame)) {

nit: please line up 'else if' statement

>+  PRBool hasArea  = rectVisibility != nsRectVisibility_kZeroAreaRect;
>+  if (hasArea) {
>+    *aIsOffscreen = PR_TRUE;
>+  }
>+  else {

nit: the same

>+  PRUint32 state ;
>+  GetFinalState(&state);
>+  if (0 == (state & STATE_UNAVAILABLE)) {  // If not disabled
>+    // And if has at least 1 action or it is focusable, it can be ENABLED and SENSITIVE
>+    PRUint8 actions;
>+    GetNumActions(&actions);
>+    if (actions > 0 || (state & STATE_FOCUSABLE)) {

I assume xul:toolbarbutton is fousable but it is not in tab navigation order. Right?

>+    if (!isMultiLine && !(state & STATE_PROTECTED)) {
>+      // Support textbox type="autocomplete" or <autocomplete> parent
>+      if (content->AttrValueIs(kNameSpaceID_None, nsAccessibilityAtoms::type,
>+                               nsAccessibilityAtoms::autocomplete, eIgnoreCase) ||
>+          (mParent && Role(mParent) == ROLE_AUTOCOMPLETE)) {
>+        *aExtState |= EXT_STATE_SUPPORTS_AUTOCOMPLETION;

Why should parent have ROLE_AUTOCOMPLETE role?

Comment 16

[Aaron Leventhal]

(In reply to comment #15)
> Does 'programatically hidden' makes difference with 'hidden'. For example, if
> some
> elements is hidden via CSS then will accessible object be destroyed or its
> accessible object will have this state?
Usually we have no a11y object if it's programatically hidden, although we'll check the frame size if there is an object, and if it's got 0 area we'll use STATE_INVISIBLE. We also use it on caret and documents.

How do you like |else| and |else if| lined up? I think those are correct, at least for what I'm used to.

> 
> > I assume xul:toolbarbutton is fousable but it is not in tab navigation order.
Wow, that's right. I'm surprised it's marked FOCUSABLE, because you cannot click to focus it with the mouse. There is no way for the user to get the focus on a toolbar button. That's a bug, it should not say focusable.

For example, the URL bar 
> Why should parent have ROLE_AUTOCOMPLETE role?
Thanks for bringing this up, this code is wrong. In fact a XUL <textbox type="autocomplete"> ends up as an nsXULComboboxAccessible with an nsHTMLTextfieldAccessible child. Will post new patch with that fix.

Comment 17

[Aaron Leventhal]

Attachment: Fix autocomplete logic

I'd like either Ginn or Neil's feedback, but Ginn seems to be gone this month (working on Flash I believe).

Comment 18

[Aaron Leventhal]

Comment on attachment 252728 [details] [diff] [review]
Fix autocomplete logic

Still a mistake in the autocomplete logic.

Comment 19

[Aaron Leventhal]

Attachment: Fix autocomplete logic again -- tested, works

Comment 20

[[e-mail]]

Comment on attachment 252771 [details] [diff] [review]
Fix autocomplete logic again -- tested, works

Nothing obviously stands out here.

>+static void TranslateStates(PRUint32 aState, const AtkStateMap *aStateMap,
>+                            AtkStateSet *aStateSet)
>+{
>+  NS_ASSERTION(aStateSet, "Can't pass in null state set");
>+
>+  // Convert every state to an entry in AtkStateMap
>+  PRUint32 stateIndex = 0;
>+  PRUint32 bitMask = 1;
>+  while (aStateMap[stateIndex].stateMapEntryType != kNoSuchState) {
>+    if (aStateMap[stateIndex].atkState) {    // There's potentially an ATK state for this
>+      PRBool isStateOn = (aState & bitMask) != 0;
>+      if (aStateMap[stateIndex].stateMapEntryType == kMapOpposite) {
>+        isStateOn = !isStateOn;
>+      }
>+      if (isStateOn) {
>+        atk_state_set_add_state(aStateSet, aStateMap[stateIndex].atkState);
>+      }
>+    }
>+    // Map extended state
>+    bitMask <<= 1;
>+    ++ stateIndex;
>+  }
>+}
One trick that you might consider assuming you don't find it unreadable is this:
while (aStateMap->stateMapEntryType != kNoSuchState) {
  if (aStateMap->atkState) {
    // etc.
  }
  bitMask <<= 1;
  ++aStateMap;
}

Comment 21

[Aaron Leventhal]

Cool trick, but yeah I think it's less readable.

Comment 22

[Ginn Chen]

Comment on attachment 252771 [details] [diff] [review]
Fix autocomplete logic again -- tested, works

Good to me.

Comments,
1) code styles, personally I prefer "} else", and "} break;", it's suggested by
 http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual
It's not a big deal, though.

2)
+  if (GetChromeFlags() & nsIWebBrowserChrome::CHROME_WINDOW_RESIZE) {
+    *aState |= STATE_SIZEABLE | STATE_MOVEABLE;
+  }

Not sizeable doesn't mean not moveable.
Actually we didn't use moveable before.
Worth log a bug to followup?

Comment 23

[Aaron Leventhal]

Comment on attachment 252771 [details] [diff] [review]
Fix autocomplete logic again -- tested, works

Ginn, I don't know how to know when something is MOVEABLE, but it seems like all top level windows may be moveable, unless they are maximized. Have you ever seen a top level window that is not moveable?

Comment 24

[Aaron Leventhal]

> Actually we didn't use moveable before.
> Worth log a bug to followup?

If you mean that there is no ATK/AT-SPI state for this, I don't think should file a bug to do it.

I'm going to check this in as, if there's a title bar it's moveable.

If you think we need to do something different we can file a bug.