90182 - Active Accessibility: nsHTMLSelectAccessible is not correct for ListBoxes

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[John Gaunt (redfive)]

The code was written with comboboxes in mind and is totally wrong for listboxes.
A Seperate class needs to be written for listboxes.

In particular the child structure is different for list/combo boxes and the
traversal using the DOM/Frame tree walkers is broken for list boxes.

Comment 1

[John Gaunt (redfive)]

I am attaching a diff of existing files and 4 new files as attachements. The 4 
new files all go in mozilla/accessible/src. You will see references to them in 
the diff.

Basically, one file: nsHTMLSelectAccessible has been broken into 3: 
nsHTMLSelectAccessible, nsHTMLComboboxAccessible and nsHTMLListboxAccessible. 
Some of the classes defined in the old nsHTMLSelectAccessible have been moved to 
the nsHTMLComboboxAccessible file. 

A couple of small changes were made to layout code reflecting the change of 
object that gets created when GetAccessible is called on the respective frames.

Comment 2

[John Gaunt (redfive)]

Attachment: Diff of Accessible and layout/html/forms/src, implementing nsHTMLListboxAccessible

Comment 3

[John Gaunt (redfive)]

Attachment: nsHTMLComboboxAccessible.h

Comment 4

[John Gaunt (redfive)]

Attachment: nsHTMLComboboxAccessible.cpp

Comment 5

[John Gaunt (redfive)]

Attachment: nsHTMLListboxAccessible.h

Comment 6

[John Gaunt (redfive)]

Attachment: nsHTMLListboxAccessible.cpp

Comment 7

[John Gaunt (redfive)]

cc'ing aaronl for r= and waterson for sr=, since you reviewed our branch and are 
more familiar with our stuff.

Comment 8

[John Gaunt (redfive)]

ccing brendan to sr, since waterson is on vacation.

Comment 9

[Aaron Leventhal]

John,

Please do a diff from your mozilla directory's parent directory:
cvs diff -uN mozilla/accessible
That will catch all new files such as nsMenuListener*.* which wasn't included
here, and you only need to create 1 attachment.
Also, by doing it above (from the parent) of your top level directory, I can
apply the patch without makefile conflicts. You'd think large patches work even
if the diff was created from the mozilla directory, but they don't.

Comment 10

[Aaron Leventhal]

Attachment: Sample file has several kinds of lists/combo boxes - notice HTML doesn't have comboboxes with an editable field!

Comment 11

[Aaron Leventhal]

Hi John,

Some comments:
- In nsHTMLSelectAccessible.cpp IsCorrectFrame, the open curly brace starting
the method should be on the next line.
- Found something significant - and I ran across this yesterday while testing
with Dolphin. GetAccState needs to be implemented for all of these classes that
inherit from nsAccessible. See nsIAccessible.idl for a list of states that get
or'd together. For these list classes, we need to return STATE_READONLY |
STATE_SELECTABLE | STATE_FOCUSABLE. Some of these are also STATE_MULTISELECTABLE
| STATE_EXTSELECTABLE.  If one or more items are selected, use the
STATE_SELECTED flag. Combo boxes need STATE_HASPOPUP. Don't worry about
STATE_FOCUSED, that should be taken care of for you by nsAccessible.cpp. Sorry
to throw that on you suddenly - just noticed the problem.

Will keep looking for more stuff.
- Aaron

Comment 12

[John Gaunt (redfive)]

Attachment: nsMenuListener.h ( whoops missed 2 )

Comment 13

[John Gaunt (redfive)]

Attachment: nsMenuListenerAccessible.cpp ( whoops missed 2 )

Comment 14

[Aaron Leventhal]

John,

- In NS_IMETHODIMP nsHTMLSelectOptionAccessible::GetAccName, can you think of
*any* reason for us to use AppendFlatStringFromSubtree here? Select options are
text only, they can't use arbitrary HTML, at least they can't right now. It's
just extra work, but should be okay I suppose.

- The attachment to this bug nsMenuListener.h should be called
nsMenuListenerAccessible.h

Comment 15

[Aaron Leventhal]

- I noticed some whitespace problems in nsMenuListener.cpp - sometimes it has 2
space indents and sometimes 3 spaces (for the second level of indentation).
- Should we be calling SetupMenuListener() in the constuctor for
nsComboboxWindowAccessible, or in the constructor for nsMenuListener? It
attaches the event listener - right now we lazily add it in two places - I
suppose that's good.
- Should nsMenuListener be it's own class when it's only used for combo boxes?
Do you think we'll be reusing it somewhere else?

Comment 16

[Aaron Leventhal]

In nsMenuListener.cpp
     nsresult rv = eventReceiver->AddEventListener(NS_LITERAL_STRING("create"),
this, PR_TRUE);   

     if (NS_FAILED(rv)) {
       return;
     }

     mRegistered = PR_TRUE;
  }

