365866 - Table cells do not provide appropriate item state information

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lynn Monsanto]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20060601 Firefox/2.0.0.1 (Ubuntu-edgy)
Build Identifier: version 3 alpha 1 (20070103)

The AccessibleState for table cells does not indicate whether the state is checked or not. For example, when a message in the message header table has an attachment,  the AccessibleState for the "attachment" table cell does not indicate there is an attachment. The AccessibleState AT-SPI-CHECKED state should be set for the "attachment" cell. The same goes for messages that have been read. The AccessibleState AT-SPI-CHECKED state should be set for the "read" cell.

This is a significant accessibility problem because assistive technologies cannot tell the user that a mail message has an attachment, or some other important attribute.

Reproducible: Always

Steps to Reproduce:
1. Start Thunderbird 3 and find a mail message header that indicates the message has an attachment.
2. Run at-poke and examine the state for the cell in the "attachment" column. You will set there is nothing in the state set to indicate the message has an attachment
3 [review]. Verify the same holds for messages that have not been read.
Actual Results:  
See above

Expected Results:  
See above

Comment 1

[Aaron Leventhal]

I'm not sure it's really a checkbox though. It seems more like an image which should have a tooltiptext saying what it means.

Do we even know that it's only a 2 way toggle?

Comment 2

[Evan Yan]

according to the description of attribute "cycler" in treecol element,
http://xulplanet.com/references/elemref/ref_treecol.html
It's should be a 2 way toggle, and I think we can expose it as checkbox.

Comment 3

[Aaron Leventhal]

Thanks. That makes sense. How do you progmatically determine if it's checked or not for a given row, col?

If you know of a fix, please feel free to take the bug. Otherwise, assigning to David Bolter so he can take it once he gets up to speed, since this affects Thunderbird a lot. 

Moving to core since this affects all tree views.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Evan, (Aaron),

I'm looking at: nsXULTreeitemAccessible::GetState (in nsXULTreeAccessible.cpp)?  (Please Let me know if you already can already tell I'm off track).

Comment 5

[Aaron Leventhal]

That's the right place.

You'll need STATE_CHECKABLE if it's a checkbox, and STATE_CHECKED if it's checked. Not sure what API you'll find that in, but you can look through the tree interfaces.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

In nsXULTreeitemAccessible::GetState I see two potential ways to decide if there should be a STATE_CHECKABLE:

1.
    PRBool isCycler;
    mColumn->GetCycler(&isCycler);
    if (isCycler)
      *_retval |= STATE_CHECKABLE;

2.
    PRInt16 type;
    mColumn->GetType(&type);
    if (type == nsITreeColumn::TYPE_CHECKBOX)
      *_retval |= STATE_CHECKABLE;

If we choose the latter I wonder if it will require us to expose the cycler with ROLE_CHECKBOX as Evan seemed to suggest in comment #2.

BTW I'm a bit stuck as to how to get the checked/unchecked state.

Comment 7

[Evan Yan]

I think both.

If (type == CHECKBOX), I believe it means the table cell will be rendered as a checkbox; while (cycler == true) means the table cell has a 2 way toggle. Either of the condition, we should expose it as CHECKABLE.

Comment 8

[Aaron Leventhal]

Does type = checkbox mean that space bar will toggle it? How is it different from the cycler? Only in appearance?

Comment 9

[Evan Yan]

As my understanding, type == checkbox means user really see a checkbox, like Thunderbird File menu->Subscribe, and yes, space bar will toggle it.
Bug 312457 is about exposing table cell whose type is checkbox.

A table cell can have cycler == true while type != checkbox, it still should be CHECKABLE. I'm not sure whether type == checkbox will implicitly mean cycler == true, but I guess not.

Mona, any comment on this?

Comment 10

[Aaron Leventhal]

I usually see checkboxes in a tree, only in the first column. This is generally for those cases where there is really a tree of checkboxes. Something like:

Items to install:
[x] Help docs
    [x] Easy help
    [ ] Intermediate help
    [ ] Advanced help
[ ] Languages
    [ ] Mandarin
    [ ] English
    [ ] Klingon

Do we use type="checkbox" anywhere in Mozilla? We need something to test.

Comment 11

[Evan Yan]

