Closed Bug 1717632 Opened 3 years ago Closed 2 years ago

Improve semantics and accessibility of the addressbook view/edit side panel

Categories

(Thunderbird :: Address Book, defect, P1)

Thunderbird 92
Unspecified
All

Tracking

(thunderbird101 wontfix, thunderbird102+ fixed)

RESOLVED FIXED
103 Branch
Tracking Status
thunderbird101 --- wontfix
thunderbird102 + fixed

People

(Reporter: henry-x, Assigned: henry-x)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open, Whiteboard: [tb-new-addrbook] )

Attachments

(5 files)

I like the look of the new address book. Its also a good opportunity to improve the semantics and improve accessibility.

Here are some thing I noticed:

  • I'm not sure if the sortContext and bookContext popups are accessible. Someone with a screen reader could test them. In the accessibility tree, the menus appear at the end of the document, rather than in place, but I'm not sure if a screen reader is able to handle this.
  • The bookContext context menu does not appear when the address book has focus and the user pressed the Menu Key on their keyboard. Maybe use the contextmenu event instead https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event. (moved to bug 1753286)

When editing a contact:

  • The id="photo" element is a <div> rather than an <img>. Which means in the accessibility tree it appears as an empty section. I don't think we should hide the photo section for screen-reader users, in which case it needs a generic but useful alt: something like "No photo for <contact>" and "Photo for <contact>", as appropriate. The same goes for the when the contact isn't being edited.
  • The photo can only be set through drag and drop. I'm not sure if another mechanism is already planned. But some way to select an image using clicks only and keyboard only should be supported.
  • The <label> and <input> pairs use both for and aria-labelledby. The former is sufficient, so there is no need for aria-labelledby (https://www.w3.org/TR/html-aam-1.0/#input-type-text-input-type-password-input-type-search-input-type-tel-input-type-email-input-type-url-and-textarea-element-accessible-name-computation). aria-labelledby is only meant for if such a <label> is missing.
  • The "First" and "Last" field names are not that clear. "First Name" and "Last Name" would be clearer.
  • The two "Phonetic" fields appear visually beneath the corresponding "First" and "Last" fields, but in the document structure, and in the accessibility tree, they are ordered as First, Last, Phonetic, Phonetic, so it isn't clear what the "Phonetic" fields refer to. The order should be changed in the document so that the First Name "Phonetic" appears immediately after "First Name". The CSS grid-column-start can be used to keep the display the same as it is now. To be extra clear, the titles "First Name Phonetic" and "Second Name Phonetic" could be used so there aren't two fields with the same label.
  • The grouped fields could be put in <section>s, with the current <h2> headers at their start. This would help split up the fields. I think that the headers would already allow screen-reader users to skip to the next section (e.g. skip "Work Address" without going through each field), but I'm not sure. But I think this would improve the semantics regardless.
  • The second <input> of the "Address" fields has no associated <label> or accessible name. Either we could use two visible <label>s with "Address Line 1" and "Address Line 2" text. Or if we want them to be invisible, you can set the aria-label attributes to "Address Line 1" and "Address Line 2" (see the previous link for the accessible name calculation, and https://www.w3.org/TR/wai-aria-1.1/#aria-label).
  • There is no "Cancel" or "Save" <button> at the bottom. If the user was tabbing through the fields, then they would have to backtrack from the end to the start. Either the buttons can be moved to the end, or duplicated at the end.

There's likely some more, but these are the ones that I noticed looking at the document and the accessibility tree alone. Also, let me know if I've got any of the aria information wrong.

Whiteboard: [tb-new-addrbook]

An update on this:

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

  • The photo can only be set through drag and drop. I'm not sure if another mechanism is already planned. But some way to select an image using clicks only and keyboard only should be supported.

You can now select an image with clicks, but still not keyboard. Moreover, there's no semantics to indicate this can be clicked.

  • There is no "Cancel" or "Save" <button> at the bottom. If the user was tabbing through the fields, then they would have to backtrack from the end to the start. Either the buttons can be moved to the end, or duplicated at the end.

This is now at the bottom. But if you use a keyboard tab navigation, the position: sticky button container is often blocking the focussed element (i.e. the scroll area is not scrolled far enough on changing focus). NOTE: scrollIntoView does not work well with the presence of sticky elements, and you need to manually calculate the scroll position needed.

Otherwise, all the other accessibility problems are still there.

No longer blocks: tb-new-addrbook

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

  • The second <input> of the "Address" fields has no associated <label> or accessible name. Either we could use two visible <label>s with "Address Line 1" and "Address Line 2" text. Or if we want them to be invisible, you can set the aria-label attributes to "Address Line 1" and "Address Line 2" (see the previous link for the accessible name calculation, and https://www.w3.org/TR/wai-aria-1.1/#aria-label).

Regarding this point alone. Would it be ok to instead change the "Address" input to be a generic multi-line input (something like <textarea>). Personally, I've used street addresses (the part before the city) that are between 1 and 3 lines, so fixed numbers can be annoying. Or is there some hard data reason we have exactly two address lines?

Flags: needinfo?(geoff)

is there some hard data reason we have exactly two address lines?

Yes, we have only two places to put the data. These fields are tied to specific property names in nsIAbCard which have been used forever.

When we switch to vCard we'll have a little more flexibility. It only has two fields as well:

the post office box;
the extended address (e.g., apartment or suite number);
the street address;
the locality (e.g., city);
the region (e.g., state or province);
the postal code;
the country name

... but this comment in the spec is interesting:

Experience with vCard 3 has shown that the first two components (post office box and extended address) are plagued with many interoperability issues. To ensure maximal interoperability, their values SHOULD be empty.

The text components are separated by the SEMICOLON character (U+003B). Where it makes semantic sense, individual text components can include multiple text values (e.g., a "street" component with multiple lines) separated by the COMMA character (U+002C).

So it looks like we could have any number of lines. I'm going to be reimplementing most of the form fields as a part of the switch to vCard storage, so I'll do it then.

Flags: needinfo?(geoff)

https://datatracker.ietf.org/doc/html/rfc6350#section-6.3 - yes, sounds like street address should be textarea to potentially (but normally not) allow for multiline.

I'm changing this to focus on the view/edit panel. I'm going to mostly leave keyboard controls to bug 1753093, and focus more on semantic issues here.

So, an update for the current design.

In "viewing" mode:

  • The content is within a <form> element. This is not appropriate in viewing mode because there are no exposed form controls. It should be in a "reading" element, like <main>.
  • The photo is not an <img> element, even though it should be. Plus it would need alt text along the lines of "Contact picture" and "Missing/No contact picture" depending on the state.
  • The #nameOuter element is structured <h1>Contact name</h1><h2>Contact email</h2> and is within a <section id="detailsHeader">.
    • This means that the heading's scope is only this otherwise empty <section>. They should be headings for the <main> element.
    • Moreover, the I'm not sure the email should count as a sub-heading because there is no other h2 level heading. I think it is only meant to be part of the <h1> heading.
  • There is a non-hidden <label class="screen-readers-only" for="displayName"> element in this section. Similar to the first point, we shouldn't be mixing form elements with the viewing page. Moreover, this element was set up to only appear on screen readers...
  • The toolbar should have role="toolbar" and have focus controls. This is a minor issue, and it probably makes sense to wait until we have a custom toolbar widget. See bug 1553231.
  • The #detailsBody element shouldn't be a <section>. It is only a container for sections.
  • The "active-time" element should probably be a <time> element.

In "editing" mode:

  • The photo button should be a <button> or an <input type="file">. I think it should also have a visible "Contact Picture" label, above or below it.
  • The photo image should be an <img> with alt text.
  • The photo "dialog" is not semantic at all, and there is no keyboard controls. I'm not sure we really need this separate dialog because it adds an extra step -- you have to click to set up a drop point and then drop the image -- when we could just accept the drop on the form itself. The photo button as a <input type="file"> makes more sense to me. EDIT: Just realised that the dialog seems to be mostly for cropping, and you can drop onto the form's photo, but the semantic issue remains.
  • The #nameOuter element has similar issues to above because they share the same DOM space.
    • The <h2> email title is still visible.
    • This should have the structure of a <fieldset>.
    • The #displayName input has no visible label: it is set as screen-reader-only even though all users need this info.
  • Initial focus is on the "First Name" input, even though "Display Name" is before it. Either the "Display Name" should be focused first, or it should appear after the name inputs.
  • The email addresses table.
    • See bug 1762126 comment 40. Note that the design I gave in bug 1762126 comment 40 is not the smoothest on a screen reader (at least on Orca), but the advantage is that the table structure provides some association between the adjacent form controls. A grid widget might improve this area, but it would require much more work.
    • Moreover, there are no visible labels in this section, but I think they would be useful for all users. I think the table headings should be made visible rather than screen-reader-only because it is the easiest way to read it. But I know this would be breaking the given design mock ups. As a compromise we could give each selector or input a title corresponding to the heading.
  • The "Websites" and "Phone Numbers" similarly have multiple entries, but are structured differently to the "Email Addresses" section. They should be consistent and use a table as well. The above points would apply to them as well.
Blocks: tb-new-addrbook
No longer blocks: tb-ab-keyboard-ux
See Also: → tb-ab-keyboard-ux
Summary: Improve semantics and accessibility of the new addressbook → Improve semantics and accessibility of the addressbook view/edit side panel

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

  • The email addresses table.
    • See bug 1762126 comment 40.
    • Moreover, there are no visible labels in this section, but I think they would be useful for all users. I think the table headings should be made visible rather than screen-reader-only because it is the easiest way to read it. But I know this would be breaking the given design mock ups. As a compromise we could give each selector or input a title corresponding to the heading.

@aleca, what do you think? Have the table headers be visible for all users or just use a title on the inputs?

Flags: needinfo?(alessandro)

In terms of which problems would require strings before the string freeze next tuesday monday:

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

  • The photo is not an <img> element, even though it should be. Plus it would need alt text along the lines of "Contact picture" and "Missing/No contact picture" depending on the state.

Needs alt text.

  • The photo button should be a <button> or an <input type="file">. I think it should also have a visible "Contact Picture" label, above or below it.

If you go for the visible label, then that needs a string.

If you go for the <button> you probably need a title. Otherwise, if you go for <input type="file"> you need a label or a title, depending on your design.

  • The photo image should be an <img> with alt text.

I think this could benefit from showing a binary state: "Contact picture" and "Missing contact picture" or "No contact picture". But this might not be necessary if the <button> title or <input type="file"> label already conveys the information.

Specifically, needs a new fluent entry to replace "vcard-email-choose-primary" without the aria-label because it is not correct to set both the text content and the aria-label. It should probably just be "Default" or "Default email".

  • Moreover, there are no visible labels in this section, but I think they would be useful for all users. I think the table headings should be made visible rather than screen-reader-only because it is the easiest way to read it. But I know this would be breaking the given design mock ups. As a compromise we could give each selector or input a title corresponding to the heading.

Depending on what aleca thinks above, you will need to give these elements a title string.

  • The "Websites" and "Phone Numbers" similarly have multiple entries, but are structured differently to the "Email Addresses" section. They should be consistent and use a table as well. The above points would apply to them as well.

Same as above.

@darktrojan Can you start on this since nicolai is away today and friday?

Flags: needinfo?(geoff)

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

  • The content is within a <form> element. This is not appropriate in viewing mode because there are no exposed form controls. It should be in a "reading" element, like <main>.

I don't think it matters that there are no editable controls showing. form seems fine here, and even preferable to give the hint that stuff around here can be edited.

  • The photo is not an <img> element, even though it should be. Plus it would need alt text along the lines of "Contact picture" and "Missing/No contact picture" depending on the state.

Since it's an illustration only, alt="" seems best. Someone with a screen reader couldn't do anything useful with a picture anyway.

  • There is a non-hidden <label class="screen-readers-only" for="displayName"> element in this section. Similar to the first point, we shouldn't be mixing form elements with the viewing page. Moreover, this element was set up to only appear on screen readers...

Agreed this should have the label on top like for other elements. In general, if something is screen-reader-only there is something semantically wrong.

Assignee: nobody → alessandro
Flags: needinfo?(alessandro)
Priority: -- → P1

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

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

  • The content is within a <form> element. This is not appropriate in viewing mode because there are no exposed form controls. It should be in a "reading" element, like <main>.

I don't think it matters that there are no editable controls showing. form seems fine here, and even preferable to give the hint that stuff around here can be edited.

The problem is when you tab into the view, a screen reader would announce that you are entering a form, which is misleading because there are no field section, pressing "Enter" does not submit, etc. You only encounter something that behaves like a form after pressing "Edit", at which point the visibility of elements gets toggled. And when this happens, the "entering a form" announcement is absent!

Really, the form should be a separate element in the DOM acting like a separate page. The presence of the "Edit" button is sufficient to inform the user that the content can be edited.

  • The photo is not an <img> element, even though it should be. Plus it would need alt text along the lines of "Contact picture" and "Missing/No contact picture" depending on the state.

Since it's an illustration only, alt="" seems best. Someone with a screen reader couldn't do anything useful with a picture anyway.

I think I know what you are getting at, but the framing is bit presumptuous about all screen reader users.

It is more than "illustration only" because it is meant to convey who the person is. However, we could say that the contact name already does this, so already acts as its alternative text. I think we should include alt text during edit though because the "missing contact picture" icon has a very specific meaning in that context.

To fix that, <form role="presentation"> should do it (and change it back to role="form" in edit mode).

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

To fix that, <form role="presentation"> should do it (and change it back to role="form" in edit mode).

That would not be correct https://w3c.github.io/aria/#presentation Moreover, other allowed roles for form are not appropriate https://www.w3.org/TR/html-aria/#el-form

Regardless, using an element with strong semantics and then changing its role (dynamically) is not good practice. A semantic markup is the best design.

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

That would not be correct https://w3c.github.io/aria/#presentation Moreover, other allowed roles for form are not appropriate https://www.w3.org/TR/html-aria/#el-form

Why you think so? It seems quite explicitly allowed, as your link shows. Using role=none/presentation will remove the built-in ARIA semantics from the element.

Regardless, using an element with strong semantics and then changing its role (dynamically) is not good practice.

If there's reason to change, I don't see why not. It's really up to assistive technology to handle the situation.

Severity: -- → S3
OS: Unspecified → All
See Also: → 1771824
Version: Thunderbird 91 → Thunderbird 92

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

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

That would not be correct https://w3c.github.io/aria/#presentation Moreover, other allowed roles for form are not appropriate https://www.w3.org/TR/html-aria/#el-form

Why you think so? It seems quite explicitly allowed, as your link shows. Using role=none/presentation will remove the built-in ARIA semantics from the element.

Oh right, I see what you mean now. It still wouldn't work well because it would still benefit from some other semantics (like <article> or <main>), and we need the focus to move from outside the form element to within the form element. We don't need an accessibility hack to address this because a semantic design is sufficient and more reliable.

Assignee: alessandro → henry

The h3 level heading no longer exists.

This improves the semantics and helps separate the name/nickname/email headings logic in aboutAddressBook.js from edit.js.

As a side effect, we also prevent dropping a contact photo whilst in viewing mode. Before this change, dropping outside of editing mode was 'responsive', but didn't actually save the photo, so we remove this functionality entirely.

In particular, if the "Prefix" field is visible because it has an existing value, we focus that instead of "First Name".

This slightly improves the semantics of the form, and gives the button an accessible name via its title.

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

So, an update for the current design.

Some of this has been addressed.

In "viewing" mode:

  • The content is within a <form> element. This is not appropriate in viewing mode because there are no exposed form controls. It should be in a "reading" element, like <main>.

Fixed in D148900, but we're using <article> rather than <main>.

  • The photo is not an <img> element, even though it should be. Plus it would need alt text along the lines of "Contact picture" and "Missing/No contact picture" depending on the state.

Fixed in D149132 but with an empty alt.

  • The #nameOuter element is structured <h1>Contact name</h1><h2>Contact email</h2> and is within a <section id="detailsHeader">.
    • This means that the heading's scope is only this otherwise empty <section>. They should be headings for the <main> element.

Fixed in D148900

  • Moreover, the I'm not sure the email should count as a sub-heading because there is no other h2 level heading. I think it is only meant to be part of the <h1> heading.

Fixed in https://hg.mozilla.org/comm-central/rev/7be816a9528f and D148795

  • There is a non-hidden <label class="screen-readers-only" for="displayName"> element in this section. Similar to the first point, we shouldn't be mixing form elements with the viewing page. Moreover, this element was set up to only appear on screen readers...

Fixed in https://hg.mozilla.org/comm-central/rev/51105e478b12

  • The #detailsBody element shouldn't be a <section>. It is only a container for sections.

Fixed in D148900

In "editing" mode:

  • The photo button should be a <button> or an <input type="file">. I think it should also have a visible "Contact Picture" label, above or below it.
  • The photo image should be an <img> with alt text.

Changed to a button in D149132, but using a title on the button for the accessible name, rather than alt text.

  • The #nameOuter element has similar issues to above because they share the same DOM space.
    • The <h2> email title is still visible.
    • This should have the structure of a <fieldset>.
    • The #displayName input has no visible label: it is set as screen-reader-only even though all users need this info.

Fixed in https://hg.mozilla.org/comm-central/rev/51105e478b12

  • Initial focus is on the "First Name" input, even though "Display Name" is before it. Either the "Display Name" should be focused first, or it should appear after the name inputs.

Fixed in https://hg.mozilla.org/comm-central/rev/51105e478b12 . And D148909 makes sure focus goes to the first visible text input, whether that be "First Name" or "Prefix".

Still outstanding is:

In "viewing" mode:

  • The toolbar should have role="toolbar" and have focus controls. This is a minor issue, and it probably makes sense to wait until we have a custom toolbar widget. See bug 1553231.
  • The "active-time" element should probably be a <time> element.

Not a priority right now, although the <time> element might be easy to do.

In "editing" mode:

  • The photo "dialog" is not semantic at all, and there is no keyboard controls. I'm not sure we really need this separate dialog because it adds an extra step -- you have to click to set up a drop point and then drop the image -- when we could just accept the drop on the form itself. The photo button as a <input type="file"> makes more sense to me. EDIT: Just realised that the dialog seems to be mostly for cropping, and you can drop onto the form's photo, but the semantic issue remains.

This is still the case, but I think the whole photo process may need more of a fundamental re-design to address this. And maybe using <input type="file"> with cropping within the form would work better than a button that launches a dialog.

  • The email addresses table.
    • See bug 1762126 comment 40. Note that the design I gave in bug 1762126 comment 40 is not the smoothest on a screen reader (at least on Orca), but the advantage is that the table structure provides some association between the adjacent form controls. A grid widget might improve this area, but it would require much more work.
    • Moreover, there are no visible labels in this section, but I think they would be useful for all users. I think the table headings should be made visible rather than screen-reader-only because it is the easiest way to read it. But I know this would be breaking the given design mock ups. As a compromise we could give each selector or input a title corresponding to the heading.
  • The "Websites" and "Phone Numbers" similarly have multiple entries, but are structured differently to the "Email Addresses" section. They should be consistent and use a table as well. The above points would apply to them as well.

Moved to bug 1774174

Flags: needinfo?(geoff)
Status: NEW → ASSIGNED
Target Milestone: --- → 103 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/89ae23e3d8b1
Change addressbook view headings from h3 to h2. r=aleca
https://hg.mozilla.org/comm-central/rev/fedeeb4a60b0
Split out the contact edit form into a distinct element. r=aleca
https://hg.mozilla.org/comm-central/rev/52f941d0dc65
Focus the first visible input when entering the contact form. r=aleca
https://hg.mozilla.org/comm-central/rev/365ec21d3c24
Convert the contact photo input into a button and img. r=aleca

See Also: → 1774174

We temporarily patch up the photo dialog to give the dialog some minimal keyboard controls. We also give the drop area a role="alert" since it functions to instruct the user and update them on failures.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/4a6286802928
Allow setting a contact photo through a file picker with a keyboard. r=aleca

Comment on attachment 9280477 [details]
Bug 1717632 - Change addressbook view headings from h3 to h2. r=aleca

[Approval Request Comment]

Attachment #9280477 - Flags: approval-comm-beta?

Comment on attachment 9280653 [details]
Bug 1717632 - Split out the contact edit form into a distinct element. r=aleca

[Approval Request Comment]

Attachment #9280653 - Flags: approval-comm-beta?

Comment on attachment 9280672 [details]
Bug 1717632 - Focus the first visible input when entering the contact form. r=aleca

[Approval Request Comment]

Attachment #9280672 - Flags: approval-comm-beta?

Comment on attachment 9281017 [details]
Bug 1717632 - Convert the contact photo input into a button and img. r=aleca

[Approval Request Comment]

Attachment #9281017 - Flags: approval-comm-beta?

Comment on attachment 9281389 [details]
Bug 1717632 - Allow setting a contact photo through a file picker with a keyboard. r=aleca

[Approval Request Comment]

Attachment #9281389 - Flags: approval-comm-beta?

Comment on attachment 9280477 [details]
Bug 1717632 - Change addressbook view headings from h3 to h2. r=aleca

[Triage Comment]
Approved for beta

Attachment #9280477 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9280653 [details]
Bug 1717632 - Split out the contact edit form into a distinct element. r=aleca

[Triage Comment]
Approved for beta

Attachment #9280653 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9280672 [details]
Bug 1717632 - Focus the first visible input when entering the contact form. r=aleca

[Triage Comment]
Approved for beta

Attachment #9280672 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9281389 [details]
Bug 1717632 - Allow setting a contact photo through a file picker with a keyboard. r=aleca

[Triage Comment]
Approved for beta

Attachment #9281389 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9281017 [details]
Bug 1717632 - Convert the contact photo input into a button and img. r=aleca

[Triage Comment]
Approved for beta

Attachment #9281017 - Flags: approval-comm-beta? → approval-comm-beta+
See Also: → 1776193

Closing this bug since the last parts have been moved to specific bugs.

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

Still outstanding is:

In "viewing" mode:

  • The toolbar should have role="toolbar" and have focus controls. This is a minor issue, and it probably makes sense to wait until we have a custom toolbar widget. See bug 1553231.

Moved to bug 1776192

  • The "active-time" element should probably be a <time> element.

This isn't that important. Right now the <time> element doesn't help with accessibility, but this might change in the future. The main difficulty is choosing the right datetime attribute which is required by the element specification, and I can't figure out what would be appropriate for showing a time from a different timezone (as is the case here).

  • The photo "dialog" is not semantic at all, and there is no keyboard controls. I'm not sure we really need this separate dialog because it adds an extra step -- you have to click to set up a drop point and then drop the image -- when we could just accept the drop on the form itself. The photo button as a <input type="file"> makes more sense to me. EDIT: Just realised that the dialog seems to be mostly for cropping, and you can drop onto the form's photo, but the semantic issue remains.

Moved to bug 1776193

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: