77648 - Active Accessibility: Implement the get_AccSelection() method for selects

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Peter Trudelle]

1) Inform MSAA client of selection events, P1/2days
Inform MSAA client that selection has happened. There are at least 3 kinds of
selection events: selection remove, selection add and selection within. Might be
completely different for text, combo boxes etc. We don't know how to tell screen
readers about selection within textfields and textareas. That's scary. 

2) Receive  & perform MSAA client generated selection tasks, P2,4days
Sometimes the MSAA client such as screen reader will tell us what needs to be
selected. Might be implemented totally differently for each widget we support,
such as combo boxes, text and textfields.

Comment 1

[Peter Trudelle]

Doh! changing summary to *selection* support.

Comment 2

[rods (gone)]

If you want, you can open a bug for just extneded selection for selects, but the 
rest is eric's

Comment 3

[Peter Trudelle]

Rod you are not responsible for selection in text or text fields. We are making
another bug for that. But add selection, remove selection, extend selection, all
have to do with list and combo boxes.

Comment 4

[Aaron Leventhal]

John has agreed to handle combo boxes and lists now.
This is part of that work.

Comment 5

[John Gaunt (redfive)]

I know how to get seletion events from textfields and textareas, I also know how
to catch events that tells us the selection of a combo box or list box changed,
but not sure if we need specifics about what kind of change or if just saying "
Hey! It changed!" is enough. Maybe that is short term vs. long term

As to recieving and performing the MSAA selection tasks, haven't even looked at
things moving in that direction yet.

I may break this up into 2 bugs since the two efforts seem drastically differeent.

Comment 6

[John Gaunt (redfive)]

Since we don't have accessibles for selects yet, this work is stalled.

Comment 7

[John Gaunt (redfive)]

nsHTMLSelects have been implemented, now we need to fire selection events for
selection add,remove,within and apparently also an event for the popup of
comboboxes.

I have also noticed some problems with the state change events coming out of
listboxes and comboboxes ( the widgets covered by nsHTMLSelect ) and the
reporting of the value of listboxes ( also there is no focus ring for the
listboxes -- just for the comboboxes ).

Comment 8

[John Gaunt (redfive)]

Attachment: initial patch to enable listboxes to create accessible objects

Comment 9

[John Gaunt (redfive)]

Ok, here's the deal. There are two cases for selects: 1) one option selected 2) 
multiple options selected. Likewise there is a variable that determines the 
ability for a select to have multiple selections. This variable only is used for 
list boxes since comboboxes only have a display area for a single selection 
anyway. ( 'least that's how I think it works :-)  )

The tenative solution is to have comboboxes return the selected value when a 
call is made to get_AccValue() and not return anything for get_AccSelection()
Listboxes, which can have multiple selections will return nothing for 
get_AccValue() and the list of selected objects in response to a 
get_AccSelection() call. Hmm... maybe returning the first selected object to the 
get_AccValue() call would be better...

regardless, the tought part is figuring out how to return the array of objects 
to the getSelection call. Some variant structure thingy....

Comment 10

[Aaron Leventhal]

Why not just always use get_accSelection for the selection, and never use 
get_accValue? Isn't that what get_accSelection is for? Just use an array size 
<=1 if there's only 0 or 1 item.

Comment 11

[John Gaunt (redfive)]

Attachment: full diff v1.0

Comment 12

[John Gaunt (redfive)]

hmmm... just got the smackdown on the DOM interface changes. I'll have to get
with jst about a better way to get the selected options. In the meantime I'm
pulling this out of the branch.

Comment 13

[John Gaunt (redfive)]

This is currently covering 2 bugs, the get_AccSelection() method that needs to
be implemented and the OBJ_EVENT_SELECTION event that needs to get fired. The
second has been fixed, so I will create a new bug and add the diff to that and
repost a diff here that contains only that code proposed for this bug. :-)

Comment 14

[John Gaunt (redfive)]

changing summary to better represent what this bug covers. Another bug has been
filed for covering the notification to MSAA of selection events happening (bug
86520). 

To expand: get_AccSelection should return Accessible objects ( IAccessible ) for
each of a select's ( combobox/listbox ) selected children ( nsHTMLOptionElements ).

Comment 15

[John Gaunt (redfive)]

Attachment: v3.0 of the fix. return using IEnumVARIANT

Comment 16

[John Gaunt (redfive)]

Finally got the IEnumVARIANT problem ironed out, thanks to a pointer to a
webpage from dbradley.

aaronl,
ready for review

Comment 17

[Aaron Leventhal]

John,
In nsHTMLListboxAccessible::GetSelectedChildren
- Can you use a better variable name for the array of selected children than
"access". How about "selectedChildren". Also, instead of tempAccess, how about
selectedChild?
- How about moving GetPresContext outside the loop?

In Accessible::get_accSelection
- We don't need the extra CoInitialize(NULL) - that's done at program startup.
I've been told to take out extra calls to it - it's a slow win32 API call.
- Same comment as above re: variable name tempAccess
- Since you're assigning to optionArray[i], shouldn't the declaration for
optionArray be CComVariant[MAXLENGTH] optionArray;
Will assigning optionArray[i], where i>0 cause a protection fault?
- Don't you want to fall down to the VT_EMPTY part of the method, when there are
0 options?
- Suggestion - perhaps some quick commentary at the beginning of the method as
to what an IEnumVariant method is? A birds eye view of what's going on here. 99%
of people reading this won't understand what this is about.
- Is it possible for an IEnumVariant to fail when being QI'd to an IUnknown? I
suppose better safe than sorry eh? Paranoid programming at it's best. That's
like saying an nsIAccessible might not be an ISupports, and checking each time
you QI'd that, isn't it?
- Don't you do a VariantInit(pvarChildren) no matter what? Why not just get it
out of the way at the beginning of the method, and do it only once.