(In reply to comment #10)
> Do we use type="checkbox" anywhere in Mozilla? We need something to test.
> 
in Thunderbird when using IMAP, below two dialogs look like.
  File menu->Subscribe
  Account Settings->Offline & Disk Space->Select folders for offline use.

However, looking into their xul code, it's not as easy as just using "type=checkbox".
I'm not clear about the implementation of them.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: unfinished patch

Just posting where I'm at with this bug so far.  Going to try and grab the checked state for a cycler now...

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Just spoke with Silver on channel #xul and I've learned that there is no standard way to get cycler state. I see two ways we could go:
1. Turn all cyclers into checkboxes (using css to swap checkmarks for appropriate icons)
2. Create a standard way to get cycler state. Although it seems to me that a cycler is something that could conceivably have many states (e.g. 5); but the documentation seems to imply there are 2 ("on" or "off") and that seems to be the common usage. [shrug]

I feel a bit too newbie-ish to know what to pursue here. 

Comment 14

[Aaron Leventhal]

Mano, can you take a look at David's questions in comment 13?

You're asking the right questions, now we just need to get the right person to answer them.

Comment 16

[Lynn Monsanto]

I changed the bug summary because the problem with table cell state information
is a general one, it's not just about check boxs. 

For Thunderbird, the most serious problem occurs in the message header list.
There are columns for whether the mail contains an attachment, whether the mail
was read, replied-to, forwarded, etc. 

Because the table cell state does not indicate the appropriate state, it is not
possible for assistive technologies to speak or Braille whether the mail has an
attachment, was read, forwarded, replied to, etc. This is a serious problem,
especially in the case of attachments, what are bad things to not know about.

Comment 17

[Asaf Romano (gone)]

2) (in comment 13) is the way to go IMHO, though maybe callers should set a separate attribute for a11y (at least for complex cases). Ccing Neil as well.

Comment 18

[Neil Deakin]

Cycler cells may cycle between more than two states. I believe there are three states for the junk flag (unknown, junk and not junk).

The label for the cell should be retrieved with GetCellText.

Note that cycling a cell by clicking on a cycler cell doesn't necessarily change any state associated with the cell.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks very much for your comments.

We need a way to get at the state (model) for the cell (view). From the context of the cell, I'm not sure (yet) if there is a way to get at the underlying state (from the context: nsXULTreeAccessible.cpp).

BTW Aaron, given your comment (#8) in bug 312457,  if we expose state checkable for a cycler, I'm wondering if this will map to an MSAA state checkbox?  And does checkable imply 2 states?

Given Neil's comment 18 perhaps a cycler should be exposed as something else?

Comment 20

[Aaron Leventhal]

Hi David, look at the definition of STATE_CHECKABLE is nsIAcccessible. I believe we're overloading MARQUEE. Terrible! :)

It doesn't imply 2 states -- it lets you know if STATE_CHECKED is not present, that it's unchecked but could be checked.

Comment 21

[Lynn Monsanto]

Hi guys. Thanks for all your effort so far.  Right now, there are only two critical accessibility bugs: this one and 365973. If you can fix these two, Thunderbird accessibility will be a lot better.  Thanks!

Comment 22

[Lynn Monsanto]

Fixed typo in comment #21. The other critical bug is bug 356617.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Lynn for your help too! FWIW I'm coming back to this bug soon. (Updating my stack ATM)

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

BTW Lynn is ORCA also susceptible to bug 364376?  (Sorry to ask here... Lynn are you on IRC? I'm davidb on irc.mozilla.org)

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Correction for my comment 19... where I write "MSAA state checkbox" I should have written "MSAA role checkbox". It get a bit confusing because state checkable is normally indicated by role checkbox in other toolkits... so we have state mapping to role (yikes). But I don't think that is our biggest problem here.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #18)
> Cycler cells may cycle between more than two states. I believe there are three
> states for the junk flag (unknown, junk and not junk).
> 
> The label for the cell should be retrieved with GetCellText.

There is no text returned for icon based cyclers. Would it make sense to have them return the header cell's text in the absence of any local text?  (Note I don't know if this is possible).

> 
> Note that cycling a cell by clicking on a cycler cell doesn't necessarily
> change any state associated with the cell.
> 

This seems like a show stopper?  I'd attempt filing a bug but I don't know where.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #17)
> 2) (in comment 13) is the way to go IMHO, though maybe callers should set a
> separate attribute for a11y (at least for complex cases). Ccing Neil as well.
> 

Thanks again for your comment Mano. Which callers and what attributes are you thinking of? This sounds promising and I want to make sure I understand your meaning.

Comment 28

[Aaron Leventhal]

We had to add STATE_CHECKABLE because there are roles like menuitem, treeitem and listitem where you need to know if it's ever checked, even if it's currently unchecked.

Comment 29

[Willie Walker]

One`thing we do in Orca to handle the missing CHECKABLE state is to look for an action named "toggle".  It's presence amidst other context helps us infer the CHECKABLE state.

Comment 30

[Aaron Leventhal]

(In reply to comment #29)
> One`thing we do in Orca to handle the missing CHECKABLE state is to look for an
> action named "toggle".  It's presence amidst other context helps us infer the
> CHECKABLE state.

Will do you see that as a temporary solution until we get a state or object attribute for this?

Otherwise it seems like app developers will need better docs as to what each AT assume each name action means about an object, so we can provide helpful info. I'd rather have something explicit, but docs from the Orca team as to the necessary magic meanings would be better than nothing.

Comment 31

[Lynn Monsanto]

