The problem here is the current logic in onStateChanged that handles
selection events. Namely (at line 3467):

        if event.type.startswith("object:state-changed:selected"):
            # If this selection state change is for the object which
            # currently has the locus of focus, and the last keyboard
            # event was Control-Space, then let the user know.
            # See bug #486908 for more details.
            #
            if isinstance(orca_state.lastInputEvent,
                          input_event.KeyboardEvent):
                keyString = orca_state.lastNonModifierKeyEvent.event_string
                mods = orca_state.lastInputEvent.modifiers
                isControlKey = mods & (1 << pyatspi.MODIFIER_CONTROL)
                if keyString != "Space" and isControlKey:
                    state = orca_state.locusOfFocus.getState()
                    if state.contains(pyatspi.STATE_FOCUSED):
                        if self.isSameObject(event.source,
                                             orca_state.locusOfFocus):
                            if event.detail1:
                                # Translators: this object is now selected.
                                # Let the user know this.
                                #
                                # ONLY TRANSLATE THE PART AFTER THE PIPE
                                # CHARACTER |
                                #
                                speech.speak(Q_("text|selected"),
                                             None, False)
                            else:
                                # Translators: this object is now unselected.
                                # Let the user know this.
                                #
                                # ONLY TRANSLATE THE PART AFTER THE PIPE
                                # CHARACTER |
                                #
                                speech.speak(Q_("text|unselected"),
                                             None, False)
                            return

Normally going from selected to unselected is done via Control-Space.
In the case of the regression test, we are arrowing down once to get
to the January table cell row, then arrowing down again to select it.

Therefore the test:

                if keyString != "Space" and isControlKey:

is failing.

This initial arrowing down case is inconsistent with what happens
when we arrow down to other rows further down in the table.

Personally I'd consider it a gail bug, but I suppose we can add in some
special logic in onStateChanged() to check if we are a table cell in a 
tree table and we've arrowed Down to row 0, and if so, speak the new
selection state.

Will, is this the approach you'd like me to take?

Thanks!

(In reply to comment #1)
> The problem here is the current logic in onStateChanged that handles
> selection events. Namely (at line 3467):

probably not related, but I find the comment and the code to be contradicting each other:

>             # If this selection state change is for the object which
>             # currently has the locus of focus, and the last keyboard
>             # event was Control-Space, then let the user know.
...
>                 if keyString != "Space" and isControlKey:

I might be tired and reading this wrong, but I'm guessing maybe we get lucky here because the keystring is typically a lower case "space" and not "Space" with a capital "S"?

> This initial arrowing down case is inconsistent with what happens
> when we arrow down to other rows further down in the table.
> 
> Personally I'd consider it a gail bug

It might be.  It also seems inconsistent with the List Store demo, though.  That is, the Tree Store demo takes two down arrows to select the first item.  The List Store demo takes only one.  So, it might be just a quirky GTK+-ism.

> but I suppose we can add in some
> special logic in onStateChanged() to check if we are a table cell in a 
> tree table and we've arrowed Down to row 0, and if so, speak the new
> selection state.

Seems reasonable, though you might also need to test it with List Store to see if it introduces any new verbosity when first down arrowing into its table.

Thanks!

Created attachment 106814 [details] [review]
Patch to change 'keyString != "Space"' to 'keyString == "space"'.

Good catch Will. Patch committed to SVN trunk.
Leaving bug open to implement the special case 
of having to Down arrow twice into the first row 
of the Tree Store demo (and also the List Store 
demo on Hardy).

Created attachment 107460 [details] [review]
Patch to fix the remaining problem.

I've adjusted the onStateChanged() method in default.py to check
for the following condition with "object:state-changed:selected"
events:
  
  * it's a focused table cell
  * the event source is the same object as the current locus of focus
  * the last key event was a Down arrow.

If it is, then the current [un]selected state is spoken.

This nicely fixes up regression test #2 for gtk-demo role_tree_table.py
but it now has the side-effect of speaking "unselected" as we move from
the "January" row to the "New Years Day" row (test #9). 

Note that when running Orca "manually" with the gtk-demo Tree Store demo, 
(either synchronously or asynchronously), this also happens, but the next 
text being spoken:

"SPEECH OUTPUT: ''",
"SPEECH OUTPUT: 'New Years Day Alex check box checked  Havoc check box checked  Tim check box checked  Owen check box checked  Dave check box not checked '",
"SPEECH OUTPUT: 'tree level 2'"

automatically interrupts the "SPEECH OUTPUT: 'unselected'", so it isn't
heard.

The patch adjusts tests test #9 to now include the "unselected" speech line.

Hopefully this is the right thing to do.

Please test.

(In reply to comment #4)
>   * it's a focused table cell

Makes sense.

>   * the event source is the same object as the current locus of focus

Makes sense.

>   * the last key event was a Down arrow.

This you cannot count on I'm afraid:

1. Move to the January row.
2. Press Control+Space to unselect it.
3. Press Up Arrow to select it.

Looking at the patch, you also check to see if the row is 0.  It might not be:

1. Move to any row other than the first.
2. Press Control+Space to unselect it.
3. Press Down Arrow (or Up Arrow) to select it.

We're not speaking anything in this case.

Created attachment 107477 [details] [review]
Revision #3

This seems to take into consideration Joanie's findings.
The nice thing here is that the "unselected" problem 
(see my previous comment) just goes away.

Confirmed.  This all works quite nicely.

I'm afraid I forgot about something when testing earlier:  If an item in a tree is unselected, you can also select it by pressing Space (i.e. without Control).  In this case we are not speaking "selected".  Sorry about that!

Created attachment 107484 [details] [review]
Revision #4.

Ugh! Meta-space will also select it too. I've adjusted the code to
only check if the keyEvent is a "space". Hopefully this now fixes it.

Created attachment 107491 [details] [review]
Revision #5.

Joanie found another variant. If you were on a table cell that was already
selected and you pressed "space", with the previous patch it spoke "unselected"
followed by "selected". Yes we were getting two events; the first to
unselect it and the second to select it.

Now it just speaks "selected". I suspect that's the best we can do.

 
> Now it just speaks "selected". I suspect that's the best we can do.

Yeah, probably.... In a way it's not all that bad.  If the user hits Space in an attempt to toggle the selection off, we're now a) saying something (which we weren't before) and b) giving the user a better understanding of what is taking place.  From the testing I've done thus far, I think this patch is good.

Thanks Joanie. Latest patch committed to SVN trunk.
 
Do we want to commit it to the gnome-2-22 branch as well?

Moving to "[pending]".

(In reply to comment #11)

> Do we want to commit it to the gnome-2-22 branch as well?

FWIW, I'm leaning towards "yes."  This started out as "...regression test #2 produces the wrong results."  But in the end we found out that there were a number of pretty basic cases where we weren't speaking the selection state when we should have been, and from what I've seen thus far your patch fixes all of those cases. I think that makes it worth committing to branch -- pending a few more tests:

This patch should not impact Gecko.py, but I'll run the FF regression tests in the A.M. to verify this.  It might be worth running the StarOffice tests as well... Of course, failures in those tests exist cand resolving them probably should be next on the regression test fix agenda.  But you could still check for diffs between runs....

What are you leaning towards?

(In reply to comment #11)
> Do we want to commit it to the gnome-2-22 branch as well?

I took a look and it looks fine to me.  If it fixes some gross problems (which is seems like it does) and it tests/pylints well, I say commit to gnome-2-22 as well.

Thanks!

thanks. Latest patch checked into the gnome-2-22 branch as well.
Closing as FIXED.