Comment 18

[John Gaunt (redfive)]

Attachment: v3.1 reviewer's fixes and some cleanup

Comment 19

[John Gaunt (redfive)]

I have taken care of those things I felt were problems. I left tempAccess in and
changed the names of other variables in the loops to show they are temporary (
tempOption, tempNode etc.. )

the CComObject * = new CComObject[length] should be just fine. Although it was
leaking, so I fixed that. ;-)

Factored some repetitive stuff towards the top in the Accessible.cpp code, the
InitVariant and the assignment of VT_EMPTY. 

Fixed the logic for the case where there are 0 elements selected, the
nsIListboxSelect will now return a nsnull nsISupportsArray in that case.

Added some detailed comments.

Aaron will be checking this again this afternoon, but I will request sr= now and
start the process. :-)

Comment 20

[Aaron Leventhal]

r=aaronl

Forgive me for missing the optionArray = new CComVariant[length]
You had it right from the beginning.

<nit>
However, why don't you declar and define it all in one line?
CComVariant* optionArray = new CComVariant[length]
</nit>

Comment 21

[Brendan Eich [:brendan]]

I agree with aaronl on optionArray being declared and initialized at once, after
length has been got.

Style nit: if you're going to return early when !pUnk, why test and do that,
rather than

    stuff...
    QI pUnk;
    delete optionArray;
    if (pUnk) {
      more stuff that uses pUnk;
    } else {
      return NS_ERROR_FAILURE;
    }
    stuff that follows more stuff;

Let the common control flow go through code that's indented more at the same level:

    stuff...
    QI pUnk;
    delete optionArray;
    if (!pUnk) {
      return NS_ERROR_FAILURE;
    }
    more stuff that uses pUnk;
    stuff that follows more stuff;

Other nits:

- Set isSelected to PR_FALSE once when initializing, then only if it comes back
true from GetSelected (don't set it to false every time thru the loop, IOW).

- The // end if (tempAccess) comments just add visual noise, at least for this
reader.  In a big, pages-long method body, maybe -- but are they really needed
here?  My 2 cents, take 'em or leave 'em.

Pick these ('cept the last, your and aaronl's taste decides) and
sr=[e-mail]

/be

Comment 22

[John Gaunt (redfive)]

Attachment: v3.2 of fix, including sr= fixes, it's going in

Comment 23

[John Gaunt (redfive)]

patch 3.2 went in monday 6 aug

Comment 24

[Scott MacGregor]

This broke my windows build! Does anyone else see this? My build now breaks in
Accessible.cpp complaining about atbase.h not being found. 

Comment 25

[John Gaunt (redfive)]

Hmmm... tell me about your system. VC++ version etc... These files ship with 
VC++ (I'm using 6.0?) so they should be there. Have you checked your environment 
to see if the files are on your path? Include path etc...? Have you searched for 
those files to see if they are actually on your machine and are just not being 
found?

Comment 26

[Scott MacGregor]

I'm running VC 6.0. No the files aren't installed on my machine at all.
Searching my hard drive didn't show a match.

isn't atlbase.h part of activeX support? This isn't listed as a requirement by
the mozilla.org build instructions for mozilla. As a result many of us don't
install VC6.0 with the active x controls which probably would have included this
file. 

If this is now a requirement then I think we need to be posting to the builds
group, and making changes to the windows build instructions on www.mozilla.org.
Also some advanced notice would have been cool =).

Comment 27

[John Gaunt (redfive)]

adding some cc's to get more eyes on this, not sure where to go on this. We need 
some MS COM stuff for the MSAA work ( for IEnumVARAINT ), but I'm not really 
keen on adding dependancy on all of ActiveX. This problem didn't pop up for me 
because I just installed everything in the VC++ dist.

also reopening since there is at least one issue still needing resolution -- do 
we increase the requirements to build on Win or do we find a different way of 
fixing this bug?

Comment 28

[timeless]

adding dependencies for mfc/atl should only be done when you really really need 
it. doing this for browser should usually be wrong. It's ok for mfcEmbed but 
that's because mfcEmbed is an mfc animal by nature.

fwiw, ATL is Active Template Library. 
http://support.microsoft.com/support/kb/articles/Q166/4/80.asp

Can someone please list the entries from atl that are being used, and the 
reasons for their use?

Oh to answer your last question: You may not increase the requirements. You may 
decrease them by removing accessibility from the default build.

ATL/MFC are huge [I ran out of space trying to setup a basic .net b2 compiler 
because i foolishly selected the atl dir]

the .net b2 dvd has IEnumVariant defined in oaidl.h.

MS's license should allow you to make a copy of the relevant definitions, 
lumped into a single header (just the ms defs) for use here.  Of course you'll 
probably need some defs from atliface.h (IAccessibleServer ...)

You may of course back out as much of Accessibilty as is necessary at any time 
to make the normal NON MFC/ATL reliant builds happy.

I seem to remember being given assurances that accessible would not be built by 
default if it relied on MFC/ATL -- WHAT HAPPENED?

Comment 29

[John Gaunt (redfive)]

There are some template classes I have used to get an IEnumVARIANT to return to
the MS Active Accessibility library. Everything I turned up in my research
pointed to using the ATL to get an IEnumVARIANT.

I'm working on a switch to disable the building of Accessibility for a short
term fix. 

Comment 30

[John Gaunt (redfive)]

I am closing this bug and have filed another to track the progress of the
accessibility switch: see bug 94768 for further discussion.

Comment 31

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381