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

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Joanmarie Diggs]

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!

Comment 1

[Aaron Leventhal]

This can be done by setting aaa:labelledby=ID in the XUL

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

I can take this.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 4

[David :Bienvenu]

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

Comment 5

[Scott MacGregor]

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

[David :Bienvenu]

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.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: addresses bienvenu's comment

Nice catch. I fixed the typo.

Comment 8

[David :Bienvenu]

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

looks good, thx for the patch.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(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

[David :Bienvenu]

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

I'll let Scott decide...

Comment 11

[[e-mail]]

Karsten Düsterloh <[e-mail]> 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.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 13

[[e-mail]]

(In reply to comment #12)
>Is the possible breakage a seamonkey-only concern?
In what sense? The code would be similar for either app.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: better patch: fixes an error in previous patch.

I noticed the previous patch broke awCleanupRows (erroneously used row instead of rowID)

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

(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

[[e-mail]]

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.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Neil, Nice catch thanks. Will rework.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 19

[David :Bienvenu]

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

probably better to get r= from Neil...

Comment 20

[[e-mail]]

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?

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 22

[[e-mail]]

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.

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

(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

[[e-mail]]

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.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 26

[[e-mail]]

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?

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

(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

[[e-mail]]

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

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 31

[[e-mail]]

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

Comment 32

[[e-mail]]

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

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 34

[[e-mail]]

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

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

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

gotta change one small thing

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 37

[David :Bienvenu]

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

thx for the patch, David

Comment 38

[:Gijs (he/him)]

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

Comment 39

[[e-mail]]

Attachment: SeaMonkey port

Comment 40

[Karsten Düsterloh]

Comment on attachment 284765 [details] [diff] [review]
SeaMonkey port

Looks okay, but I can't test if it's working with a screenreader...

Comment 41

[Firefox Bug Husbandry Bot]

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