1764652 - Convert message header XUL custom elements to HTML custom elements

Status: RESOLVED FIXED

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

Description

[Alessandro Castellani [:aleca]]

Those custom elements have a very convoluted structure and they create a bunch of problems when resizing the window.
Let's clean them up and convert them into simpler HTML elements.

Comment 1

[newsfan]

Wasn't something like this already done here in bug 1589861 comment #32 and later backed out? Bug 1589861 comment #58 said: "realistically much of this is going to be very similar in the end".

Comment 2

[Alessandro Castellani [:aleca]]

Similar but done differently and a bit better.
That patch was backed out because it created a bit of a mess with the grid structure of the message header.
This bug will only exclusively focus on those custom elements, without changing the HTML structure of the message header, or the labels.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: Bug 1764652 - Replace XUL mail-multi-emailheaderfield, mail-headerfield, and mail-newsgroup with HTML custom elements. r=mkmelin,henry

Comment 4

[Alessandro Castellani [:aleca]]

I started working on this and decided to upload an early WIP patch without going to forward before gathering some feedback.

I'm focusing on the rows that need to handle multiple addresses as well as the more button.
The initial idea is to have this type of structure:

div
  ul
    li > address
    li > address
    ...
  button.more

Does this make sense semantically?
Does using the <address> element makes sense to wrap the email address?
Any suggestions or recommendations?

Comment 5

[Magnus Melin [:mkmelin]]

I would use <ol> instead of <ul> since it's (loosely) ordered addresses.
You can use <address>, but there might not be a need for it. If we can get by with simply using the link, then the extra box wouldn't add any value.

Comment 6

[Henry Wilkes (they/them) [:henry-x]]

I think the proposal would work well for a static document, but these elements require some interactivity. My initial thought is we want:

