Accessible name not reported for mailnews message list tree view items

VERIFIED FIXED

Status

SeaMonkey
MailNews: Message Display
--
major
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

(4 keywords)

Trunk
access, fixed1.8.0.5, fixed1.8.1, sec508

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

13 years ago
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? :)
(Assignee)

Updated

12 years ago
Component: Disability Access APIs → General
Product: Core → Mozilla Application Suite

Comment 1

12 years ago
Is this only SeaMonkey or Thunderbird also?
Component: General → MailNews: Main Mail Window
(Assignee)

Comment 2

12 years ago
(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.
(Assignee)

Updated

11 years ago
Depends on: 335577
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 4

11 years ago
I suspect that tree doesn't play well with the a11y cache because it doesn't fire changes through nsIDocumentObserver.
(Assignee)

Comment 5

11 years ago
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?
Assignee: Louie.Zhao → nobody
Severity: normal → major
(Assignee)

Comment 6

11 years ago
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).
Assignee: nobody → aaronleventhal
Status: NEW → ASSIGNED
Attachment #221463 - Flags: review?(ginn.chen)

Comment 7

11 years ago
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
Attachment #221463 - Flags: review?(ginn.chen) → review+
(Assignee)

Updated

11 years ago
Attachment #221463 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 8

11 years ago
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.
Attachment #221463 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 9

11 years ago
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.
Attachment #221463 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 10

11 years ago
Created attachment 221822 [details] [diff] [review]
Patch for trunk (mechanism for adding event listener slightly different)

Comment 11

11 years ago
I'm not likely to get to this before June.
(Assignee)

Updated

11 years ago
Attachment #221463 - Flags: superreview?(bzbarsky) → superreview?(neil)

Comment 12

11 years ago
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.
Attachment #221463 - Flags: superreview?(neil)

Comment 13

11 years ago
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.
(Assignee)

Comment 14

11 years ago
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.
Attachment #221463 - Flags: superreview?(neil)
(Assignee)

Comment 15

11 years ago
> 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

11 years ago
(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)
(Assignee)

Updated

11 years ago
Attachment #221463 - Flags: superreview?(neil)
(Assignee)

Comment 17

11 years ago
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.
Attachment #222139 - Flags: superreview?(neil)

Comment 18

11 years ago
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.
(Assignee)

Comment 19

11 years ago
Created attachment 222289 [details] [diff] [review]
Addresses Neil's comments
Attachment #222289 - Flags: superreview?(neil)

Comment 20

11 years ago
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.
Attachment #222289 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

11 years ago
Attachment #222139 - Flags: superreview?(neil)
(Assignee)

Comment 21

11 years ago
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.
Attachment #222341 - Flags: approval-branch-1.8.1?(bzbarsky)

Updated

11 years ago
Attachment #222341 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Assignee)

Comment 22

11 years ago
*** Bug 335577 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 23

11 years ago
*** Bug 328273 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 24

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

Comment 25

11 years ago
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?
Attachment #222341 - Flags: approval1.8.0.5?
(Assignee)

Comment 26

11 years ago
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

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

Comment 28

11 years ago
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

11 years ago
(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

11 years ago
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.
Keywords: fixed1.8 → fixed1.8.1
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
Attachment #222341 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Comment 32

11 years ago
Had to back out for bustage
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Assignee)

Comment 33

11 years ago
*** Bug 327396 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 34

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

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.