246236 - Accessible name not reported for mailnews message list tree view items

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Aaron Leventhal]

Steps:
1. Open mail
2. Run screen reader or accessibility testing tool
3. Arrow through list of mail messages

What happens:
Accessible name is not spoken/reported for each tree item, even though accesible
names work for other trees in the the system.

What should happen:
Names should be something like:
"New, unread, marked as junk. New herbal supplementals for $$ CHEAP $$$.
[e-mail]. 1 attachment. 6/10/2004 11:15 pm"
"Flagged. Louie bring some wine to the barbeque on Sunday. Aaron Leventhal.
6/10/2004 10:25 pm"

Louie, are you interested in taking this one? :)

Comment 1

[Andrew Schultz]

Is this only SeaMonkey or Thunderbird also?

Comment 2

[Aaron Leventhal]

(In reply to comment #1)
> Is this only SeaMonkey or Thunderbird also?

Most likely tbird as well but no one has looked at either in a while.

Comment 3

[Aaron Leventhal]

Still happens on Thunderbird version 2 alpha 1 (20060503)

I usually lose the names the 2nd time I go to the tree view when I switch folders.
Down arrow shift+tab down arrow tab

But, depending on how many items are in each folder and where I end up, it doesn't always happen. That's a clue if we could figure out when it doesn't happen.

Comment 4

[Aaron Leventhal]

I suspect that tree doesn't play well with the a11y cache because it doesn't fire changes through nsIDocumentObserver.

Comment 5

[Aaron Leventhal]

Must have something to do with nsIAccessibleTreeCache.

Louie Zhao is no longer available. Ken Perry (Whistler) wanted to work on it, but this is a very tough first bug. Who wants it?

Comment 6

[Aaron Leventhal]

Attachment: Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

Also fixes bug 328273 (could not duplicate, but descriptions are identical, just in bookmarks intead).

Comment 7

[Ginn Chen]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

r=me

Comment 8

[Aaron Leventhal]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

I forgot the file to fire the event.

Comment 9

[Aaron Leventhal]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

Doh, it's in there -- nsTreeBodyFrame.cpp.

Comment 10

[Aaron Leventhal]

Attachment: Patch for trunk (mechanism for adding event listener slightly different)

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this before June.

Comment 12

[[e-mail]]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

Sorry, I don't understand why you need to watch for calls to Invalidate() either a) at all or b) only and not the other variants.

Comment 13

[[e-mail]]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

>-  nsCOMPtr<nsITreeColumn> col = aColumn;
>+  nsCOMPtr<nsITreeColumn> col;
>+#ifdef MOZ_ACCESSIBILITY_ATK
>+  col = aColumn;
>+#endif
Nor do I understand this change.

Comment 14

[Aaron Leventhal]

Comment on attachment 221463 [details] [diff] [review]
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code

Let me explain -- the nsXULTreeAccessible assumes that the current tree box view will stay constant. If that changes, we need to invalidate the nsXULTreeAccessible and thus a new one will be created when it's next needed. If there are other places where that can happen we should deal with them as well.

Comment 15

[Aaron Leventhal]

> Nor do I understand this change.
In MSAA we don't expose separate accessibles for each column. Each entire row of the tree is treated as one accessible. When col != nsnull it means we're trying to get the accessible for a given column, which is only a valid thing to do in ATK.  This change ensures we keep col = nsnull for MSAA, but is not related to the rest of this fix. We don't need it because the callers do it too, but it's just safer to have it.

A long term goal will be to have the MSAA and ATK implementations merge so that we don't need these ugly ifdefs.

> 

Comment 16

[[e-mail]]

(In reply to comment #14)
>If there are other places where that can happen we should deal with them as well.
invalidate
invalidateColumn
invalidateRow
invalidateCell
invalidateRange
rowCountChanged
endUpdateBatch (may be nested, only the last one counts)

Comment 17

[Aaron Leventhal]

Attachment: Fire TreeInvalidated only when mView changes

Neil, I actually made it fire TreeInvalidated less often. The accessible tree only becomes invalid when it's internally cached nsCOMPtr<nsITreeView> mView changes.

The cached tree accessibles don't need to be invalidated when tree data changes unless mView changes. They are based on row and column, so each new call on the cached accessible will already get correct current info.

Comment 18

[[e-mail]]

Comment on attachment 222139 [details] [diff] [review]
Fire TreeInvalidated only when mView changes

>+  nsCOMPtr<nsIBoxObject> treeBox = do_QueryInterface(mTreeBoxObject);
>+  if (treeBox) {
>+    nsCOMPtr<nsIDOMElement> treeElt;
>+    treeBox->GetElement(getter_AddRefs(treeElt));
>+    nsCOMPtr<nsIContent> treeContent = do_QueryInterface(treeElt);
>+    if (treeContent) {
>+      FireDOMEvent(NS_LITERAL_STRING("TreeInvalidated"), treeContent);
>+    }
>+  }
I think you probably want to use something along the lines of
  nsIContent* baseElement = GetbaseElement();
  if (baseElement)
    FireDOMEvent(NS_LITERAL_STRING("TreeInvalidated"), baseElement);
or failing that you want to use SetView's existing box variable.

Comment 19

[Aaron Leventhal]

Attachment: Addresses Neil's comments

Comment 20

[[e-mail]]

Comment on attachment 222289 [details] [diff] [review]
Addresses Neil's comments

>+  nsCOMPtr<nsIContent> treeContent = GetBaseElement();
>+  if (!treeContent) {
>+    nsCOMPtr<nsIDOMElement> treeElt;
>+    box->GetElement(getter_AddRefs(treeElt));
>+    treeContent = do_QueryInterface(treeElt);
>+  }
Sorry, when I said "failing that" I mean "if you can't get GetBaseElement to work", not "use box as a fallback for GetBaseElement"; since EnsureBoxObject calls GetBaseElement this will crash because box will be null too. Assuming GetBaseElement worke just use that and ignore box. sr=me with that fixed.

FireDOMEvent is safe to call from the middle of a frame method, right?

Since you've moved the event firing from Invalidate to SetView maybe you should rename the event, although "TreeViewChanged" is a bit of a mouthful.

Comment 21

[Aaron Leventhal]

Attachment: Final patch for branch, addressed Neil's last comments, tested and works

Neil, it's safe to fire the events in that case because FireDOMEvent() uses an nsPLDOMEvent which causes the event to be fired when it is safe.

Comment 22

[Aaron Leventhal]

*** Bug 335577 has been marked as a duplicate of this bug. ***

Comment 23

[Aaron Leventhal]

*** Bug 328273 has been marked as a duplicate of this bug. ***

Comment 24

[Aaron Leventhal]

*** Bug 257461 has been marked as a duplicate of this bug. ***

Comment 25

[David :Bienvenu]

Comment on attachment 222341 [details] [diff] [review]
Final patch for branch, addressed Neil's last comments, tested and works

is this worth taking in the next point release?

Comment 26

[Aaron Leventhal]

David, people are asking loudly for it because it will make Thunderbird accessible, but I assumed we only take security/stability fixes there.

Comment 27

[David :Bienvenu]

we also take a few high impact safe bug fixes - this seems like one we'd take.

Comment 28

[James Teh [:Jamie] (away returning 28 Jan)]

Note that this does cause crashes in Thunderbird 1.5.0.4, as I describe in my comments for bug 257461. I am now using Thunderbird 2alpha without these crashes, which would seem to confirm that this was the cause.

Comment 29

[Marco Zehe]

(In reply to comment #27)
> we also take a few high impact safe bug fixes - this seems like one we'd take.

David, has a decision for TB 1.5.0.5 been made yet? I realize that 1.5.0.4 is in the final testing stages for release.
I've been getting quite a number of people telling me that this is the only thing that keeps them from using tB. ut they're too shy to use the 2.0 alphas.

Comment 30

[David :Bienvenu]

no, it hasn't been decided yet - when it's decided, the request will either be + or -, not ?. I don't think they've started looking at .05 nominations yet.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 222341 [details] [diff] [review]
Final patch for branch, addressed Neil's last comments, tested and works

approved for 1.8.0 branch, a=dveditz for drivers

Comment 32

[Aaron Leventhal]

Had to back out for bustage

Comment 33

[Aaron Leventhal]

*** Bug 327396 has been marked as a duplicate of this bug. ***

Comment 34

[Aaron Leventhal]

*** Bug 328272 has been marked as a duplicate of this bug. ***

Comment 35

[Firefox Bug Husbandry Bot]

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