1671261 - Wrong aria-labels after editing a pill and after removing pills from multiple address rows

Status: RESOLVED FIXED

(Thunderbird :: Disability Access, defect, P3)

Description

[Jean-Philippe MENGUAL]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

1. ctrl-n
2. Enter an address in the To field
3. Press enter or tab
4. Back to the address with arrow key
5. Open the context menu and choose Modify the address
6. Change it
7. Press enter
8. Back again to the address

Actual results:

While the address is changed, that may be confirmed re-doing Modify the address, which displays the good address, after step 8, the label to the screen reader still mentions the former address.

Expected results:

TB should send to the screen reader the new address contents instead of the former one.

Comment 1

[Magnus Melin [:mkmelin]]

I guess some aria-label is missing - code should be around https://searchfox.org/comm-central/rev/42a71ab4cc114ddc6bb78af20937e559b7b8e59c/mail/base/content/mailWidgets.js#1992

Comment 2

[Alex ARNAUD]

Hello,

I also confirm the issue on Thunderbird 78.x.

I think we should raise the priority of this issue for this reason: this is not possible to correct an wrongly typed address because we wouldn't know your correction works. This is for blind person like if the screen was not correctly updated for a sighted person.

Thanks in advance.

Comment 3

[Thomas D. (:thomas8)]

More bugs in this corner, let me take this.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1671261_fixAriaLabelsEditPill&RemovePills.diff

Fix aria labels when editing or removing pills, cleanup and rectify related functions.

Coding style take aways:

• As we add content to a function, let's ensure that the function name is still matching the content, it's confusing and error-prone when a function name claims to change just one address input aria label and in reality it's changing the whole row of pills and the input.
• When adding conditionals to skip some existing code, let's ensure that the scope of the conditional is large enough, and if it's large, it should probably end up as an early return for the reverse condition.

Changes in detail:

• When a pill is edited, update its aria-label.
• When pills are removed (possibly from multiple rows), update the aria-labels
  of pills in all rows as the row's pill count is in the aria-label of each pill
  and each non-custom address input. Perhaps this could be optimized, to update
  only rows where a pill got removed.
• Rename udpateAddressingInputAriaLabel() to udpateAriaLabelsOfAddressRow().
• Cleanup and rectify code flow in udpateAriaLabelsOfAddressRow().
• Rename updateStringsOfAddressingFields() to
  updateAriaLabelsAndTooltipsOfAllAddressRows().
• Improve querySelector scope of updateAriaLabelsAndTooltipsOfAllAddressRows().
• Rename updateTooltipsOfAddressingFields() to updateTooltipsOfAddressRow().
• Add, update and rectify various inline and function comments.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9181827 [details] [diff] [review]
1671261_fixAriaLabelsEditPill&RemovePills.diff

Review of attachment 9181827 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great and works like a charm, thanks for updating it.
Just a couple of nits and questions, but overall looks great.


We should rename the file to remove the **&** as it won't be easily importable for unix systems (and it might create problems when landing, or maybe not, but better safe than sorry).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4125,2 @@
>   */
> +function updateAriaLabelsAndTooltipsOfAllAddressRows() {

I'm okay with this name, but maybe let's ping Magnus for a feedback just to be sure he approves and it's not too verbose.

@@ +4125,5 @@
>   */
> +function updateAriaLabelsAndTooltipsOfAllAddressRows() {
> +  for (let row of document
> +    .querySelector("mail-recipients-area")
> +    .querySelectorAll(".address-row")) {

Do we need the first querySelector("mail-recipients-area")?
Since the address-row class is specifically only used for those elements and nothing else, I think we can avoid the extra selector, or maybe I'm missing something?

If we really need it, maybe we should use .getElementById("recipientsContainer") as first selector like we did in other locations, to keep it consistent.

Also, what do you think about adding an extra attribute to the row selector? ".address-row:not(.hidden)"
So we avoid looping through rows that we're sure don't have any pills since they're hidden.

@@ +4152,5 @@
> +  }
> +
> +  let type = row.querySelector(".address-label-container > label").value;
> +  let pills = row.querySelectorAll("mail-address-pill");
> +  let pillsCount = pills.length;

Do we need to declare a variable for the pills count?
I wonder if there's any memory benefit for this compared to tapping the length attribute when necessary.

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 9181827 [details] [diff] [review]
1671261_fixAriaLabelsEditPill&RemovePills.diff

Review of attachment 9181827 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Alessandro Castellani (:aleca) from comment #5)
> Review of attachment 9181827 [details] [diff] [review] ⧉[details] [diff] [review]:
>
> This looks great and works like a charm, thanks for updating it.
> Just a couple of nits and questions, but overall looks great.

Thanks :-))

Will fix the the nit(s) a bit later.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4125,2 @@
>   */
> +function updateAriaLabelsAndTooltipsOfAllAddressRows() {

Approved by Magnus.

@@ +4125,5 @@
>   */
> +function updateAriaLabelsAndTooltipsOfAllAddressRows() {
> +  for (let row of document
> +    .querySelector("mail-recipients-area")
> +    .querySelectorAll(".address-row")) {

I talked about this with mkmelin. Outcome:
- We still want recipient-area to be as self-contained as possible, so one day this should live in mailWidgets.js. So let's avoid getElementById("outerID") as a hint to ourselves (doesn't matter much, and I don't care much).
- let's chain the two selectors into one.
- the extra selector avoids looking in the entire composition document for address-rows because we already know that they live in mail-recipients-area.
- not(.hiden) may not work because we might still need to update the aria of the address input even if the row is empty? will think about that.

@@ +4152,5 @@
> +  }
> +
> +  let type = row.querySelector(".address-label-container > label").value;
> +  let pills = row.querySelectorAll("mail-address-pill");
> +  let pillsCount = pills.length;

There might be thousands of pills, so my assumption is that having a known variable should be faster than getting the length attribute of the pill each time. Is the length attribute static or does it get recounted every time you call it? But even if static, pill.length needs to first look for the pill in memory and then go to the length attribute, whereas the variable should be a direct memory access. Don't know for sure.

Comment 7

[Magnus Melin [:mkmelin]]

For pillsCount, I don't think you need a temp variable - the access time for that would be insignificant.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Magnus Melin [:mkmelin] from comment #7)

For pillsCount, I don't think you need a temp variable - the access time for that would be insignificant.

Insignificant yes, but the temp variable is still around 20% faster from a rough test.
https://jsbench.me/fdkgmnqs51/1

Comment 9

[Magnus Melin [:mkmelin]]

Something like a 2μs win yes.

Comment 10

[Thomas D. (:thomas8)]

Attachment: 1671261_fixAriaLabelsEditPillAndRemovePills.diff

This should take care of everyone's inputs wrt nits.

Aleca: ...should use .getElementById("recipientsContainer") as first selector like we did in other locations, to keep it consistent.

Done; that's consistent, fast, and easy to read.

Aleca: ".address-row:not(.hidden)"

No, this will fail as we need to update the aria labels for address inputs of empty rows whose pills we have just removed before we get here.

Removed temp variable pillsCount and restored pills.length (used twice) for mkmelin.

Comment 11

[Alessandro Castellani [:aleca]]

Aleca: ".address-row:not(.hidden)"

No, this will fail as we need to update the aria labels for address inputs of empty rows whose pills we have just removed before we get here.

Ah, interesting, thanks for checking as I missed this scenario.
Maybe we could, in a tiny follow up bug, implement a little improvement to this area in order to update the aria label of the address input when the user hides it and has pills that will be deleted in it.

If we do that, we can reduce the loop of the addressing fields to only those visible, improving performance especially if the user has many custom headers.
This is probably a very tiny improvement, so it's a minor enhancement with a very low priority, if we want to consider it.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9183671 [details] [diff] [review]
1671261_fixAriaLabelsEditPillAndRemovePills.diff

Review of attachment 9183671 [details] [diff] [review]:
-----------------------------------------------------------------

This looks and works great.
Thank you so much!

Comment 13

[Thomas D. (:thomas8)]

Thanks for the reviews!

Lots of flags helpfully set by reviewer, except checkin-needed-tb...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5478c71d9fe2
Fix aria labels when editing or removing pills, cleanup and rectify related functions. r=aleca

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9183671 [details] [diff] [review]
1671261_fixAriaLabelsEditPillAndRemovePills.diff

[Approval Request Comment]
Regression caused by (bug #): pills
User impact if declined: accessiblity issues when correcting an address
Testing completed (on c-c, etc.): on c-c-
Risk to taking this patch (and alternatives if risky): not risky

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9183671 [details] [diff] [review]
1671261_fixAriaLabelsEditPillAndRemovePills.diff

[Triage Comment]
Approved for beta

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 83.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/efd1472716ec

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9183671 [details] [diff] [review]
1671261_fixAriaLabelsEditPillAndRemovePills.diff

[Triage Comment]
Approved for esr78

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 78.4.1:
https://hg.mozilla.org/releases/comm-esr78/rev/450a7a3d6c2d