If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Recent changes broke accessibility labels for "from", "subject" etc. fields when reading messages

VERIFIED FIXED in Thunderbird 3.0rc1

Status

Thunderbird
Message Reader UI
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: MarcoZ, Assigned: dmose)

Tracking

({access, regression, sec508})

Trunk
Thunderbird 3.0rc1
access, regression, sec508
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact][patch needs minor tweaks])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
The new features introduced in bug 449560 and others to speak the proper labels for "from", "to", "subject" etc. have recently broken. Screen readers will only speak the colon and the actual field contents, but no longer the type like "from". So wherever the aria-label stuff is being composed, the info of the field type is no longer retrieved correctly.

Likewise, the read-only textboxes with date and subject no longer have proper label associations.
Flags: blocking-thunderbird3?
(Reporter)

Comment 1

8 years ago
The build where this was introduced is: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090910 Shredder/3.0b4pre
(Assignee)

Updated

8 years ago
Assignee: nobody → dmose
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Will the fix for this bug have a string impact? If yes, we need a patch here ASAP as the we freeze strings for TB3 tomorrow.
(Reporter)

Comment 3

8 years ago
I don't think this will have l10n impact. I believe what happened is that some ID of the node that displays "from", "to" etc. has changed and that the reference in the patch for bug 449560 simply doesn't work any more.

Updated

8 years ago
Whiteboard: [no l10n impact]
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact] → [no l10n impact][needs patch; eta weds 10/21]
(Reporter)

Updated

8 years ago
Duplicate of this bug: 524641
(Assignee)

Comment 5

8 years ago
Created attachment 409075 [details] [diff] [review]
patch, v1

Here's a patch that I think should work.  Unfortunately, I'm having a heck of a time testing it, as my install of NVDA is very confused and not working well, and attempting to install JAWS seems to have messed up my Windows XP install.  Any testing would be extremely helpful...
Attachment #409075 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs patch; eta weds 10/21] → [no l10n impact][has patch; needs testing davidb, review Standard8]
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has patch; needs testing davidb, review Standard8] → [no l10n impact][has patch; needs testing MarcoZ, review Standard8]
(Reporter)

Comment 6

8 years ago
We're almost there! The to:, from:, and CC: etc. fields work as expected again.

There must also have been a change to the Subject: Date:, and encoding: fields. These were read-only textbox elements previously, but are now also announced as labels. These do not have the proper associations yet. It would appear that these receive some other form of treatment which isn't covered by the above patch yet.
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has patch; needs testing MarcoZ, review Standard8] → [no l10n impact][needs updated patch; ETA late Fri 10/30]
(Assignee)

Updated

8 years ago
Attachment #409075 - Flags: review?(bugzilla)
(Assignee)

Comment 7

8 years ago
Created attachment 409462 [details] [diff] [review]
patch, v2

If we're lucky, this will fix all of the remaining label lossage.  Marco, can you give this version a try?
Attachment #409075 - Attachment is obsolete: true
Attachment #409462 - Flags: ui-review?
Attachment #409462 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

8 years ago
Attachment #409462 - Flags: ui-review?(marco.zehe)
Attachment #409462 - Flags: ui-review?
Attachment #409462 - Flags: review?(mkmelin+mozilla)
Attachment #409462 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs updated patch; ETA late Fri 10/30] → [no l10n impact][has patch; needs testing MarcoZ, Standard8]
(Assignee)

Comment 8

8 years ago
OK, I've finally gotten NVDA configured in such a way that I can make sense of it.  While it still tries to pronounce things in French, which I don't understand well enough to make sense of, I've switched it to the "display virtual synthesizer", so it just shows the stuff it reads off visually, which is even better from a testing point of view.

So, in the current iteration of the patch, here's what happens:

Subject is now read this way:

subject : Foo edit
selected Foo

Date is a little bit interesting because in the visual version, it's drawn in two different ways: in the "All Headers" mode, it is drawn just like a normal header with the header name (Date:) in front of the header value.  In the "Normal Headers" mode, however, the word "date" is not rendered at all anywhere: the user is presumed to simply infer what the thing is because of the way it's visually formatted.  

In the current patch, the word date is not read aloud by the screen reader in either case.  In the "All Headers" case, it's clearly a bug, which I'll put together a patch for.  MarcoZ, what do you think we should do in the "Normal Headers" case?  Prepend