Why not just:
if (NS_SUCCEEDED(eventReceiver->AddEventListener(NS_LITERAL_STRING("create"),
this, PR_TRUE))
  mRegistered = PR_TRUE;

Comment 17

[John Gaunt (redfive)]

adding dependancy on 90624, IsCorrectFrame is being moved to nsAccessible to 
accomodate the patch for that bug too. I will be removing it from 
nsHTMLSelectAccessible and posting a new patch. 

Brendan: can you sr= 90624 first? Thanks.

Comment 18

[Aaron Leventhal]

John when I run with this code I get an assertion
"Error non prescontext passed to accessible factory: 'c' -
nsAccessibilityService.cpp, line 144"

Comment 19

[John Gaunt (redfive)]

Attachment: Diff and new files in one. all preceeding diffs and files are invalid

Comment 20

[John Gaunt (redfive)]

Just posted a diff that also contains all the new files to be added. 
nsMenuListenerAccessible no longer exists and the file nsHTMLSelectAccessible 
has become nsHTMLSelectListAccessible; it's contents have changed.

I have addressed Aaron's concerns, except for the AppendFlatStringFromSubTree... 
method. It should be fine this way for now. I've also spoken to him about the 
assertion he was getting, it was due to an out of date tree.

Comment 21

[Aaron Leventhal]

John,

 if ( ! nsAccessible::IsCorrectFrameType( frame, nsLayoutAtoms::blockFrame) )
    return;

  frame->GetNextSibling(&frame);
  if ( ! nsAccessible::IsCorrectFrameType( frame,
nsLayoutAtoms::gfxButtonControlFrame) )
    return;

  frame->GetParent(aRelativeFrame);
  if ( ! nsAccessible::IsCorrectFrameType( *aRelativeFrame,
nsLayoutAtoms::areaFrame) )
    return;

I thought we were going to #ifdef DEBUG these, or change them into assertions?

- Aaron

Comment 22

[Aaron Leventhal]

John,

The state stuff looks pretty good - have you checked IE's handling of states for
these objects? My main question is whether MULTISELECTABLE and EXTSELECTABLE are
supposed to be on the list box or on the options themselves.

If you check into that for me, and get rid of the frame checks in opt builds,
r=aaronl

Comment 23

[John Gaunt (redfive)]

Added 

#ifdef DEBUG
  // code using IsCorrectFrameType()
#endif

for all the calls to IsCorrectFrameType in nsHTMLComboboxAccessible.cpp, the 
only place it gets called in my code.

IE doesn't report multi/ext selectable in their state.

Brendan? Your turn. :-)

Comment 24

[Brendan Eich [:brendan]]

I skimmed, started to whine about one-letter comptr-variable names (s, c) and
empty doc comments, but thought I'd really do better to review the whole bit
patch.  But I'm pressed for time, trying to land the FASTLOAD_20010703_BRANCH
and prepare for the O'Reilly Open Source conference next week, so I'm punting
you to waterson for sr=.

BTW, is it possible to do accessible work like this in a more incremental
fashion?  It's sure easier to review and digest.  I could be wrong, and it's
better to do big patches for this sort of change.  What do you think?

/be

Comment 25

[Brendan Eich [:brendan]]

s/whole bit patch/whole big patch/ -- what I meant was, I would not want to
whine about nits without really studying the code in context, and I'm afraid I
don't have time this week or next.  Waterson's your man.

/be

Comment 26

[John Gaunt (redfive)]

As to the big patch...

cvs diff -uN <blah>

is _not_ picking up the new files as is promised ( and as it has done in the 
past ). Which is why I have included the files in one big text file. I know it 
sucks, but at least you no longer have to look at all the different patches.

As to the single letter variables... they are legacies from evaughan and I 
didn't notice them when shuffling the code around. I'll go back and fix those 
since I'm creating new files anyway.

Comment 27

[timeless]

you need to cvs add new files before cvs diff -N will work

Comment 28

[John Gaunt (redfive)]

Attachment: Latest patch, with removal of 1 char vars and files 'cvs add'd

Comment 29

[Chris Waterson]

Style nit. Make the mainline case return NS_OK:

+  *_retval = new nsHTMLComboboxAccessible(aDOMNode, weakShell);
+  if (*_retval) {
+    NS_ADDREF(*_retval);
+    return NS_OK;
+  } 
+ 
+  return NS_ERROR_OUT_OF_MEMORY;

In other words,

   *_retval = /*whatever*/;
   if (! *_retval)
     return NS_ERROR_OUT_OF_MEMORY;

   NS_ADDREF(*_retval);
   return NS_OK;

