Closed Bug 367412 (statesa11y) Opened 15 years ago Closed 15 years ago

Fix Mozilla's usage of states, and refactor

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(1 file, 10 obsolete files)

57.82 KB, patch
ginnchen+exoracle
: review+
Details | Diff | Splinter Review
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.
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
Attachment #251964 - Attachment is obsolete: true
Alias: statesa11y
Attachment #252017 - Attachment is obsolete: true
Attachment #252070 - Attachment is obsolete: true
Attachment #252084 - Attachment is obsolete: true
Attachment #252087 - Attachment is obsolete: true
Basically works, but SHOWING is turning off when something scorlls off, for some reason.
Summary: Fix Mozilla's usage of extended states → Fix Mozilla's usage of states, and refactor
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/
Attached patch Ready for review (obsolete) — Splinter 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.
Attachment #252104 - Attachment is obsolete: true
Attachment #252194 - Attachment is obsolete: true
Attachment #252214 - Flags: review?(ginn.chen)
Attachment #252214 - Flags: review?(surkov.alexander)
Blocks: 366340
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.
Attachment #252339 - Flags: review?(surkov.alexander)
Attachment #252214 - Attachment is obsolete: true
Attachment #252214 - Flags: review?(surkov.alexander)
Attachment #252214 - Flags: review?(ginn.chen)
Attachment #252339 - Flags: review?(ginn.chen)
The extra TranslateStates() was because of the bad nsStateMap I accidentally posted in the last patch.

Will fix STATE_EDITABLE comment.
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?
Attachment #252339 - Flags: review?(surkov.alexander) → review+
(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.

Attached patch Fix autocomplete logic (obsolete) — Splinter Review
I'd like either Ginn or Neil's feedback, but Ginn seems to be gone this month (working on Flash I believe).
Attachment #252339 - Attachment is obsolete: true
Attachment #252728 - Flags: superreview?(neil)
Attachment #252728 - Flags: review?(ginn.chen)
Attachment #252339 - Flags: review?(ginn.chen)
Comment on attachment 252728 [details] [diff] [review]
Fix autocomplete logic

Still a mistake in the autocomplete logic.
Attachment #252728 - Attachment is obsolete: true
Attachment #252728 - Flags: superreview?(neil)
Attachment #252728 - Flags: review?(ginn.chen)
Attachment #252771 - Flags: superreview?(neil)
Attachment #252771 - Flags: review?(ginn.chen)
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;
}
Attachment #252771 - Flags: superreview?(neil) → superreview+
Cool trick, but yeah I think it's less readable.
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?
Attachment #252771 - Flags: review?(ginn.chen) → review+
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?
> 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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 966910
Depends on: 989958
Depends on: 997752
You need to log in before you can comment on or make changes to this bug.