Created attachment 91552 [details] [review]
A first "feel the water" patch.

Before I take this too much further, I want to make sure I'm
on the right track. If we are doing "speak table cell row"
and we move to a new row, then for each table cell that has
a "toggle" action, we precede the speaking of that table 
cell with speaking the table column header.

This patch just uses self._getSpeechForTableColumnHeader()
to get what should be spoken for the table column header.
Therefore it's also speaking the rolename ("column
header") by default. Do we want that?

I've removed the non-visible text cell renderers from the
text attribute pane in the Orca preference dialog, so that's
one test case to try. The other is the Tree Store sub-demo
in gtk-demo. Notice that it speaks the hidden checkboxes on
lines like "February". What should we be checking so that those
hidden checkboxes aren't spoken? Doesn't look like "VISIBLE" 
state will work. If I'm on the "February" row and in the "Tim" 
column, here's the state of that table cell:

          +-name=None role='table cell' state='ACTIVE ENABLED FOCUSABLE SELECTABLE SELECTED SENSITIVE SHOWING TRANSIENT VISIBLE' relations=''

Note that I haven't done the matching braillegenerator changes yet.
I wanted to make sure we had consensus on the speech changes first.

Thanks.

> Before I take this too much further, I want to make sure I'm
> on the right track. If we are doing "speak table cell row"
> and we move to a new row, then for each table cell that has
> a "toggle" action, we precede the speaking of that table 
> cell with speaking the table column header.

This seems close, except that it seems like it might be too verbose for cells that have labelled checkboxes.  So, you might want to check to see if there is any text associated with the checkbox before inserting the column header information.

> This patch just uses self._getSpeechForTableColumnHeader()
> to get what should be spoken for the table column header.
> Therefore it's also speaking the rolename ("column
> header") by default. Do we want that?

I don't think we want 'column header' in there, but Mike's the better one to answer that.

> in gtk-demo. Notice that it speaks the hidden checkboxes on
> lines like "February". What should we be checking so that those
> hidden checkboxes aren't spoken? Doesn't look like "VISIBLE" 
> state will work. If I'm on the "February" row and in the "Tim" 
> column, here's the state of that table cell:
> 
>           +-name=None role='table cell' state='ACTIVE ENABLED FOCUSABLE
> SELECTABLE SELECTED SENSITIVE SHOWING TRANSIENT VISIBLE' relations=''

Bummer.  I'm not sure what we can do for those.  I guess presenting them is all we can do for now.  :-(

> 
> > This patch just uses self._getSpeechForTableColumnHeader()
> > to get what should be spoken for the table column header.
> > Therefore it's also speaking the rolename ("column
> > header") by default. Do we want that?
> 
> I don't think we want 'column header' in there, but Mike's the better one to
> answer that.
Will is correct here.

Created attachment 91567 [details] [review]
Revised patch (speech only at this point).

Incorporates feedback from Will and Mike (thanks).
If this is what we want, then please let me know and
I'll go ahead and implement the equivalent on the 
braille side.

Created attachment 91575 [details] [review]
Further revised patch (includes braille support).

Please test.

The special code in the Evolution script for handling checkbox
table cells in the mail message header list at the moment does
the following:

       # Check if the current table cell is a check box. If it
       # is, then to reduce verbosity, only speak and braille it,
       # if it's checked or if we are moving the focus from to the
       # left or right on the same row.
       #
       # The one exception to the above is if this is for the
       # Status checkbox, in which case we speak/braille it if
       # the message is unread (not checked).

This also means that we speak "Read" instead of the column header
(which is "Status").

In short, I don't think there is any advantage in taking it out and
using the new "default" table cell column header code. It will
be a lot more verbose and use will also lose the Status==>Read.

I suggest we keep the special Evolution scripting code in "as is".

> I suggest we keep the special Evolution scripting code in "as is".

Sounds good.  Thanks for looking into it.

I've just applied this patch and restarted orca.  When I arrow down the rows in the treestore demo I just hear "checkbox checked, checkbox checked" and so on.  When arrowing down by row I would also expect to hear the name before the checkbox.

Are you "reading by cell" or "reading by row"? From Will's initial
bug description:

"Note that when navigating up/down by row when read table cell row is not
enabled, normal contextual information should be presented.  Thus, when
navigating up/down, only the new row header (if it exists) should be presented,
and when navigating left/right, only the new column header (if it exists)
should be presented."

If I have "reading by row" enabled, it speaks/brailles the whole row for me.
If I have "reading by cell" enabled, and I'm just moving up and down, then it
justs speaks/brailles the current cell (no column header).

I understood this to be the desired behavior...

Yeah, yeah, yeah. I should be resting up. But the pain's minimal (probably
the pain killers having worn off yet). I'm just feeling woozy. Hopefully 
that's not having an effect on this comment.

I just re-tested this and the speech side seems to work great.  The only thing I'd like to see changed is to put the label on the braille display after the actual check box.  Currently it is before.  This will be consistant with our other implementation of checkboxes in dialog boxes.  

Created attachment 91701 [details] [review]
Further revised patch based on feedback from Mike.

Hopefully the checkbox and labels are now in reverse order.

This now seems to be working nicely when panning across the row.  The problem is that when arrowing across the columns the labels are still before the checkbox.  I'm thinking that the labels should be after the checkboxes in this case as well.

> This now seems to be working nicely when panning across the row.  The problem
> is that when arrowing across the columns the labels are still before the
> checkbox.  I'm thinking that the labels should be after the checkboxes in this
> case as well.

This is caused by the code in braillegenerator._getBrailleRegionsForTableCell()
at about line 1143:

        else:
            [cellRegions, focusRegion] = self._getDefaultBrailleRegions(\
                self._script.getRealActiveDescendant(obj))
            regions[0].extend(cellRegions)

The _getDefaultBrailleRegions() method is the fallback braille generator 
should no other specialized braille generator exist for this object. It
handles the labels and roles in the opposite order to what we now have
in the table cell handling code.

I suspect reversing the order we do things in that routine will 
adversely affect the brailling of a lot of other objects.

Will, what is the best way to fix this?

Will, worked out what was going wrong. I needed to do the text for
a checkboxed table cell with no label in _getBrailleRegionsForTableCell()
not in _getBrailleRegionsForTableCellRow(). New patch top follow.

Created attachment 91742 [details] [review]
Revised version of the patch.

Mike, hopefully this will fixup your last concern (and still keep
all the original functionality).

Now I think this patch looks great.  It really is nicer in several places than what we had before.  thanks much for doing it. 

Thanks Mike. Patch committed. Putting the bug in the "[pending]" state.

looks good 

Closing as fixed.