date :

or not?

Finally, I'm not sure what you're referring to with the phrase "encoding: field".  the Content-Type-Encoding header?  Something else?
(Assignee)

Comment 9

8 years ago
While working on a patch for the Date problem in "All Headers" mode, I'm finding that the JS labelElement property ends up getting set for some XUL elements pointed to via a "control" attribute, but not others.  Exactly what the pattern is is not clear to me.  :-(
(Assignee)

Comment 10

8 years ago
Comment on attachment 409462 [details] [diff] [review]
patch, v2

Removed the review requests because there's still bustage at least in All-Headers mode.
Attachment #409462 - Flags: ui-review?(marco.zehe)
Attachment #409462 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has patch; needs testing MarcoZ, Standard8] → [no l10n impact][needs updated patch dmose (eta 11/2), reviews]
(Reporter)

Comment 11

8 years ago
Hi Dan,

if it's not visually present in the case of "normal" headers mode, then we shouldn't prepend I think. We can expect the same level of smartness from a blind user.

The "encoding" is something that can show like "7bit" or the like, which is the content-transfer-encoding field which, at least for me, shows up for some messages.

Glad you got NVDA working and can use the visual synth for testing!
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs updated patch dmose (eta 11/2), reviews] → [no l10n impact][needs updated patch dmose (eta 11/3), reviews]
(Reporter)

Comment 12

8 years ago
(In reply to comment #11)
> The "encoding" is something that can show like "7bit" or the like, which is the
> content-transfer-encoding field which, at least for me, shows up for some
> messages.

This is due to the pref mailnews.headers.extraExpandedHeaders being set to true in my profile. I had Enigmail installed once here, and it changes that pref.
(Assignee)

Comment 13

8 years ago
Created attachment 410722 [details] [diff] [review]
patch, v3 (WIP)

Work-in-progress.  Changing from labels to readonly text boxes is done for some headers, but not yet all of them.  In addition to finishing that, I still need to figure out why some XUL elements (e.g. Date) aren't getting the header field names prepended, but others are not, and I also need to write tests.  This may be related to the labelElement lossage mentioned previously, which may also be related to non-standard message headers in All Headers mode and to the fact that we're ending up with two Date headers in All Headers mode as well.

I suspect we could get away without fixing the All Headers mode bugs, since it's non-default, if push came to shove.  We'll see how far I get tomorrow.
Attachment #409462 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs updated patch dmose (eta 11/3), reviews] → [no l10n impact][needs updated patch dmose, reviews]
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs updated patch dmose, reviews] → [no l10n impact][needs updated patch dmose (eta 11/6), reviews]
(Assignee)

Comment 14

8 years ago
Created attachment 410890 [details] [diff] [review]
patch, v4

This fixes the big issues that I can see in the default message header.  While I'd like to get the "all headers" mode fixed up too, I don't think those changes truly block Thunderbird 3.

My next step is to put together some mozmill tests as a separate patch, but I thought it would be worth allowing you to get started on the review early.
Attachment #410722 - Attachment is obsolete: true
Attachment #410890 - Flags: review?(bwinton)
Whiteboard: [no l10n impact][needs updated patch dmose (eta 11/6), reviews] → [no l10n impact][needs review bwinton]
Attachment #410890 - Flags: review?(bwinton) → review+
Comment on attachment 410890 [details] [diff] [review]
patch, v4

I’ve already mentioned most of these over IRC, but just to get them down in writing, and make sure I didn't miss anything:

>+++ b/mail/base/content/mailWidgets.xml
>@@ -175,21 +175,43 @@
>+        <setter> 

There’s a trailing space on the above line.

>+          let valueNode = document.getAnonymousElementByAttribute(this,
>+                                                                  'anonid',
>+                                                                  'headerValue');

I’ld prefer double-quotes for the string parameters (cause we're no longer in an attribute, so we can).

>@@ -252,17 +274,17 @@
>-            var ariaLabel = this.getAttribute("label") + ": " +
>+            var ariaLabel = aEmailNode.getAttribute("headerName") + " : " +

You’ve added in a space before the ":" here.  I don’t know if it matters to screen readers, but it looks kind of odd.
"From: Blake" vs. "From : Blake".

(You also did it above in the line:
    let ariaLabel = this.labelElement.value + " : " + val;
 which leads me to believe it was intentional.)

>+++ b/mail/base/content/msgHdrViewOverlay.xul
>@@ -377,17 +376,18 @@
>-              <label id="dateLabel" class="dateLabel" flex="1"/>
>+              <label id="dateLabel" class="dateLabel" flex="1" role="textbox"
>+                     aria-readonly="true"/>

While I’m here, do we need to do anything for the Newsgroups and MessageIds headers?

r=me with the nits fixed.

Thanks,
Blake.
(Assignee)

Comment 16

8 years ago
Created attachment 411206 [details] [diff] [review]
patch with test, v5 (WIP)

This version has some tests working, and a bunch of tests still in progress.  Writing the tests caused me to find a bug in a11y for URL fields (eg Website), which I've fixed.  All of Blake's comments have been addressed except for Newsgroups and Message-Ids.  I think the high-order bit at this point is getting the tests done.  With luck, it'll be easy to address those issues once the tests are working.
Attachment #410890 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][needs review bwinton] → [no l10n impact][patch works; tests in-progress dmose]
(Assignee)

Comment 17

8 years ago
Try server builds have been kicked off for Marco and/or davidb to test-drive.  <http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry> is the page to watch...
(Assignee)

Comment 18

8 years ago
Created attachment 411361 [details] [diff] [review]
patch with test, v6

Gotta love this test-writing.  I found a number of bugs while writing the tests, including giving non-localized strings to the screen readers and doing the wrong thing for various different types of headers.  The patch has them fixed.  A few notes:

* the thing where we have spaces in the dtd file after the header names is why there are a bunch of slice()s in the both the code and the tests.  I'll file a bug to fix the strings and use CSS for that spacing so we can get rid of this hackiness post 3.0.

* I didn't get a chance to add a11y for the newsgroup References header.  Rather than filing a bug to fix that, I'm going to file a bug to make the current References link UI go away, because it's likely to be incomprehensible to the vast majority of users, and we really just want Newsgroup conversations to be treated like any other conversations.
Attachment #411206 - Attachment is obsolete: true
Attachment #411361 - Flags: review?(bugzilla)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][patch works; tests in-progress dmose] → [no l10n impact][has patch; needs review Standard8]
(Assignee)

Comment 19

8 years ago
I've just uploaded a new patch to the try server, so within a few hours there should be builds to try.  Keep an eye on <http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry>...

Comment 20

8 years ago
Newsgroup conversations are different since conversations can move between groups, and then you can't easily follow the whole discussion without going to the original messages which would be linked.
Comment on attachment 411361 [details] [diff] [review]
patch with test, v6


>diff --git a/mail/test/mozmill/message-header/test-message-header.js b/mail/test/mozmill/message-header/test-message-header.js
>diff --git a/mail/test/mozmill/shared-modules/test-folder-display-helpers.js b/mail/test/mozmill/shared-modules/test-folder-display-helpers.js

nit: the changes in these files contain quite a bit of whitespace on the end of lines.

>+function test_a11y_attrs() {
...
>+  headersToTest.forEach(verify_header_a11y);  

This fails on mac - headersToTest is undefined because Ci.nsIAccessibleRole is undefined. Adding 'if (!"nsIAccessibleRole" in Components.interfaces)' before that line worked for me, though may not be the best check.

r=Standard8 with those issues fixed.
Attachment #411361 - Flags: review?(bugzilla) → review+
(Reporter)

Comment 22

8 years ago
The latest try-server build looks fine with NVDA speaking all the labels as expected. Thanks!
Whiteboard: [no l10n impact][has patch; needs review Standard8] → [no l10n impact][patch needs minor tweaks]
(Assignee)

Comment 23

8 years ago
Created attachment 411456 [details] [diff] [review]
patch with test, v7

Mark's comments addressed; r+ carried forward.
Attachment #411361 - Attachment is obsolete: true
Attachment #411456 - Flags: review+
(Assignee)

Comment 24

8 years ago
Pushed:

http://hg.mozilla.org/releases/comm-1.9.1/rev/0dad8c61231b
http://hg.mozilla.org/comm-central/rev/10d108ff37f0
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

8 years ago
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091111 Shredder/3.0pre
Status: RESOLVED → VERIFIED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.