Last Comment Bug 246236 - Accessible name not reported for mailnews message list tree view items
: Accessible name not reported for mailnews message list tree view items
Status: VERIFIED FIXED
: access, fixed1.8.0.5, fixed1.8.1, sec508
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Aaron Leventhal
:
:
Mentors:
: 257461 327396 328272 328273 335577 (view as bug list)
Depends on: 335577
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-10 09:59 PDT by Aaron Leventhal
Modified: 2009-11-07 13:04 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch for 1.8 branch -- fix for trunk would be the same except for nsRootAccessible listener adding code (5.38 KB, patch)
2006-05-09 09:09 PDT, Aaron Leventhal
ginn.chen: review+
Details | Diff | Splinter Review
Patch for trunk (mechanism for adding event listener slightly different) (3.83 KB, patch)
2006-05-12 09:58 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fire TreeInvalidated only when mView changes (5.94 KB, patch)
2006-05-15 20:22 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Addresses Neil's comments (5.90 KB, patch)
2006-05-16 18:27 PDT, Aaron Leventhal
neil: superreview+
Details | Diff | Splinter Review
Final patch for branch, addressed Neil's last comments, tested and works (5.73 KB, patch)
2006-05-17 06:18 PDT, Aaron Leventhal
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Aaron Leventhal 2004-06-10 09:59:08 PDT
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 $$$.
g3sdf22@netscams.com. 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 2005-10-02 20:38:49 PDT
Is this only SeaMonkey or Thunderbird also?
Comment 2 Aaron Leventhal 2005-10-02 21:06:56 PDT
(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 2006-05-03 14:11:53 PDT
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 2006-05-03 14:16:36 PDT
I suspect that tree doesn't play well with the a11y cache because it doesn't fire changes through nsIDocumentObserver.
Comment 5 Aaron Leventhal 2006-05-03 18:35:44 PDT
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 2006-05-09 09:09:38 PDT
Created attachment 221463 [details] [diff] [review]
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 2006-05-11 21:00:25 PDT
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 2006-05-12 08:42:45 PDT
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 2006-05-12 08:44:55 PDT
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 2006-05-12 09:58:10 PDT
Created attachment 221822 [details] [diff] [review]
Patch for trunk (mechanism for adding event listener slightly different)
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 11:17:34 PDT
I'm not likely to get to this before June.
Comment 12 neil@parkwaycc.co.uk 2006-05-12 14:21:25 PDT
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 neil@parkwaycc.co.uk 2006-05-12 14:22:13 PDT
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 2006-05-14 18:27:43 PDT
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 2006-05-14 18:32:09 PDT
> 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 neil@parkwaycc.co.uk 2006-05-15 09:46:09 PDT
(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 2006-05-15 20:22:37 PDT
Created attachment 222139 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2006-05-16 16:55:40 PDT
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 2006-05-16 18:27:13 PDT
Created attachment 222289 [details] [diff] [review]
Addresses Neil's comments
Comment 20 neil@parkwaycc.co.uk 2006-05-17 04:17:27 PDT
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 2006-05-17 06:18:09 PDT
Created attachment 222341 [details] [diff] [review]
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 2006-05-17 08:58:47 PDT
*** Bug 335577 has been marked as a duplicate of this bug. ***
Comment 23 Aaron Leventhal 2006-05-17 09:01:45 PDT
*** Bug 328273 has been marked as a duplicate of this bug. ***
Comment 24 Aaron Leventhal 2006-05-25 06:07:50 PDT
*** Bug 257461 has been marked as a duplicate of this bug. ***
Comment 25 David :Bienvenu 2006-05-25 08:29:49 PDT
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 2006-05-25 08:43:55 PDT
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 2006-05-25 08:47:14 PDT
we also take a few high impact safe bug fixes - this seems like one we'd take.
Comment 28 James Teh [:Jamie] 2006-05-25 17:45:35 PDT
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 2006-05-30 03:23:11 PDT
(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 2006-05-30 09:29:34 PDT
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] 2006-06-14 14:51:15 PDT
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 2006-06-15 11:36:58 PDT
Had to back out for bustage
Comment 33 Aaron Leventhal 2006-07-11 12:29:24 PDT
*** Bug 327396 has been marked as a duplicate of this bug. ***
Comment 34 Aaron Leventhal 2006-07-11 12:32:48 PDT
*** Bug 328272 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.