I think it will work as a temporary solution. Can you prototype a solution for me to test?  Thanks!

Comment 32

[Willie Walker]

The detection of the "toggle" action is semi-heavyweight in that it can require several round trips between Orca and the application.  If we can restrict the round tripping to a few well-known component roles (e.g., table cells), however, then we can live with this in Orca for even the long term since the CHECKABLE state is non-existent in any AT-SPI implementation at this point.

From what I can tell from Tbird, we're getting an "Activate" action on *all* table cells in the message summary area.  What we'd need is a "toggle" action for only those cells that act as checkboxes.  Thanks!

Comment 33

[Aaron Leventhal]

It would be easy to expose "checkable" as an object attribute. How about that?

Comment 34

[Willie Walker]

I suppose we could work with that if you cannot expose a "toggle" action.  Which route do you think you'll take?  Thanks!

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

Will, does Orca require/assume a toggle to have two states?  Or does that not matter?

Comment 36

[Aaron Leventhal]

We should definitely expose a "toggle" action. I meant the object property idea as an alternative for AT developers' consideration.

If you want we'll expose the checkable object attribute if that makes things faster for for the AT or other developers find it to be cleaner or clearer.

Comment 37

[Willie Walker]

(In reply to comment #36)
> We should definitely expose a "toggle" action. I meant the object property idea
> as an alternative for AT developers' consideration.
> 
> If you want we'll expose the checkable object attribute if that makes things
> faster for for the AT or other developers find it to be cleaner or clearer.

Right now, we assume that if it's not CHECKED, then it's not checked.  We probably should also look for STATE_INDETERMINATE, but we currently don't do so.

Comment 38

[Aaron Leventhal]

For now let's just use the toggle action to indicate checkable.

Comment 39

[David Bolter [:davidb] (NeedInfo me for attention)]

Aaron, this sounds reasonable. 

So we still need to solve the problem of exposing the toggle state. From comment 18 I'm worried that there are cases where the object represented by the xulaccessible implementation does not 'know' its own cycle/toggle state (the MVC pattern is broken). I might be misunderstanding the comment but it sounds like there could be code that watches and acts on the widget (perhaps some javascript) but which the widget does not know about -- i.e. there is no connection from the widget model to the actual state?  If this is the case we would want to fix this kind of thing (a xul design issue) to avoid maintenance waste.  In the sort term I would need to find out where the real state is (in each case) and connect it somehow.  

I hope I'm making sense.

I'm going to dig in again now...

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

Update: I now have the cell for read/unread status (currently a green light thingy), reporting localized read/unread status strings (via GetCellValue).  I'm hoping to finish up for the others and post a patch late tonight as I'm in meetings tomorrow. 

Comment 41

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: proposed nsXULTreeitemAccessible patch

This patch provides improved accessible names for cycler type tree cells using GetCellValue (patch coming next). IMHO this most closely fits with the way non-text based cyclers are used in tbird.

Comment 42

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: proposed mailnews patch

An nsMsgDBView::GetCellValue implementation.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: at-poke showing named message cells

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

I created bug 368782 for the toggle action (or checkable state) issue. I felt that work didn't quite fit in this bug which is asking for information like the "read/unread" status of messages. 

Comment 45

[Aaron Leventhal]

Hi David, thanks for the patch.

Can you do your diffs with 
cvs diff -pu5N

+  if (aName.IsEmpty())
+  	mTreeView->GetCellValue(mRow, mColumn, aName);
Formatting nits: we always put { and } even for single lines, and use 2 spaces for indent (except for some files that use 4 spaces).

Why did you decide to drop the code from your first patch which had stuff like:
+    mColumn->GetType(&type);
+    if (type == nsITreeColumn::TYPE_CHECKBOX) {
 ...
}

For cyclers, did you determine whether there was a way to know if it's checked? We really should have CHECKED and state change events, etc.

Comment 46

[Aaron Leventhal]

Is there anything here that would affect the XUL accessibility guidelines -- e.g. how one exposes the text equivalent for cyclers? If so, can you help Aaron Anderson with that?

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

Aaron I ripped out the TYPE_CHECKBOX check because my trace output was showing that check never succeeded (at least in tbird). Shall I put it back in though as a just in case? (BTW I tried to follow the 2 space thing and for the {}'s I tried to use the first style I found in surrounding code... but I prefer putting them in anyway and will of course follow your suggestion)

I'll ping AaronAndy on IRC when I can to follow up on possible guideline affect.

Comment 48

[Aaron Leventhal]

For the TYPE_CHECKBOX you might need to create your own tree view with that to test it. Better to fix everything while we're here, if possible.

Or, if that's too time consuming, file a follow up bug. I believe there's a TYPE_PROGRESSMETER as well, or something like that. We need to be able to deal with what XUL authors are provided with.

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

Sounds good.

I've got some updated patches ready but would rather check over them when I'm fresh tomorrow before posting. 

Maybe I should create a XUL tree cell state tracker and treat this as specific to the Thunderbird message list view?  (since the patches here include a mailnews diff).  

Do moz module owners generally prefer bite-sized bugfix patches, or sweeping uber-patches for commits? 

Comment 50

[David Bolter [:davidb] (NeedInfo me for attention)]

Sounds good.

I've got some updated patches ready but would rather check over them when I'm fresh tomorrow before posting. 

Maybe I should create a XUL tree cell state tracker, and treat this bug here as specific to the Thunderbird message list view (as described by Lynn)?  (note mailnews patch is fairly specific to this bug).  

Do moz module owners generally prefer bite-sized bugfix patches, or sweeping uber-patches for commits?  I prefer smaller commits but am fine either way.

Comment 51

[David Bolter [:davidb] (NeedInfo me for attention)]

If someone has "editbugs" privs please feel free to delete this comment (51?) and comment 49

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: improved patch for module: accessible

Aaron I put back the checkbox code. I'd like to create a separate bug to test that code as well as adding other checks.

Comment 53

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: improved patch for modules: mail, and mailnews

Comment 54

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: improved patch for modules: mail, and mailnews

Removed untested fix for another bug that creeped into this diff.

Comment 55

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 253618 [details] [diff] [review]
improved patch for module: accessible

note the Makefile.in part of the diff should be ignored

Comment 56

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 253620 [details]
improved patch for modules: mail, and mailnews

this touches mail and mailnews.

Comment 57

[Aaron Leventhal]

Comment on attachment 253618 [details] [diff] [review]
improved patch for module: accessible

I think we better add a comment in front of the cycler/CHECKABLE code, like:
// XXX need changes to blah blah to support CHECKED state

Comment 58

[David :Bienvenu]

Comment on attachment 253620 [details]
improved patch for modules: mail, and mailnews

hey David, looks good. Congrats on your first patch :-) A few comments:

I think this code can be tightened up with the handy dandy ? operator :-)