Unfortunately, the kooky way is sprinkled througout the rest of the code, isn't
it? Who sr='d that? ;-) I don't propose you change it everywhere, but keep it in
mind and clean it up some day.

Why are you leaving this code in?

+  // XXX - jgaunt - are we using this content?
+  //nsCOMPtr<nsIContent> content (do_QueryInterface(aDOMNode));

(I guess if it compiled, you ain't using it!)

I'd prefer |!accService| here unless it breaks with local custom.

+  nsAccessibilityService* accService = new nsAccessibilityService();
+  if (accService == nsnull)
       return NS_ERROR_OUT_OF_MEMORY;


Ack! Space police!

+  *_retval = new nsHTMLComboboxWindowAccessible(this, mDOMNode, mPresShell);
+  if ( ! *_retval )

You're inconsistent with your addition of spaces around conditions.

In nsHTMLComboboxButtonAccessible::AccDoAction(PRUint8 index), what's the magic
value ``zero'' mean? Comment, please!

+  if (index == 0) {

Meta note here. Why aren't you guys using enums, or at least preprocessor
mnemonics for this stuff? Wouldn't this be easier on the eyes?

   if (index == eAction_Click)

Then I wouldn't have to wail about the magic numbers (albeit, only zero) that
exist in all your DoAction() methods.

Didn't we decide that presContext was going to be null sometimes? You don't
check it here (and elsewhere...)

+void nsHTMLComboboxButtonAccessible::GetBounds(nsRect& aBounds, nsIFrame**
aRelativeFrame)
+{
+  // get our second child's frame
+  nsIFrame* frame = nsAccessible::GetBoundsFrame();
+  nsCOMPtr<nsIPresContext> context;
+  GetPresContext(context);

Waah! More magic numbers.

+NS_IMETHODIMP nsHTMLComboboxButtonAccessible::GetAccName(nsAWritableString&
_retval)
+{
+  return GetAccActionName(0, _retval);
+}

Don't use nsCOMPtr's on nsIFrame objects:

+  PRBool isOpen = PR_FALSE;
+  nsIFrame *boundsFrame = GetBoundsFrame();
+  nsCOMPtr<nsIComboboxControlFrame> comboFrame (do_QueryInterface(boundsFrame));
+  comboFrame->IsDroppedDown(&isOpen);

Don't you want NS_ERROR_OUT_OF_MEMORY?

+  *_retval = new nsHTMLComboboxWindowAccessible(parent, mDOMNode, mPresShell);
+  if ( ! *_retval )
+    return NS_ERROR_FAILURE;

Should member variables be protected in nsHTMLComboboxAccessible,
nsComboboxTextFieldAccessible, ...? For example,

+
+  PRBool mRegistered;
+  PRBool mOpen;

Comment 30

[John Gaunt (redfive)]

Ok, I've taken care of everything but this:

Don't use nsCOMPtr's on nsIFrame objects:

+  PRBool isOpen = PR_FALSE;
+  nsIFrame *boundsFrame = GetBoundsFrame();
+  nsCOMPtr<nsIComboboxControlFrame> comboFrame(do_QueryInterface(boundsFrame));
+  comboFrame->IsDroppedDown(&isOpen);

IsDroppedDown () is a memeber of nsIComboboxControlFrame. Should I use the "old" 
QI call to get at the ComboboxControlFrame? NS_STATIC_CAST? Someone on irc said 
that I could QI a frame and it wouldn't mess things up. So I guess I was 
supposed to use the old/standard way and just put it in a 
nsIComboboxControlFrame* ???

I'm going to test the latest patch on linux and wait for your response(or 
someone's) on the QI before posting the new diff.

Comment 31

[John Gaunt (redfive)]

Attachment: v4.0 of patch, all sr issues (should be) fixed

Comment 32

[Chris Waterson]

Yes, use QueryInterface() the old way, but don't NS_RELEASE() the resulting 
pointer. For example,

  nsIFrame* frame = /* get this somehow */;
  nsIFooFrame* fooframe = nsnull;
  frame->QueryInterface(NS_GET_IID(nsIFooFrame), (void**) &fooFrame);
  if (fooFrame) {
    /* etc. */
  }

Comment 33

[John Gaunt (redfive)]

cool, that jives with what I found/figured out. :-) So the patch attached ( v4.0
) has that as well as fixes for the rest of the stuff. Let me know when you've
gone through it.

Comment 34

[Chris Waterson]

sr=waterson if you've fixed everything. thanks!

Comment 35

[Mike Pinkerton (not reading bugmail)]

with the addition of 2 files to accessible.mcp, this builds and runs on mac (tho 
not sure how to test it). john, i'll land the .mcp file when you tell me.

Comment 36

[John Gaunt (redfive)]

checked in fri july 20

Comment 37

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!