LABELLED_BY, LABEL_FOR relations should be set for header fields in the Thunderbird message composition window

RESOLVED FIXED

Status

Thunderbird
Message Compose Window
--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Joanmarie Diggs, Assigned: davidb)

Tracking

(Blocks: 2 bugs, {access, sec508})

Trunk
access, sec508
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: orca:urgent)

Attachments

(2 attachments, 8 obsolete attachments)

5.79 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.60 KB, patch
Karsten Düsterloh
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
In Thunderbird's message composition window, fields in the message header are functionally labeled by the combo boxes on their left (e.g. To, CC, BCC, and so on).  Setting/exposing official LABELLED_BY/LABEL_FOR relationships between these objects via AT-SPI would cause screen readers such as Orca to automatically speak the "label" for each of these fields.  

Thanks!
Blocks: 370226

Comment 1

10 years ago
This can be done by setting aaa:labelledby=ID in the XUL
Assignee: aaronleventhal → nobody
Component: Disability Access APIs → Message Compose Window
OS: Linux → All
Product: Core → Thunderbird
QA Contact: accessibility-apis → message-compose
Hardware: PC → All
I can take this.
Assignee: nobody → david.bolter

Updated

10 years ago
Blocks: 396347
Created attachment 282193 [details] [diff] [review]
possible fix (tested on linux using accerciser; works)

Moves setting of To, Reply-to etc. popup element id and the corresponding input element id in the addressingWidget to one function: awSetInputAndPopupID (there is a precedent function named awSetInputAndPopupValue).

This allows us to add/update/set the attribute "aaa:labelledby" required for accessibility in one place essentially.

I don't know how to run the unit tests (yet) so I'm not certain if I've broken anything with this patch.

Note: no suite patch yet.
Attachment #282193 - Flags: review?(bienvenu)

Comment 4

10 years ago
Comment on attachment 282193 [details] [diff] [review]
possible fix (tested on linux using accerciser; works)

I haven't had a chance to try this, but I think we'll want Scott's eyes on this too...
Attachment #282193 - Flags: superreview?(mscott)

Comment 5

10 years ago
Neil might want to take a look at this too. He's the most knowledgeable for autocomplete related stuff and he might want to pick this up for seamonkey too.

Comment 6

10 years ago
Comment on attachment 282193 [details] [diff] [review]
possible fix (tested on linux using accerciser; works)

r=bienvenu, with one very important caveat:

   if (rowNumber >= 0)
-  {
-    inputElem.setAttribute("id", "addressCol2#" + rowNumber);
-    popupElem.setAttribute("id", "addressCol1#" + rowNumber);
-  }
+    awSetInputandPopupId(inputElem, popupElem, rowNumber);


that last line needs to be awSetInputAndPopupId, not awSetInputandPopupId, or else bringing up a new compose window doesn't work.
Attachment #282193 - Flags: review?(bienvenu) → review+
Created attachment 282327 [details] [diff] [review]
addresses bienvenu's comment

Nice catch. I fixed the typo.
Attachment #282193 - Attachment is obsolete: true
Attachment #282327 - Flags: review?(bienvenu)
Attachment #282193 - Flags: superreview?(mscott)

Comment 8

10 years ago
Comment on attachment 282327 [details] [diff] [review]
addresses bienvenu's comment