+  // provide a string "value" for cells that do not normally have text.
+  switch (colID[0]) {
+    case 'a':
+      if (flags & MSG_FLAG_ATTACHMENT) {
+        aValue.Assign(GetString(NS_LITERAL_STRING("messageHasAttachment").get()));
+      }
+      else {
+        aValue.Assign(GetString(NS_LITERAL_STRING("messageHasNoAttachment").get()));
+      }
+      break;
+    case 'u':
+      if (flags & MSG_FLAG_READ) {
+        aValue.Assign(GetString(NS_LITERAL_STRING("messageRead").get()));
+      }  
+      else {
+        aValue.Assign(GetString(NS_LITERAL_STRING("messageUnread").get()));
+      }
+      break;


can be something like

aValue.Assign(GetString(flags & MSG_FLAG_ATTACHMENT ? NS_LITERAL_STRING("messageHasAttachment").get()
: NS_LITERAL_STRING("messageHasNoAttachment").get()));

and similarly for the messageRead code...
also, you don't need braces around single line if statements, and we definitely don't need K & R style braces :-)

+  nsresult rv = NS_OK;
+
+  rv = GetMsgHdrForViewIndex(aRow, getter_AddRefs(msgHdr));


this should be combined into one line:

nsresult rv = GetMsgHdr...

Comment 59

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: added comment (module: accessible)

Comment 60

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: formatting fixes (modules: mail, mailnews)

DB, Thanks for the style suggestions. I'll be sure to pay attention to the styles in the file I'm working on in the future!  (That crazy K&R style became a habit from my years in GNOME world).

Comment 61

[alexander :surkov (:asurkov)]

