1589859 - Partially broken accessibility in the headers pane for a message

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect, P1)

Description

[Kostadin Kolev]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

1. Launch a screen reader like NVDA or Narrator.
2. Launch Thunderbird 68.
3. Make sure that the message preview pane is enabled.
4. Navigate to/select a folder containing a few messages.
5. Select a message from the list, so it appears in the message preview pane.
6. Use the TAB key to navigate between the controls in the headers pane (e.g. "From", "Subject", etc.) in the message preview pane.

Actual results:

Controls/fields such as "From", "To", "Date", etc. do not have labels for screen readers - only the content is read (e.g. the name of the sender is read, but not the associated with it label "From").

Expected results:

Controls/fields such as "From", "To", "Date", etc. should have labels for screen readers as they did before version 68 of Thunderbird.

Comment 1

[Jorg K (CEST = GMT+2)]

Alex and Jean-Philippe, can you confirm?

Comment 2

[Jean-Philippe MENGUAL]

Yes I confirm. I had not seen this because blind people disable the display pane, usually. I disable it and prefer opening the message with Enter. But for those using the display pane, indeed, this should be fixed.

Best regards

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks.

Comment 4

[Wayne Mery (:wsmwk)]

But this doesn't affect the standalone windows?
If it does, this is pretty serious - and hard to imagine this isn't a duplicate report.

Comment 5

[Jean-Philippe MENGUAL]

yes it does. If you:

1. Press enter on a message to open it in a new window (dont know how happens with open in tabs, I disabled it as buggy)
2. Press tab and shift tab to reach the message info pane

Result: the same as this described in this bug. You here the sender (but not the label "From"; then, "Subject" (not more), then "Me" and your address, but you dont have label

Comment 6

[Magnus Melin [:mkmelin]]

Likely same as bug 1493608.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1589859-accessibility-header.diff

This patch brings back the proper accessibility labels for all those header fields, and fixes the tests disabled in bug 1493608.

Comment 8

[Alessandro Castellani [:aleca]]

Sweet green try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=52d9eb827cb3b49073890ceb08859f265eb21e09

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9133994 [details] [diff] [review]
1589859-accessibility-header.diff

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

::: mail/base/content/mailWidgets.js
@@ +381,1 @@
>  

was role necessary?
textbox is a bit weird for this. See bug 1489748 comment 37.

@@ +2287,5 @@
>        });
>        for (let css of fixedClassed) {
>          pill.emailInput.classList.add(css);
>        }
> +      pill.emailInput.setAttribute("labelID", element.getAttribute("labelID"));

Instead of a custom labelId attribute, could this be using  aria-labelledby?

::: mail/test/browser/message-header/browser_messageHeader.js
@@ +434,5 @@
>    // make sure it loads
>    assert_selected_and_displayed(mc, curMessage);
>  
>    headersToTest.forEach(verify_header_a11y);
> +}); // disabled for now, see bug 1489748

please remove

Comment 10

[Alessandro Castellani [:aleca]]

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

was role necessary?
textbox is a bit weird for this. See bug 1489748 comment 37.

The test was looking for that role.
Based on this https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/textbox_role, the role doesn't seem to be related to anything XBL, but rather used to identify non input elements that allow free-form text.
Do we want that?
If it's not necessary I can remove that check from the test.

Instead of a custom labelId attribute, could this be using aria-labelledby?

That was my first approach but apparently aria-labelledby overrides aria-label.
I would have to remove the aria-labelledby attribute once set, that's why I went with a custom attribute.

Comment 11

[Magnus Melin [:mkmelin]]

From 1489748 comment 37 is sounds like we should not set a role for it.

Maybe it's still preferable to set the aria-labelledby and maybe nuke it if then later needed

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1589859-accessibility-header.diff

Updated!

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9134193 [details] [diff] [review]
1589859-accessibility-header.diff

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

::: mail/test/browser/message-header/browser_messageHeader.js
@@ -392,5 @@
>      `didn't find accessible element for header '${aHeaderInfo.headerName}'`
>    );
>  
> -  Assert.equal(
> -    headerAccessible.role,

should probably remove the expectedRole data for the tests too

Comment 14

[Alessandro Castellani [:aleca]]

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

Comment on attachment 9134193 [details] [diff] [review]
should probably remove the expectedRole data for the tests too

I thought I removed everything, is there any leftover?

Comment 15

[Magnus Melin [:mkmelin]]

I was referring to https://searchfox.org/comm-central/rev/e4f7cb14fda6e7c86bd5353f310105311ea78aa3/mail/test/browser/message-header/browser_messageHeader.js#234 (and the others)

Comment 16

[Magnus Melin [:mkmelin]]

I was confused.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d51e4a31fe1d
Fix accessibility labels in message pane headerarea. r=mkmelin

Comment 19

[Alessandro Castellani [:aleca]]

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=d51e4a31fe1d03d1ce4c068b522aed0376174e7a

This caused bct3 test failure which didn't appear in the try-run.
Creating a patch right now.

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1589859-bustage-fix.diff

For some strange reason, the mail-newsgroup element doesn't seem to get the aria-labelledby element.
This fixes it and those tests pass locally.
Here's an artifact try-run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ff08ca70e2786377cce0815dbfb3754eeb2cb94f

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9134801 [details] [diff] [review]
1589859-bustage-fix.diff

Empty patch

Comment 22

[Alessandro Castellani [:aleca]]

Attachment: 1589859-bustage-fix.diff

Darn, sorry about that.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9134855 [details] [diff] [review]
1589859-bustage-fix.diff

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

This is not correct, it would just be papering over the problem.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/67ab25b59f96
followup: fix mistakenly removed aria-labelledby. rs=bustage-fix

Comment 25

[Magnus Melin [:mkmelin]]

You were removing the parent attribute inside a loop.

Comment 26

[Alessandro Castellani [:aleca]]

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

You were removing the parent attribute inside a loop.

Ah, I totally missed that.
Thanks for fixing it.