looks good, thx for the patch.
Attachment #282327 - Flags: review?(bienvenu) → review+
(In reply to comment #8)
> (From update of attachment 282327 [details] [diff] [review])
> looks good, thx for the patch.
> 

Cool, did you still want sr+ from mscott?  (It got erased when I obsoleted the original patch. I probably should have waited before updating the patch.)

Comment 10

10 years ago
Comment on attachment 282327 [details] [diff] [review]
addresses bienvenu's comment

I'll let Scott decide...
Attachment #282327 - Flags: superreview?(mscott)

Updated

10 years ago
Blocks: 360488

Comment 11

10 years ago
Karsten Düsterloh <mnyromyr@tprac.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |360488

This sometimes works better than CCing me, but in this case I had a quick peek at the patch and I'm worried that it breaks the address list editor.
Thanks for taking a peek Neil.  I'm left wondering if there is anything I can do? Is the possible breakage a seamonkey-only concern?
Status: NEW → ASSIGNED

Comment 13

10 years ago
(In reply to comment #12)
>Is the possible breakage a seamonkey-only concern?
In what sense? The code would be similar for either app.
Created attachment 282546 [details] [diff] [review]
better patch: fixes an error in previous patch.

I noticed the previous patch broke awCleanupRows (erroneously used row instead of rowID)
Attachment #282327 - Attachment is obsolete: true
Attachment #282327 - Flags: superreview?(mscott)
(In reply to comment #13)
> (In reply to comment #12)
> >Is the possible breakage a seamonkey-only concern?
> In what sense? The code would be similar for either app.
> 

Similar yes, but I'm still too noob to know just how similar. Can you describe how the address list editor is broken (or might be)? I've tried it out in tbird and it seems ok.

Comment 16

10 years ago
Comment on attachment 282546 [details] [diff] [review]
better patch: fixes an error in previous patch.

> function awDeleteRow(rowToDelete)
> {
>   /* When we delete a row, we must reset the id of others row in order to not break the sequence */
>   var maxRecipients = top.MAX_RECIPIENTS;
>   awRemoveRow(rowToDelete);
> 
>   var numberOfCols = awGetNumberOfCols();
>   for (var row = rowToDelete + 1; row <= maxRecipients; row ++)
>+  {
>     for (var col = 1; col <= numberOfCols; col++)
>+    {
>       awGetElementByCol(row, col).setAttribute("id", "addressCol" + (col) + "#" + (row-1));
>+    }
>+    awUpdateInputAndPopupRelations(awGetInputElement(row), awGetPopupElement(row));
This looks as if it won't work with the address list editor, because its rows have no popup elements.
Neil, Nice catch thanks. Will rework.
Created attachment 282573 [details] [diff] [review]
patch to address neil's concerns.

It seems that this addressing widget is used in a few places in different ways; which I hadn't realized. This patch should fit in better I think. Please let me know if I've missed anything.
Attachment #282546 - Attachment is obsolete: true
Attachment #282573 - Flags: review?(bienvenu)

Comment 19

10 years ago
Comment on attachment 282573 [details] [diff] [review]
patch to address neil's concerns.

probably better to get r= from Neil...
Attachment #282573 - Flags: superreview?(bienvenu)
Attachment #282573 - Flags: review?(neil)
Attachment #282573 - Flags: review?(bienvenu)

Comment 20

10 years ago
Comment on attachment 282573 [details] [diff] [review]
patch to address neil's concerns.

>+function awSetElementLabelledBy(elem, labelElem)
>+{
>+  if (elem && labelElem)
I think your callers already seem to check this where necessary.

>+    elem.setAttribute("aaa:labelledby", labelElem.getAttribute('id'));
This is wrong, you need to use setAttributeNS(..., "labelledby", ...);

>+  popupElem.setAttribute("id", "addressCol1#" + rowNumber);
>+  inputElem.setAttribute("id", "addressCol2#" + rowNumber);
Note: You can probably use .id as a shortcut for .[gs]etAttribute("id", ...);

>-      inputElem.setAttribute("id", "addressCol2#" + rowID);
>-      awGetPopupElement(row).setAttribute("id", "addressCol1#" + rowID);
>+      awSetInputAndPopupId(inputElem, popupElem, rowID);
I don't see where you set popupElem.

>+    var inputElem = awGetPopupElement(row);
>+    var popupElem = awGetInputElement(row);
Wrong way around?
Attachment #282573 - Flags: review?(neil) → review-
Created attachment 282771 [details] [diff] [review]
fixes sleepy hacking from last patch

New patch based on Neil's latest comments, this patch should fix all points:
 * redundant checks for null.
 * now uses proper namespaced SetAttributeNS call
 * using .id works for me (I normally would use this but couldn't find precedent for directly accessing .id in this file)
 * fixes remaining two issues (popupElem and switched naming) - I'm embarrassed.

This patch tests ok for me using the compose window and the address list but I'm not sure how best to debug javascript on tbird. The error console didn't seem to have anything pertinent.
Attachment #282573 - Attachment is obsolete: true
Attachment #282771 - Flags: review?(neil)
Attachment #282573 - Flags: superreview?(bienvenu)

Comment 22

10 years ago
Sigh... why do I never learn not to look at addressing widget code.

I'm really tempted to suggest that abMailListDialog.js should have its own version of awDeleteRow thus avoiding having to do any of these checks at all.
(In reply to comment #22)
> Sigh... why do I never learn not to look at addressing widget code.

:)  (I thought it was just me)

> I'm really tempted to suggest that abMailListDialog.js should have its own
> version of awDeleteRow thus avoiding having to do any of these checks at all.

It makes sense to me. Should we put this patch in in the interim, or do the work here? Or...?

Comment 24

10 years ago
Comment on attachment 282771 [details] [diff] [review]
fixes sleepy hacking from last patch

Giving the mail list dialog its own awDeleteRow implementation will mean that awSetElementLabelledBy will only have one caller, so it can be merged into awSetInputAndPopupId, and it won't have to do any null checks.

>+function awSetInputAndPopupId(inputElem, popupElem, rowNumber)
>+{
>+  if (popupElem)
>+    popupElem.id = "addressCol1#" + rowNumber;
>+
>+  if (inputElem)
>+    inputElem.id = "addressCol2#" + rowNumber;
>+
>+  awSetElementLabelledBy(inputElem, popupElem);
These null checks are unnecessary as all of awSetInputAndPopupId's callers pass in existing elements.
Created attachment 283246 [details] [diff] [review]
Two kinds of row delete updating.

It looks like there is a similar chain for both abMailListDialog.xul and messengercompose.xul handlers for onkeydown to call:

awRecipientKeyDown[1]->awDeleteHit->awDeleteRow

What I've done here in order to retain the common code in the chain is to pass along another function argument and then in awDeleteRow perform one of two types of id updating accordingly. This widget already seems to know/guess/assume information about its context... which is unfortunate.

Anyways, let me know if this is what you had in mind. This will be an important fix to get in for accessibility.

[1] there is a call to awKeyDown via the composer side bar which can call awDeleteHit but that is handled in this patch appropriately I think.
Attachment #282771 - Attachment is obsolete: true
Attachment #283246 - Flags: review?(neil)
Attachment #282771 - Flags: review?(neil)

Updated

10 years ago
Flags: blocking-thunderbird3?
Keywords: sec508
Whiteboard: orca:ugent

Updated

10 years ago
Whiteboard: orca:ugent → orca:urgent

Comment 26

10 years ago
Comment on attachment 283246 [details] [diff] [review]
Two kinds of row delete updating.

Sorry for the lateness of review, I happened to be out all day Tuesday last week and I subsequently forgot about the patch :-(

>-function awDeleteRow(rowToDelete)
>+function awDeleteRow(rowToDelete, updateLabelledBy)
> {
>   /* When we delete a row, we must reset the id of others row in order to not break the sequence */
>   var maxRecipients = top.MAX_RECIPIENTS;
>   awRemoveRow(rowToDelete);
> 
>-  var numberOfCols = awGetNumberOfCols();
>-  for (var row = rowToDelete + 1; row <= maxRecipients; row ++)
>-    for (var col = 1; col <= numberOfCols; col++)
>-      awGetElementByCol(row, col).setAttribute("id", "addressCol" + (col) + "#" + (row-1));
>+  if (updateLabelledBy)
>+  {
>+    // assume 2 column update (input and popup)
>+    for (var row = rowToDelete + 1; row <= maxRecipients; row ++)
>+      awSetInputAndPopupId(awGetInputElement(row), awGetPopupElement(row), row);
>+  }
>+  else
>+  {
>+    // update only the id's for all remaining cells
>+    var numberOfCols = awGetNumberOfCols();
>+    for (var row = rowToDelete + 1; row <= maxRecipients; row ++)
>+      for (var col = 1; col <= numberOfCols; col++)
>+        awGetElementByCol(row, col).setAttribute("id", "addressCol" + (col) + "#" + (row-1));
>+  }
> 
>   awTestRowSequence();
> }
I'm not keen on this... is it possible to put a separate awDeleteRow in abMailListDialog.js instead?
(In reply to comment #26)
> I'm not keen on this... is it possible to put a separate awDeleteRow in
> abMailListDialog.js instead?
> 

I considered that; since I'm not overly keen on this solution eiter. I do think it is possible but would require some changes to the event handling... possibly adding more/similar complexity.

Comment 28

10 years ago
(In reply to comment #27)
>(In reply to comment #26)
>>is it possible to put a separate awDeleteRow in abMailListDialog.js instead?
>I do think it is possible but would require some changes to the event handling
Sorry, but I don't see why it would need any event handling changes; the event handlers should automatically call the correct version.
(In reply to comment #28)
> (In reply to comment #27)
> >(In reply to comment #26)
> >>is it possible to put a separate awDeleteRow in abMailListDialog.js instead?
> >I do think it is possible but would require some changes to the event handling
> Sorry, but I don't see why it would need any event handling changes; the event
> handlers should automatically call the correct version.
> 

OK, I'll give it a whirl.
Created attachment 284177 [details] [diff] [review]
address book handles own row deletion (see comment below)

Neil, you're right of course, this seems cleaner, although I did end up having to duplicate a bit of code in the chain (as I had worried about in comment 25). Anyways, this tests ok for me. Thanks for your patience on this one.
Attachment #283246 - Attachment is obsolete: true
Attachment #284177 - Flags: review?(neil)
Attachment #283246 - Flags: review?(neil)
Severity: normal → major

Comment 31

10 years ago
Comment on attachment 284177 [details] [diff] [review]
address book handles own row deletion (see comment below)

>-                   onkeydown="awRecipientKeyDown(event, this);"
>+                   onkeydown="handleKeyDown(this, event);"
You don't need this...

>-                   onkeydown="awRecipientKeyDown(event, this);"
>+                   onkeydown="handleKeyDown(this, event);"
Or this...

>-      awGetElementByCol(row, col).setAttribute("id", "addressCol" + (col) + "#" + (row-1));
>+    awSetInputAndPopupId(awGetInputElement(row), awGetPopupElement(row), row);
You're missing the - 1 on the last parameter. r=me with this fixed.

>+function handleKeyDown(element, event)
>+{
>+  switch(event.keyCode) {
>+  case 46:
>+  case 8:
>+    /* do not query directly the value of the text field else the autocomplete widget could potentially
>+       alter it value while doing some internal cleanup, instead, query the value through the first child
>+    */
>+    if (!element.value)
>+      awDeleteHit(element);
>+    
>+    event.stopPropagation();
>+    break;
>+  }
>+}
Or this...

>+function awDeleteHit(inputElement)
>+{
>+  var row = awGetRowByInputElement(inputElement);
>+
>+  /* 1. don't delete the row if it's the last one remaining, just reset it! */
>+  if (top.MAX_RECIPIENTS <= 1)
>+  {
>+    inputElement.value = "";
>+    return;
>+  }
>+
>+  /* 2. Set the focus to the previous field if possible */
>+  if (row > 1)
>+    awSetFocus(row - 1, awGetInputElement(row - 1))
>+  else
>+    awSetFocus(1, awGetInputElement(2))   /* We have to cheat a little bit because the focus will */
>+                                          /* be set asynchronusly after we delete the current row, */
>+                                          /* therefore the row number still the same! */
>+
>+  /* 3. Delete the row */
>+  awDeleteRow(row);
>+}
Or this. (But you do need awDeleteRow.)
Attachment #284177 - Flags: review?(neil) → review+

Comment 32

10 years ago
(In reply to comment #30)
>Thanks for your patience on this one.
You beat me to it, I was about to thank you for your patience :-)
Created attachment 284337 [details] [diff] [review]
patch to go in (thanks Neil)

Wow that was a lot simpler than I thought. Neil the light has finally gone on. Thanks for guiding me to the correct solution.
Attachment #284337 - Flags: review?(neil)

Comment 34

10 years ago
Comment on attachment 284337 [details] [diff] [review]
patch to go in (thanks Neil)

>-      awGetElementByCol(row, col).setAttribute("id", "addressCol" + (col) + "#" + (row-1));
>+    awSetInputAndPopupId(awGetInputElement(row), awGetPopupElement(row), row);
You forgot to fix this...
Attachment #284337 - Flags: review?(neil) → review-
Comment on attachment 284337 [details] [diff] [review]
patch to go in (thanks Neil)

gotta change one small thing
Attachment #284337 - Flags: review-
Created attachment 284344 [details] [diff] [review]
patch to go in

Neil I think we both caught that at the same time. Sorry about that! This patch should be good to go.
Attachment #284337 - Attachment is obsolete: true
Attachment #284344 - Flags: approval-thunderbird3?
Attachment #284344 - Flags: approval-thunderbird3? → superreview+
Attachment #284344 - Flags: superreview+ → superreview?
Attachment #284344 - Flags: superreview? → superreview?(bienvenu)

Comment 37

10 years ago
Comment on attachment 284344 [details] [diff] [review]
patch to go in

thx for the patch, David
Attachment #284344 - Flags: superreview?(bienvenu) → superreview+
Keywords: checkin-needed

Comment 38

10 years ago
Checking in mozilla/mail/components/compose/content/addressingWidgetOverlay.js;
/cvsroot/mozilla/mail/components/compose/content/addressingWidgetOverlay.js,v  <--  addressingWidgetOverlay.js
new revision: 1.13; previous revision: 1.12
done
Checking in mozilla/mail/components/compose/content/messengercompose.xul;
/cvsroot/mozilla/mail/components/compose/content/messengercompose.xul,v  <--  messengercompose.xul
new revision: 1.109; previous revision: 1.108
done
Checking in mozilla/mailnews/addrbook/resources/content/abMailListDialog.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abMailListDialog.js,v  <--  abMailListDialog.js
new revision: 1.51; previous revision: 1.50
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #284177 - Attachment is obsolete: true

Comment 39

10 years ago
Created attachment 284765 [details] [diff] [review]
SeaMonkey port
Attachment #284765 - Flags: review?(mnyromyr)

Comment 40

10 years ago
Comment on attachment 284765 [details] [diff] [review]
SeaMonkey port

Looks okay, but I can't test if it's working with a screenreader...
Attachment #284765 - Flags: review?(mnyromyr) → review+
No longer blocks: 360488
Flags: blocking-thunderbird3?
You need to log in before you can comment on or make changes to this bug.