Closed Bug 1764652 Opened 3 years ago Closed 2 years ago

Convert message header XUL custom elements to HTML custom elements

Categories

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

Tracking

(thunderbird_esr91 wontfix)

RESOLVED FIXED
102 Branch
Tracking Status
thunderbird_esr91 --- wontfix

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(9 files)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

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.

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

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.

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?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(henry)
Status: NEW → ASSIGNED

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.

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(henry)

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>
Attachment #9273344 - Attachment description: WIP: Bug 1764652 - Replace XUL custom elements with HTML in the message header. → WIP: Bug 1764652 - Replace XUL mail-multi-emailheaderfield with an HTML custom element.
Attachment #9273344 - Attachment description: WIP: Bug 1764652 - Replace XUL mail-multi-emailheaderfield with an HTML custom element. → Bug 1764652 - Replace XUL mail-multi-emailheaderfield and mail-headerfield with HTML custom elements. r=mkmelin,henry
Attachment #9273344 - Attachment description: Bug 1764652 - Replace XUL mail-multi-emailheaderfield and mail-headerfield with HTML custom elements. r=mkmelin,henry → Bug 1764652 - Replace XUL mail-multi-emailheaderfield, mail-headerfield, and mail-newsgroup with HTML custom elements. r=mkmelin,henry
Target Milestone: --- → 102 Branch

Pushed by alessandro@thunderbird.net:
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

  • 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
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/724a35930277 follow-up - Fix leaked observer. r=aleca

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 <my.contact@server.org>",
  2. "Address is in the addressbook",
  3. "To: second@server.org",
  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 <my.contact@server.org>. One of two.",
  2. "To: second@server.org. 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. "second@server.org. 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.

Flags: needinfo?(alessandro)
Regressions: 1770147

(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 <my.contact@server.org>",
  2. "Address is in the addressbook",
  3. "To: second@server.org",
  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 <my.contact@server.org>. One of two.",
  2. "To: second@server.org. 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. "second@server.org. 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!

Flags: needinfo?(alessandro)

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.

Attachment #9277239 - Attachment description: WIP: Bug 1764652 - Add tests for message header customization → Bug 1764652 - Add tests for message header customization. r=darktrojan

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

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.

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

Pushed by mkmelin@iki.fi:
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

Pushed by mkmelin@iki.fi:
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

Attachment #9278055 - Attachment description: WIP: Bug 1764652 - Replace website and message ids XUL custom elements with HTML in the message header. → Bug 1764652 - Replace website and message ids XUL custom elements with HTML in the message header. r=mkmelin
Keywords: leave-open

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

Pushed by geoff@darktrojan.net:
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

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f55176d77e75
Replace website and message ids XUL custom elements with HTML in the message header. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

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 <team@thuunderbird.net>"
On 102, the header reads "Thunderbird Team"

I fixed this in the upcoming patch on bug 1556261.

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.

Regressions: 1771802
Regressions: 1772319
Regressions: 1773013
Regressions: 1777802
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: