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

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Marco Zehe (:MarcoZ)]

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.

Comment 1

[Marco Zehe (:MarcoZ)]

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

Comment 2

[Simon Paquet [:sipaq]]

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.

Comment 3

[Marco Zehe (:MarcoZ)]

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.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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...

Comment 6

[Marco Zehe (:MarcoZ)]

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.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

If we're lucky, this will fix all of the remaining label lossage.  Marco, can you give this version a try?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

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.  :-(

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

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

Removed the review requests because there's still bustage at least in All-Headers mode.

Comment 11

[Marco Zehe (:MarcoZ)]

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!

Comment 12

[Marco Zehe (:MarcoZ)]

(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.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

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

[Magnus Melin [:mkmelin]]

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 21

[Mark Banner (:standard8)]

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.

Comment 22

[Marco Zehe (:MarcoZ)]

The latest try-server build looks fine with NVDA speaking all the labels as expected. Thanks!

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch with test, v7

Mark's comments addressed; r+ carried forward.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Pushed:

http://hg.mozilla.org/releases/comm-1.9.1/rev/0dad8c61231b
http://hg.mozilla.org/comm-central/rev/10d108ff37f0

Comment 25

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091111 Shredder/3.0pre

Comment 26

[Firefox Bug Husbandry Bot]

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