Closed Bug 91716 Opened 23 years ago Closed 23 years ago

Active Accessibility: change is not reported when selection changes in combobox/listbox

Categories

(Core :: XUL, defect, P4)

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: mozilla, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(5 files)

ListBoxes and ComboBoxes report the first item selected in response to the 
get_AccValue() call. But when the selection is changed an automatic response is 
not shown in the inspect tool.

My guess is the correct event is not getting fired to notify the inspect tool 
that a change has happend. This could be: state_change selection, selection_add, 
selection_remove, selection_within... not sure.
Blocks: 90179
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Priority: P3 → P4
A focus bug is also not fired for the options as the user arrows through them,
which I now believe is also part of this bug.

I believe throwing a focus event will take care of this bug.

I'm about half way through a fix for this one.
Assignee: jgaunt → aaronl
Status: ASSIGNED → NEW
Keywords: access, fcc508
Whiteboard: Looking for r=/sr=
Status: NEW → ASSIGNED
So what I gather from the patch is that we aren't receiving a focus event when
an option is selected, but we do recieve a selection event. So we fire a
selection and a focus event when we see the selection of an option change.

This patch also seem to change the behavior of initial focus of a listbox.
Instead of the listbox getting the focus the options does.

I like the comments you have changed and the comments you left in.

in nsHTMLSelectOptionAccessible::GetFocusedOptionNode doesnt' the following call
just return mSelectedOption which is the first selected item anyway? What
happens if there is more than one selected item?

+  // Get what's focused by asking frame for "selected item". 
+  // Don't use DOM method of getting selected item, which instead gives *first*
selected option 
+  PRInt32 focusedOptionIndex = 0;
+  listFrame->GetSelectedIndex(&focusedOptionIndex);  

In the following code, if options exists and the focused option is greater than
or equal to 0 *nothing* is selected??? Is this comment right?

+  if (options && focusedOptionIndex >= 0)   // Nothing is selected, return
normal target content
+    options->Item(focusedOptionIndex, getter_AddRefs(aFocusedOptionNode));

in nsRootAccessible::HandleEvent do we really want to return NS_OK if we fail?

+    if (NS_FAILED(GetTargetNodes(aEvent, targetNode, optionTargetNode)))
       return NS_OK;

I'm unable to apply this patch right now since I'm in the middle of building. I
will apply it and test ASAP and reply back here with any new questions or
resolution of the questions I have stated already.

Overall I like and approve of the changes. I just want to test out some of the
logic.
John,

1. GetSelectedIndex for content nodes returns first selected item
   GetSelectedIndex for frames returns focused item 
   I'm using GetSelectedIndex for frames here, so this is indeed correct.

2. When index < 0, that means no option is focused
   I've fixed the comments and code around that, and tested it.
   Therefore it now returns the <select> domnode when 
     there are no options or none are selected.

3. Improper return code fixed
I've run the new patch, looks good. Changes to the patch are great.

Sorry it took so long, found a regression (?) in the listbox code that is
returning the wrong text when the list box is tabbed to with no options selected
( it is reporting the first element as being selected )  but it doesn't look
like it is due to this code, maybe only exposed by this code. :-)

r=jgaunt
Whiteboard: Looking for r=/sr= → Looking for sr=
QA Contact: aegis → dsirnapalli
Nits only, I can't see bugs at this point, so won't hold up checkin:

+      nsCOMPtr<nsIDOMNode> parentNode;

+      aNode->GetParentNode(getter_AddRefs(parentNode));

+      nsCOMPtr<nsIAccessible> parentAccessible;

+      if (parentNode)

+        GetAccessibleFor(parentNode, getter_AddRefs(parentAccessible));

+      if (parentAccessible) {

+        nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(shell));

+        newAcc = new nsHTMLSelectOptionAccessible(parentAccessible, aNode,
weakShell);

+      }

might better be written as

+      nsCOMPtr<nsIDOMNode> parentNode;

+      aNode->GetParentNode(getter_AddRefs(parentNode));

+      if (parentNode)
 {
+        nsCOMPtr<nsIAccessible> parentAccessible;
+        GetAccessibleFor(parentNode, getter_AddRefs(parentAccessible));

+        if (parentAccessible) {

+          nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(shell));

+          newAcc = new nsHTMLSelectOptionAccessible(parentAccessible, aNode,
weakShell);

+        }
+      }

The data and control flow are clearer, the scopes more minimal, and there isn't
a doomed parentAccessible test in the case where parentNode is null.(hardly a
runtime cycle concern, just aesthetics), it seems to me.

I don't like nsCOMPtr<T>& argument types, they introduce yet another pattern.
They do relieve you from having to declare ** out params and call NS_ADDREF, on
the other hand.  What do you think?  Since this is in a concrete class's
internal method, I can't say "change it", but please don't spread it outside
your private/protected methods, ok?  Your comments welcome.

sr=brendan@mozilla.org with nits picked as you wish.

/be
Final patch coming next.

Took brendan's advice on first issue.
In the future I will also avoid nsCOMPtr<T>& because:
a) code is more inspectable when (*foo = bar) clearly shows a side effect
b) shouldn't introduce extra patters that people don't knowc) scriptability 
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Blocks: 95819
Summary: change is not reported in msaa when selection changes in combobox/listbox → Active Accessibility: change is not reported when selection changes in combobox/listbox
Whiteboard: Looking for sr=
-- Verified in the latest mozilla nightly build. Works fine. Marking the bug as 
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: