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!

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

I can take this.

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

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

Nice catch. I fixed the typo.

Comment on attachment 282327 [details] [diff] [review] addresses bienvenu's comment looks good, thx for the patch.

(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 on attachment 282327 [details] [diff] [review] addresses bienvenu's comment I'll let Scott decide...

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?

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

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

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

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 on attachment 282573 [details] [diff] [review] patch to address neil's concerns. probably better to get r= from Neil...

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?

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.

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

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

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

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

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

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 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 on attachment 284337 [details] [diff] [review] patch to go in (thanks Neil) gotta change one small thing

Neil I think we both caught that at the same time. Sorry about that! This patch should be good to go.

Comment on attachment 284344 [details] [diff] [review] patch to go in thx for the patch, David

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 on attachment 284765 [details] [diff] [review] SeaMonkey port Looks okay, but I can't test if it's working with a screenreader...