Active Accessibility: Implement the get_AccSelection() method for selects

RESOLVED FIXED in mozilla0.9.4

Status

()

defect
P2
normal
RESOLVED FIXED
18 years ago
18 days ago

People

(Reporter: trudelle, Assigned: mozilla)

Tracking

({access})

Trunk
mozilla0.9.4
x86
Windows 98
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Reporter

Description

18 years ago
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.
Reporter

Updated

18 years ago
Blocks: 75785
Whiteboard: ETA mm/dd???, needed for embedding 0.9
Reporter

Comment 1

18 years ago
Doh! changing summary to *selection* support.
Summary: Finish event support for MSAA in Gecko → Finish selection support for MSAA in Gecko

Comment 2

18 years ago
If you want, you can open a bug for just extneded selection for selects, but the 
rest is eric's
Assignee: rods → evaughan
Reporter

Comment 3

18 years ago
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.
Assignee: evaughan → rods
Target Milestone: --- → mozilla0.9.1

Updated

18 years ago
Status: NEW → ASSIGNED
Summary: Finish selection support for MSAA in Gecko → Finish selection for HTML selects (for MSAA in Gecko)

Comment 4

18 years ago
John has agreed to handle combo boxes and lists now.
This is part of that work.
Assignee: rods → jgaunt
Status: ASSIGNED → NEW
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Assignee

Updated

18 years ago
Whiteboard: ETA mm/dd???, needed for embedding 0.9 → 0.9.2
Reporter

Updated

18 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee

Comment 5

18 years ago
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.
Assignee

Comment 6

18 years ago
Since we don't have accessibles for selects yet, this work is stalled.
Severity: enhancement → normal
Depends on: 80992
Reporter

Updated

18 years ago
Keywords: access
Assignee

Comment 7

18 years ago
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 ).
Blocks: 77647
Reporter

Updated

18 years ago
Priority: -- → P2
Assignee

Comment 9

18 years ago
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

18 years ago
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.
Assignee

Updated

18 years ago
Depends on: 85743
Assignee

Comment 11

18 years ago
Assignee

Comment 12

18 years ago
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.
Assignee

Updated

18 years ago
Blocks: 86517
Assignee

Comment 13

18 years ago
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. :-)

Updated

18 years ago
No longer blocks: 86517
Assignee

Comment 14

18 years ago
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 ).
Summary: Finish selection for HTML selects (for MSAA in Gecko) → Implement the get_AccSelection() method for selects
Assignee

Updated

18 years ago
Whiteboard: 0.9.2 → IEnumVARIANT problems
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Assignee

Updated

18 years ago
Blocks: 90179
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Assignee

Comment 16

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

aaronl,
ready for review
Keywords: review
Whiteboard: IEnumVARIANT problems → needs r=/sr=

Comment 17

18 years ago
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.




Assignee

Comment 19

18 years ago
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

18 years ago
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>
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=brendan@mozilla.org

/be
Assignee

Comment 23

18 years ago
patch 3.2 went in monday 6 aug
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Keywords: review
Resolution: --- → FIXED
Whiteboard: needs r=/sr=

Comment 24

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

Comment 25

18 years ago
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

18 years ago
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 =).
Assignee

Comment 27

18 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 28

18 years ago
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?
Assignee

Comment 29

18 years ago
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. 
Assignee

Comment 30

18 years ago
I am closing this bug and have filed another to track the progress of the
accessibility switch: see bug 94768 for further discussion.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
Assignee

Updated

18 years ago
Blocks: 95819

Updated

18 years ago
Keywords: fcc508
Summary: Implement the get_AccSelection() method for selects → Active Accessibility: Implement the get_AccSelection() method for selects
You need to log in before you can comment on or make changes to this bug.