1. Some sort of list.
2. Each address has some interactive controls, which require focus.
3. Ideally we could combine the addresses into a single widget that controls the focus, so each address isn't a focus stop.
4. We might want to allow for multi-selection (e.g. to address https://thunderbird.topicbox.com/groups/ux/Tfec8f7aebbb89bdc).
5. We might want to combine "To", "Cc", "Reply-To", etc into a single widget.

Therefore, if the list was fully expanded, I think the "Grouped lists" widget I'm planning in bug 1752532 comment 9 might work quite well. Specifically, it would look like

<ul is="grouped-list" role=listbox">
  <li role="none">
    <span id="toHeading">To</span>
    <ul role="group" aria-labeledby="toHeading">
      <li role="option">first@server</li>
      <li role="option">second@server</li>
    </ul>
  </li>
  <li role="none">
    <span id="ccHeading">CC</span>
    <ul role="group" aria-labeledby="ccHeading">
      <li role="option">third@server</li>
    </ul>
  </li>
  <li role="none">
    <span id="replyToHeading">Reply-To</span>
    <ul role="group" aria-labeledby="replyToHeading">
      <li role="option">reply@server</li>
    </ul>
  </li>
</ul>

However, I'm not sure how well this would work in the "collapsed" state shown here https://bug1556261.bmoattachments.org/attachment.cgi?id=9243994 We could try putting the "3 Cc", "1 Reply to" and "more" buttons as list "option"s in no group at the end and see how that runs on a screen reader.

This would mean that bug 1752532, which would provide the API to construct such a structure, would be blocking. But we could work together to try and synchronise what API you would need for this widget.

Regarding using a wrapping <address>. I don't think it is quite appropriate. It seems to be more limited in scope than its name might imply: it is meant to be linked to an article or body https://html.spec.whatwg.org/multipage/sections.html#the-address-element

Comment 7

[Alessandro Castellani [:aleca]]

Thanks both for the valuable feedback.

(In reply to Henry Wilkes [:henry] from comment #6)

However, I'm not sure how well this would work in the "collapsed" state shown here https://bug1556261.bmoattachments.org/attachment.cgi?id=9243994 We could try putting the "3 Cc", "1 Reply to" and "more" buttons as list "option"s in no group at the end and see how that runs on a screen reader.

I'm gonna temporarily defer this change (collapsing all recipients row into 1) for later since it needs more work to guarantee proper a11y.
So we can start with a simpler structure for now and improve it later.

This would mean that bug 1752532, which would provide the API to construct such a structure, would be blocking. But we could work together to try and synchronise what API you would need for this widget.

I would love to use the new widget from that bug, so definitely that's the final objective.
For now, I think I'll implement a simpler structure to move things forward, and later we can adapt it to the new widget in order to not being a blocker.

This is the structure I will use for each recipient row:

<div>
    <span id="toHeading">To</span>
    <ul aria-labeledby="toHeading">
      <li>first@server</li>
      <li>second@server</li>
    </ul>
</div>

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f4fb3a75992a
Replace XUL mail-multi-emailheaderfield, mail-headerfield, and mail-newsgroup with HTML custom elements. r=mkmelin,henry

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: Bug 1764652 - Fix hover sate of address book indicator and other CSS improvements in message header. r=henry,Paenglab

• Improves the hover state of the address book indicator
• Use a consistent outline on :focus-visible
• Remove outline when :hover and :focus-visible for all elemenets

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1764652 follow-up - Fix leaked observer. r=aleca

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/724a35930277
follow-up - Fix leaked observer. r=aleca

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: Bug 1764652 - Add tests for message header customization. r=darktrojan

Comment 13

[Henry Wilkes (they/them) [:henry-x]]

Reminder that we need to prevent focus being lost when the recipient list is refreshed. In particular when the MORE button is pressed. https://phabricator.services.mozilla.com/D144358#inline-803010

@aleca Have you already written a patch for this? Otherwise I could put one together.

Another thing I noticed is that we're setting an aria-label on the header-recipient list items using the fullAddress https://hg.mozilla.org/comm-central/rev/f4fb3a75992a#l10.242 However, the fullAddress is not necessarily the displayed text and it is missing the "(not) in address book" information. I think your previous approach of setting aria-label on the <ol> element worked better and saved reading out the field each time for every item in the list. I imagine the reason you changed it was because this was the old behaviour and one of the tests was testing for it. However, we now have the added context of a "list" which we did not have before.

So Basically, in version 91 each Tab stop for a list of two recipienst would read as

1. "To: My Contact <[e-mail]>",
2. "Address is in the addressbook",
3. "To: [e-mail]",
4. "Address is not in the addressbook".

Note that in 91, an address in the addressbook was still shown as the full address, rather than the contact name, so the accessible name matched the displayed text.

Currently, each Tab stop reads as

1. "List with 2 items. To: My Contact <[e-mail]>. One of two.",
2. "To: [e-mail]. Two of two".

And with aria-label only on the <ol>, each Tab stop would instead reads as

1. "To. List with 2 items. My Contact. In the address book. One of two.",
2. "[e-mail]. Not in the addressbook. Two of two".

The latter correspond to what a we display visually, and takes the semantics into account.

If you want, I can put together a patch for this as well.

Comment 14

[Alessandro Castellani [:aleca]]

(In reply to Henry Wilkes [:henry] from comment #13)

Reminder that we need to prevent focus being lost when the recipient list is refreshed. In particular when the MORE button is pressed. https://phabricator.services.mozilla.com/D144358#inline-803010

@aleca Have you already written a patch for this? Otherwise I could put one together.

I haven't written anything for that front.
If you have the bandwidth, it would be great.

Another thing I noticed is that we're setting an aria-label on the header-recipient list items using the fullAddress https://hg.mozilla.org/comm-central/rev/f4fb3a75992a#l10.242 However, the fullAddress is not necessarily the displayed text and it is missing the "(not) in address book" information. I think your previous approach of setting aria-label on the <ol> element worked better and saved reading out the field each time for every item in the list. I imagine the reason you changed it was because this was the old behaviour and one of the tests was testing for it. However, we now have the added context of a "list" which we did not have before.

I changed that to respect the "accessible" strings we were expecting from the tests.
That was a temporary step, indeed we need to improve the whole area and now we have the flexibility to do so (another patch to convert the remaining XUL headers is coming).

So Basically, in version 91 each Tab stop for a list of two recipienst would read as

1. "To: My Contact <[e-mail]>",
2. "Address is in the addressbook",
3. "To: [e-mail]",
4. "Address is not in the addressbook".

Note that in 91, an address in the addressbook was still shown as the full address, rather than the contact name, so the accessible name matched the displayed text.

Currently, each Tab stop reads as

1. "List with 2 items. To: My Contact <[e-mail]>. One of two.",
2. "To: [e-mail]. Two of two".

And with aria-label only on the <ol>, each Tab stop would instead reads as

1. "To. List with 2 items. My Contact. In the address book. One of two.",
2. "[e-mail]. Not in the addressbook. Two of two".

The latter correspond to what a we display visually, and takes the semantics into account.

If you want, I can put together a patch for this as well.

That would be awesome!

Comment 15

[Alessandro Castellani [:aleca]]

Henry, if you implement the persistent tab focus, keep in mind that we need to make those toolbar buttons accessible in the focus ring.
The initial idea is:

• First focus lands on the first button
• Use arrows or Home/End to move between buttons
• Tab moves the focus to the adjacent element (not button)

Very similar to the spaces toolbar implementation.

Also, something that needs a lot of improvement is how to handle the focus on the first place.

• Should the focus first reach the whole header to we can feed screen readers a summarized overview of sender + title + actions?
• What's the correct tab cycle? Sender buttons > recipients, or Buttons > sender > recipients
• Do we want to allow moving the focus to the address book indicator icon of a recipient? The whole recipient is focused via Tab but pressing arrow left/right will move the focus between the text content and the AB indicator?

Of course all of this doesn't need to happen all at once, and we can postpone some of this more fine tuned improvements after 102, considering that the message header as always been in this pretty horrible state for years.

Comment 16

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Alessandro Castellani [:aleca] from comment #15)

Henry, if you implement the persistent tab focus, keep in mind that we need to make those toolbar buttons accessible in the focus ring.
The initial idea is:

• First focus lands on the first button
• Use arrows or Home/End to move between buttons
• Tab moves the focus to the adjacent element (not button)

Very similar to the spaces toolbar implementation.

I'm not working on this in my upcoming patch, I'm just going to stop the focus being lost when you click the MORE button.

But this is what should happen in the future.

Also, something that needs a lot of improvement is how to handle the focus on the first place.

• Should the focus first reach the whole header to we can feed screen readers a summarized overview of sender + title + actions?

I don't think so. Just like that the screen reader will not just read out all the content of the focused element, it will only read its accessible name, and reading out the entire content would be bad anyway.

• What's the correct tab cycle? Sender buttons > recipients, or Buttons > sender > recipients

I have been thinking in the back of my mind about what this area would look like semantically. This header seems to have developed from a mix of information shoved in places where there was a gap, so it is not easily dissected. But currently, I'm thinking the structure, in DOM order, should be

1. The button toolbar.
2. A listbox widget that contains all the emails grouped under "From", "To", "Cc", etc. See comment 6. Maybe use a grid if we want to separate the name from the addressbook icon, but this might be over complicating it.
3. A vertical grid widget with a heading and value cell on each row to collect all the other header fields. Maybe include the time, tags and the encryption "button", but I'm unsure about this.
4. The subject as a heading element that labels the document. I'll have to think about this last one some more, partly because a focusable heading might be odd.

• Do we want to allow moving the focus to the address book indicator icon of a recipient? The whole recipient is focused via Tab but pressing arrow left/right will move the focus between the text content and the AB indicator?

No, I don't think it should receive focus. The addressbook indicator's click behaviour is redundant because it essentially just activates the first item in the popup menu that appears if you click anywhere else on the recipient. I personally think it should just be a non-interactive icon with the title "In the addressbook" and "Not in the addressbook", it just doesn't seem particularly useful as a distinct interactive part which wouldn't necessarily require more usage than anything else in the popup.

Comment 17

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1764652 - Preserve focus when activating the MORE recipients button. r=aleca

Comment 18

[Alessandro Castellani [:aleca]]

All sounds good from comment 16, I agree entirely.
We can tackle this after 102 and have a little session to define scope and implementation strategy with another developer so we can divide and conquer this area.

Comment 19

[Alessandro Castellani [:aleca]]

Try run with the addition of tests to cover the customization options: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=863d13d09b2053c13da223a7e049dc9d5c61889f

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: Bug 1764652 - Replace tags XUL custom elements with HTML in the message header. r=mkmelin

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/be1c2780ec01
Fix hover sate of address book indicator and other CSS improvements in message header. r=henry,Paenglab

Comment 22

[Alessandro Castellani [:aleca]]

Waiting for the try run to be good before marking this for check-in:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=d2307b5ff33554d4012dfd1c8a09ff06d82dc248

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b0b210928557
Add tests for message header customization. r=darktrojan
https://hg.mozilla.org/comm-central/rev/be7bfd24a7ad
Preserve focus when activating the MORE recipients button. r=aleca
https://hg.mozilla.org/comm-central/rev/14b42dad7e2b
Replace tags XUL custom elements with HTML in the message header. r=mkmelin

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: Bug 1764652 - Replace website and message ids XUL custom elements with HTML in the message header. r=darktrojan

Comment 25

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1764652 - Change accessible naming for message header lists. r=aleca

We can use the semantic structure to simply the screen reader output and make it correspond to the visually displayed text.

Comment 26

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1764652 - Patch up the read name for simple message headings (for Orca). r=aleca

Depends on D147298

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5b2ca5b3d489
Change accessible naming for message header lists. r=aleca
https://hg.mozilla.org/comm-central/rev/f63f329f0a8e
Patch up the read name for simple message headings (for Orca). r=aleca

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f55176d77e75
Replace website and message ids XUL custom elements with HTML in the message header. r=darktrojan

Comment 29

[Alessandro Castellani [:aleca]]

There's a strange inconsistency in the "From" field when an email comes from a forum or mailing list, and that mailing list is saved in the address book.

On 91, the header reads "Alessandro from Thunderbird Team <[e-mail]>"
On 102, the header reads "Thunderbird Team"

I fixed this in the upcoming patch on bug 1556261.

Comment 30

[Alessandro Castellani [:aleca]]

I found the "problem" which is not really a problem.
In 91 when adding a contact to the Address Book we weren't populating the display name of the card right away, but only saving the email.

In my patches I added the display name autodiscovery and if we have one I save it in the ABCard right away.
The display name for the team mailing list is "Thunderbird Team", since that's what topicbox pushes.
Deleting the "Display Name" of the saved mailing list makes the full address appear.

In my upcoming patch of bug 1556261 I'm adding an option to allow users to always show the "Full address (name + email) for the sender". That will take care of it and give the option to the user to control the outcome.