Comment on attachment 253707 [details] [diff] [review]
added comment (module: accessible)

 
> NS_IMETHODIMP nsXULTreeitemAccessible::GetName(nsAString& aName)
> {
>   NS_ENSURE_TRUE(mTree && mTreeView, NS_ERROR_FAILURE);
>-  return mTreeView->GetCellText(mRow, mColumn, aName);
>+  mTreeView->GetCellText(mRow, mColumn, aName);
>+  
>+  // if still no name try cell value
>+  if (aName.IsEmpty()) {
>+    mTreeView->GetCellValue(mRow, mColumn, aName);
>+  }

I'd rather check column type since GetCellText() and GetCellValue() are mutually exclusive.

>+  // states for checkbox (or cycler)
>+  if (mColumn) {
>+    PRInt16 type;
>+    mColumn->GetType(&type);
>+    if (type == nsITreeColumn::TYPE_CHECKBOX) {
>+      *_retval |= STATE_CHECKABLE;
>+      nsCOMPtr<nsIDOMXULCheckboxElement> xulCheckboxElement(do_QueryInterface(mDOMNode));

Does it ever work? I wonder if xul:tree implements nsIDOMXULCheckboxElement because mDOMNode is xul:tree, right?

>+      if (xulCheckboxElement) {
>+        PRBool checked = PR_FALSE;
>+        xulCheckboxElement->GetChecked(&checked);
>+        if (checked) {
>+          *_retval |= STATE_CHECKED;
>+          PRInt32 checkState = 0;
>+          xulCheckboxElement->GetCheckState(&checkState);
>+          if (checkState == nsIDOMXULCheckboxElement::CHECKSTATE_MIXED) {  
>+            *_retval |= STATE_MIXED;
>+          }
>+        }
>+      }  
>+    }
>+    else {

Nit, please line up 'else {' statment. Should be '} else {'.

Btw, what about progressmeter type? I guess it's out of box of this bug but it's fine to add XXX comment about this.

>+      PRBool isCycler;
>+      mColumn->GetCycler(&isCycler);
>+      if (isCycler) {
>+        *_retval |= STATE_CHECKABLE;
>+        // XXX we need xul cycler states work before we can reliably 
>+        // discern CHECKED state. 

I'm not familiar with cycler. What is it and why state is checkable if column is not checkbox?

> // "activate" action is available for all treeitems
> // "expand/collapse" action is avaible for treeitem which is container

Btw, what aboaut actions for checkbox column? How they should be keeped by GetActionName()?

Comment 62

[David Bolter [:davidb] (NeedInfo me for attention)]

Here is my plan partially based on discussion on #accessibility today.

1. Remove the CHECKABLE state work for checkboxes and cyclers in xul tree columns.
2. Implement nsXULTreeitemAccessible::GetValue to return current cycler value.
3. Implement correct state set including ENABLED + SENSITIVE (if necessary).
4. Expose "cycle" action. "Toggle" doesn't fit all cycler cases.
5. Explain this to ia2 and g-a-dev lists.
6. Get feedback from orca etc.

I'm much happier with this. It feels like a decent solution until the xul cycler rethink/redesign happens (or even if it doesn't).

Comment 63

[David Bolter [:davidb] (NeedInfo me for attention)]

Errata:
#2 I'll leave my patch as is to have the value returned via GetName. ATK doesn't allow string values.

Comment 64

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: proposed combined patch

Improved patch. Implemented "cycler" action. Added flag status. Note this patch touches: accessibility, mail, mailnews, and dom modules.

Comment 65

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: removed an unecessary header include.

Comment 66

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 253966 [details] [diff] [review]
removed an unecessary header include.

Surkov, perhaps David Bienvenu can help review the mail, mailnews parts. I've made the changes he requested but he might want another pass.

Comment 67

[David :Bienvenu]

David, the mail parts look OK - except I was thinking you could move the ? operator into the GetString argument, not outside of it - or did that not compile?

Comment 68

[Evan Yan]

Comment on attachment 253966 [details] [diff] [review]
removed an unecessary header include.

>+# Used in message database list view to provide a text value for graphic based cells.
>+messageRead=Read
>+messageUnread=Unread
>+messageHasFlag=Flagged
>+messageHasNoFlag=No Flag
>+messageHasAttachment=Has Attachment
>+messageHasNoAttachment=No Attachment

Will l10n team listen to cvs changes and then get these translated, or should we give some notification?

Comment 69

[alexander :surkov (:asurkov)]

Comment on attachment 253966 [details] [diff] [review]
removed an unecessary header include.

Please follow mozilla code style rules (you can see then at http://www.mozilla.org/hacking/mozilla-style-guide.html) though they are probably are out of date.

>-  if (mRow < firstVisibleRow || mRow > lastVisibleRow)
>+  if (mRow < firstVisibleRow || mRow > lastVisibleRow) {
>     *_retval |= STATE_INVISIBLE;
>-
>+  }

nit: you shoudn't add braces here.

> // "activate" action is available for all treeitems
> // "expand/collapse" action is avaible for treeitem which is container

If you add new "cycle" action then please mention it here.

>   if (index == eAction_Click) {
>-    nsAccessible::GetTranslatedString(NS_LITERAL_STRING("activate"), _retval);
>+    PRBool isCycler;
>+    mColumn->GetCycler(&isCycler);
>+    if (isCycler) {
>+      nsAccessible::GetTranslatedString(NS_LITERAL_STRING("cycle"), _retval);
>+    }

I'm not sure will not AT be confused when they will see 'cycle' action name?

>+  	else {
>+      nsAccessible::GetTranslatedString(NS_LITERAL_STRING("activate"), _retval);
>+  	}

nit: please use two spaces indent here.

>   if (index == eAction_Click) {
>+  	nsresult rv;

nit: too

>     nsCOMPtr<nsITreeSelection> selection;
>     mTreeView->GetSelection(getter_AddRefs(selection));
>     if (selection) {
>-      nsresult rv = selection->Select(mRow);
>+      rv = selection->Select(mRow);
>       mTree->EnsureRowIsVisible(mRow);
>-      return rv;

if tree has selection then cycle action will be executed never.

>     }
>+    PRBool isCycler;
>+    mColumn->GetCycler(&isCycler);
>+    if (isCycler) {
>+      rv = mTreeView->CycleCell(mRow, mColumn);
>+    }
>+    return rv;

rv is not defined here.

> NS_IMETHODIMP nsMsgDBView::GetCellValue(PRInt32 aRow, nsITreeColumn* aCol, nsAString& aValue)
> {
>-  return NS_OK;
>+  nsCOMPtr <nsIMsgDBHdr> msgHdr;
>+  nsresult rv = GetMsgHdrForViewIndex(aRow, getter_AddRefs(msgHdr));
>+
>+  if (NS_FAILED(rv) || !msgHdr) 
>+  {
>+    ClearHdrCache();
>+    return NS_MSG_INVALID_DBVIEW_INDEX;
>+  }
>+
>+  const PRUnichar* colID;
>+  aCol->GetIdConst(&colID);
>+
>+  PRUint32 flags;
>+  msgHdr->GetFlags(&flags);
>+  
>+  // provide a string "value" for cells that do not normally have text.
>+  switch (colID[0]) 
>+  {
>+    case 'a': // attachment column

I think it's potentialy dangerous to check only the first letter of column id. Isn't it?

>+      aValue.Assign((flags & MSG_FLAG_ATTACHMENT) ? 
>+      GetString(NS_LITERAL_STRING("messageHasAttachment").get())
>+      : GetString(NS_LITERAL_STRING("messageHasNoAttachment").get()));
>+      break;

In general I'd like to see here if/else statements. Or like David Bienvenu noticed.

Comment 70

[alexander :surkov (:asurkov)]

David, I guess you should get additional r or sr for dom/mail/mailnews changes too.

Comment 71

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Surkov, I'll respond to some of your comments here:

(In reply to comment #69)
> (From update of attachment 253966 [details] [diff] [review])
> >+  if (mRow < firstVisibleRow || mRow > lastVisibleRow) {
> >     *_retval |= STATE_INVISIBLE;
> >-
> >+  }
> 
> nit: you shoudn't add braces here.
>

We should probably all agree on a consistent style for the accessible module. I guess Aaron, Evan, and yourself can decide?
 
> >+      nsAccessible::GetTranslatedString(NS_LITERAL_STRING("cycle"), _retval);
> >+    }
> 
> I'm not sure will not AT be confused when they will see 'cycle' action name?
>

Yes, however this came from a request by the Orca team. We can easily go back to 'activate' if this causes a problem.
 
> >     nsCOMPtr<nsITreeSelection> selection;
> >     mTreeView->GetSelection(getter_AddRefs(selection));
> >     if (selection) {
> >-      nsresult rv = selection->Select(mRow);
> >+      rv = selection->Select(mRow);
> >       mTree->EnsureRowIsVisible(mRow);
> >-      return rv;
> 
> if tree has selection then cycle action will be executed never.

I think you misread the diff here.  The return was removed.

> 
> >     }
> >+    PRBool isCycler;
> >+    mColumn->GetCycler(&isCycler);
> >+    if (isCycler) {
> >+      rv = mTreeView->CycleCell(mRow, mColumn);
> >+    }
> >+    return rv;
> 
> rv is not defined here.
> 

Again, just a mis-read I believe.  The code compiles and tests okay :)

> >+  // provide a string "value" for cells that do not normally have text.
> >+  switch (colID[0]) 
> >+  {
> >+    case 'a': // attachment column
> 
> I think it's potentialy dangerous to check only the first letter of column id.
> Isn't it?
>

I totally agree but I am copying similar code in the same file.
 
> >+      aValue.Assign((flags & MSG_FLAG_ATTACHMENT) ? 
> >+      GetString(NS_LITERAL_STRING("messageHasAttachment").get())
> >+      : GetString(NS_LITERAL_STRING("messageHasNoAttachment").get()));
> >+      break;
> 
> In general I'd like to see here if/else statements. Or like David Bienvenu
> noticed.
> 

David Bienvenu asked me to remove the if/else and use the ? operator. I can follow up with him on the mail and mailnews parts of the patch. I'll prepare and post a new accessible patch soon.

Thanks again.

Comment 72

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: accessible, updated patch

Surkov, Aaron likes the curly braces... I think we should follow his lead for the 'accessible' module. I've added a comment about the new action and removed those sneaky tabs.

Comment 73

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: mail, mailnews, updated patch

Comment 74

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: dom

This seems harmless, Aaron can you review this (from dom module) or should I assign to that module owner?

Comment 75

[Aaron Leventhal]

Comment on attachment 254171 [details] [diff] [review]
module: dom

Let them complain -- doesn't seem like a problem to me.

Comment 76

[David :Bienvenu]

David, I was thinking this code:

+    case 'a': // attachment column
+      aValue.Assign((flags & MSG_FLAG_ATTACHMENT) ? 
+      GetString(NS_LITERAL_STRING("messageHasAttachment").get())
+      : GetString(NS_LITERAL_STRING("messageHasNoAttachment").get()));
+      break;
+    case 'f': // flagged column
+      aValue.Assign((flags & MSG_FLAG_MARKED) ? 
+      GetString(NS_LITERAL_STRING("messageHasFlag").get())
+      : GetString(NS_LITERAL_STRING("messageHasNoFlag").get()));
+      break;
+    case 'u': // read/unread column
+      aValue.Assign((flags & MSG_FLAG_READ) ?
+      GetString(NS_LITERAL_STRING("messageRead").get())
+      : GetString(NS_LITERAL_STRING("messageUnread").get()));
+      break;

could instead be:

+    case 'a': // attachment column
+      aValue.Assign(
+      GetString((flags & MSG_FLAG_ATTACHMENT) ? NS_LITERAL_STRING("messageHasAttachment").get())
+      : NS_LITERAL_STRING("messageHasNoAttachment").get()));
+      break;
+    case 'f': // flagged column
+      aValue.Assign(
+      GetString((flags & MSG_FLAG_MARKED) ? NS_LITERAL_STRING("messageHasFlag").get())
+      : NS_LITERAL_STRING("messageHasNoFlag").get()));
+      break;
+    case 'u': // read/unread column
+      aValue.Assign(GetString((flags & MSG_FLAG_READ) ?
NS_LITERAL_STRING("messageRead").get())
+      : NS_LITERAL_STRING("messageUnread").get()));
+      break;

Comment 77

[alexander :surkov (:asurkov)]

(In reply to comment #71)

> > if tree has selection then cycle action will be executed never.
> 
> I think you misread the diff here.  The return was removed.

> > rv is not defined here.
> > 
> 
> Again, just a mis-read I believe.  The code compiles and tests okay :)

Sorry, I was blind :)

Comment 78

[alexander :surkov (:asurkov)]

Comment on attachment 254169 [details] [diff] [review]
module: accessible, updated patch


> NS_IMETHODIMP nsXULTreeitemAccessible::DoAction(PRUint8 index)
> {
>-  NS_ENSURE_TRUE(mTree && mTreeView, NS_ERROR_FAILURE);
>+  NS_ENSURE_TRUE(mColumn && mTree && mTreeView, NS_ERROR_FAILURE);
> 
>   if (index == eAction_Click) {
>+    nsresult rv;
>     nsCOMPtr<nsITreeSelection> selection;
>     mTreeView->GetSelection(getter_AddRefs(selection));
>     if (selection) {
>-      nsresult rv = selection->Select(mRow);
>+      rv = selection->Select(mRow);
>       mTree->EnsureRowIsVisible(mRow);
>-      return rv;
>     }
>+    PRBool isCycler;
>+    mColumn->GetCycler(&isCycler);
>+    if (isCycler) {
>+      rv = mTreeView->CycleCell(mRow, mColumn);
>+    }
>+    return rv;

I can see the one suspected thing. I think cycler code should be moved at top. Because when I mouse click on cycler column then selection isn't changed. So if it is cycler column then you shoudn't change selection. Right? 

>Index: dom/locales/en-US/chrome/accessibility/mac/accessible.properties
>===================================================================
>RCS file: /cvsroot/mozilla/dom/locales/en-US/chrome/accessibility/mac/accessible.properties,v
>retrieving revision 1.1

If this goes in another patch then please remove this.

With those r=me.

Comment 79

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #76)

David Bienvenu, I see your point... it is less code. Shall I make that change?

Comment 80

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #78)
> (From update of attachment 254169 [details] [diff] [review])
> 
> > NS_IMETHODIMP nsXULTreeitemAccessible::DoAction(PRUint8 index)
> > {
> >-  NS_ENSURE_TRUE(mTree && mTreeView, NS_ERROR_FAILURE);
> >+  NS_ENSURE_TRUE(mColumn && mTree && mTreeView, NS_ERROR_FAILURE);
> > 
> >   if (index == eAction_Click) {
> >+    nsresult rv;
> >     nsCOMPtr<nsITreeSelection> selection;
> >     mTreeView->GetSelection(getter_AddRefs(selection));
> >     if (selection) {
> >-      nsresult rv = selection->Select(mRow);
> >+      rv = selection->Select(mRow);
> >       mTree->EnsureRowIsVisible(mRow);
> >-      return rv;
> >     }
> >+    PRBool isCycler;
> >+    mColumn->GetCycler(&isCycler);
> >+    if (isCycler) {
> >+      rv = mTreeView->CycleCell(mRow, mColumn);
> >+    }
> >+    return rv;
> 
> I can see the one suspected thing. I think cycler code should be moved at top.
> Because when I mouse click on cycler column then selection isn't changed. So if
> it is cycler column then you shoudn't change selection. Right? 

Hmmmm you are right!  I had assumed the row would be selected as well normally.

Updated patch coming.

Comment 81

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: accessible

Comment 82

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: mail, mailnews, updated patch

Comment 83

[David :Bienvenu]

Comment on attachment 254181 [details] [diff] [review]
module: mail, mailnews, updated patch

thx, David!

Comment 84

[Aaron Leventhal]

Comment on attachment 254180 [details] [diff] [review]
module: accessible

(Conditional r+) 
In the following code there is a case where rv is uninitalized. r+ if you fix that.
+    nsresult rv;
+    PRBool isCycler;
+    mColumn->GetCycler(&isCycler);
+    if (isCycler) {
+      rv = mTreeView->CycleCell(mRow, mColumn);
+    } 
+    else {
+      nsCOMPtr<nsITreeSelection> selection;
+      mTreeView->GetSelection(getter_AddRefs(selection));
+      if (selection) {
+        rv = selection->Select(mRow);
+        mTree->EnsureRowIsVisible(mRow);
+      }
     }
+    return rv;

Comment 85

[Scott MacGregor]

Comment on attachment 254181 [details] [diff] [review]
module: mail, mailnews, updated patch

The patch looks good David. There are some minor wording issues with the strings in messenger.properties though:

1) I think all of these strings exist elsewhere in messenger.properties. But maybe they are used in different contexts, so they may be localized differently..hmm.

2) If we do use these new strings, we no longer call flags: Flags. They are now stars :). See the values we already use in that file:

http://lxr.mozilla.org/mozilla/source/mail/locales/en-US/chrome/messenger/messenger.properties#200

Comment 86

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: flags are now stars

Scott, thanks very much for catching the flag/star wording (your 2nd point). I have flip flopped over whether to try and use existing name/string pairs, but in the end went with what I felt was "safer" (your 1st point regarding context); this after irc'ing with biesi and mrfinkle last week :)

Comment 87

[Scott MacGregor]

Comment on attachment 254185 [details] [diff] [review]
flags are now stars

excellent.

Comment 88

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: module: accessible, (patch to commit)

Comment 89

[Evan Yan]

Mike recommended that, when a thread is not marked as starred or having attachment, it's better to say nothing than to say "Not Starred", "No Attachment".

I think it's reasonable. Keep reporting the normal status just bothering users.

Any comments?

Comment 90

[Aaron Leventhal]

I agree.

Comment 91

[Evan Yan]

We now don't expose CHECKABLE state for cycle table cells, instead we expose 'cycle' action for them. But how ATs can see the status(checked/unchecked/...) of a cycle cell?

Besides, attachment cells don't have 'cycle' action.

Comment 92

[Aaron Leventhal]

The accessible name, I think. David Bolter, is that correct?

Comment 93

[David Bolter [:davidb] (NeedInfo me for attention)]

Evan, Aaron is correct.

I know it is a bit unusual, but we are exposing the "value" of the cell via the accessible name (at least, last time I checked). In theory, cyclers are a bit like spinners in that they can have many different values (more than checked or unchecked). 

E.g. a cycler might have these values: "high", "medium", "low", "trivial". To say such a cycler is checkable, or checked doesn't fit.

That said, there might be a better way to expose the values via atk... I'm not sure.

Comment 94

[Evan Yan]

Attachment: use empty string for the normal states "Read", "Not Starred", "No Attachment" and Not Junk"

so that ATs won't bother user with keep reporting the normal status

Comment 95

[Scott MacGregor]

Comment on attachment 268921 [details] [diff] [review]
use empty string for the normal states "Read", "Not Starred", "No Attachment" and Not Junk"

David Bolter and Aaronl say this is the right thing to do, that's good enough for me. 

Evan, since we are changing the "meaning" of these strings, we want to make sure the change propogates out to all locales. So we need to change the entity names to alert localizers.

messageHasNoFlag1, messageRead1

should do the trick.

Thanks!

Comment 96

[Evan Yan]

Attachment: change entity names

Comment 97

[Scott MacGregor]

Comment on attachment 269041 [details] [diff] [review]
change entity names

Don't we we need to change the callers of these entity names too?

Comment 98

[Evan Yan]

Attachment: also change suite/ accordingly

sorry, I was a bone head.

Comment 99

[Scott MacGregor]

Comment on attachment 269161 [details] [diff] [review]
also change suite/ accordingly

One last parting though, but i'll still plus this patch.

If it's the case that for all locales, we would always want these strings to be empty for accessibility reasons, maybe we should remove the strings all together and just change the C++ to use an empty string.

i.e.

       aValue.Assign(GetString ((flags & MSG_FLAG_READ) ?
-      NS_LITERAL_STRING("messageRead").get()
+      EmptyString()
       : NS_LITERAL_STRING("messageUnread").get()));

Comment 100

[Evan Yan]

Attachment: patch to check in, with Scott's comments addressed

Comment 101

[Firefox Bug Husbandry Bot]

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