Closed Bug 1589861 Opened 3 years ago Closed 3 months ago

Content of "Subject" field in headers pane not read by screen readers

Categories

(Thunderbird :: Disability Access, defect, P1)

defect

Tracking

(thunderbird_esr91+ fixed, thunderbird98 fixed)

RESOLVED FIXED
99 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird98 --- fixed

People

(Reporter: k.kolev1985, Assigned: henry)

References

Details

(Whiteboard: [works for braille but not speech synth])

Attachments

(3 files, 13 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
22.17 KB, patch
aleca
: review+
Details | Diff | Splinter Review

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 with a filled "Subject" field from the list, so it appears in the message preview pane.
  6. Use the TAB key to navigate to the "Subject" field in the headers pane in the message preview pane.

Actual results:

The screen reader says only "Subject".

Expected results:

The screen reader should report not only the label ("Subject) for the field as it does now, but the text content (value) of that subject field.

Alex and Jean-Philippe, there's more and even more.

Flags: needinfo?(jpmengual)
Flags: needinfo?(aarnaud)

Just like for bug 1589859, I confirm and suggest to fix. Even if it may be not a typical use case for a blind prople, which will rather disable the pane, open the message via enter and close via escape, I think it may be useful to have proper feedback for persons using the scren reader to speak the area under the mouse. The screen reader uses then labels and these feedbacks. So a bug that would be interesting to fix.

Regards

Flags: needinfo?(aarnaud)

Thanks again.

Flags: needinfo?(jpmengual)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
See Also: → 1589862

I just stumbled into this issue which still happens in nightly (although I understand bug1589859 has landed), at least on Linux with the Orca screen reader.

What I see is that the AT-SPI2 tree for the Subject value looks weird compared to the other ones (e.g. From, To, etc, work):
(The node name is on the left, with the node role in parentheses)

    • [nameless] (table)
      • [nameless] (table row)
        • "Subject" (row header)
          • "Subject" (label)
        • [nameless] (table cell)
          • "Subject: test2" (unknown)
      • [nameless] (table row)
        • "To" (row header)
          • "To" (label)
        • [nameless] (table cell)

As you can see, content of the Subject table cell is an object (highlighted above) with the unknown role (which ideally should never happen, it's likely to suggest a node that lacks proper exposition to the AT-SPI2 layer), with no children. On the other hand, there is a similar node for the To cell (highlighted as well), but this one has the relevant information in a label sub-node.

Having a proper non-unknown-roled node with the relevant information would most likely fix this for the Orca screen reader. Either have similar nodes as the To field, or make the unknown-roled node have a more meaningful role (e.g. label or something valid with the properties it exposes).

Did you try a nightly build where bug 1589859 has landed?

Yes. To be sure I checked that the patch's changes effectively appear in chrome/messenger/content/messenger/mailWidgets.js in my nightly's omni.ja, and they do, and the inspector shows the expected aria-label without aria-labelledby. However, these changes are not reflected in the AT-SPI2 tree, and the relations are still visible there, so I guess that's the problem I have actually.

Subject is a <mail-headerfield> and the content is it's value. It has no child label to hold the value.
I don't know of a role we could set to make it be a label.

I'm not sure, but if I manually set role="heading" (for a lack of a label role) through the devtools, Orca reads it properly.
If there's no equivalent ARIA role, what about actually having a child label with the value? (or it we can get the node itself to be considered as a label, inheriting from the base label instead of the base element? silly idea as I'm not familiar enough with XUL to know if it makes any sense)

Actually, it seems there is a (mozilla internal) role="label".

Assignee: nobody → mkmelin+mozilla

Actually, it seems there is a (mozilla internal) role="label".

This unfortunately doesn't seem to work at least if I use it from the devtools: it doesn't alter the role in the Accessibility tab (it stays "nothing"), nor in the AT-SPI2 representation (stays "unknown").

I guess if role="heading" helps, we could just add that.

Joanie, what do you think about having a heading role there? Does that make enough sense to you, or will it cause other kind of confusion?

Flags: needinfo?(jdiggs)

Is this still an issue?

Assignee: mkmelin+mozilla → nobody

Hi,

Actually here I have the feedback in braille but not with the speech synth. Perhaps @Joanmarie has an idea ether it is a TB or a screen reader issue?

Whiteboard: [works for braille but not speech synth]
  1. Yes the role of "unknown" is bogus and bad, but Orca is actually coping with that because the thing claims focus via accessibility events so Orca has to say something.

  2. For non-document (i.e. native app GUI) objects, Orca presents either the label (determined by the accessible labelledby relation) or the name (determined by the accessible name property) or both. By default, it's "or" rather than "both". Orca does not have custom handling for role "unknown" (because it doesn't know what that handling should be). Thus when these mystery objects claim focus, Orca does the following:

    a. checks first for the labelledby relation and, if found, presents the concatenated labels to the user and considers itself done
    b. checks for the accessible name

In the case of all the fields BUT the subject, the object with role "unknown" lacks a label. Its name is the full line, so things work. HOWEVER, in the case of the subject field, the object with role "unknown" HAS a label. The labelledby relationship points to the accessible label whose name is "Subject". So Orca considers itself done and speaks that without ever checking for the name.

I don't consider this a bug in Orca. You're not telling Orca what the thing is (bogus role). And you are telling Orca that, whatever it is, you are providing an explicit label to speak. Orca trusts you. ;)

As for why it works (kinda) in braille: The mystery object lives in a table row. For braille, by default the full row is presented. So Orca is doing extra work trying to piece together the row contents from the accessibility tree (which, as an aside, could use some pruning if at all possible). That Orca doesn't do the same thing in speech is also not a bug. When focus moves within a table (or anywhere else for that matter), the surrounding non-focused things are not of immediate interest and thus speaking them would just be unwanted noise.

As for the fix: I'm guessing the quick and dirty solution is to remove the labelledby relation. But as is hopefully more clear, there are other problems which should arguably be addressed.

Flags: needinfo?(jdiggs)
Attached patch a11yheaderfield.patch (obsolete) — Splinter Review

This patch does the following:

  1. Fixes a11y tree for the <mail-headerfield> and <mail-urlfield> headers, ie Subject, and all the rest in All Headers view. Keyboard focus results in the label and value spoken, plus menu (as those values have a context popup). Tested using Orca on linux)
  2. Convert label to div; inline textContent dtds.
  3. Make tags and messageId header rows keyboardable. They are otherwise not yet fixed (Part 2).

IMO hardcoding the ":" for a11y seems wrong. It should be a pref whether to display a header separator, for visual and speech.

This requires the patch in Bug 1745619. It should not be influenced by any future hypothetical work on this widget.

Assignee: nobody → alta88
Attachment #9256460 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9256460 [details] [diff] [review]
a11yheaderfield.patch

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

This seems good, with some nits. (And need the other patches to land first.)

::: mail/base/content/widgets/mailWidgets.js
@@ +53,5 @@
>      "resource:///modules/TagUtils.jsm"
>    );
>  
>    class MozMailHeaderfield extends MozXULElement {
>      connectedCallback() {

Not that this is adding a child, please protect against connectedCallback running multiple times. 
Like https://searchfox.org/comm-central/rev/3206f09bcd05afbda9e029d6cea98acd743d63ad/mail/base/content/widgets/mailWidgets.js#276

@@ +60,5 @@
>        this.classList.add("headerValue");
> +      let headerName = this.getAttribute("headerName");
> +
> +      this.valueNode = document.createElement("li");
> +      this.valueNode.setAttribute("id", "expanded" + headerName + "Value");

Using ids in custom elements is not nice, since at least theoretically one doesn't know if that element will be used many times within the doc. 
Was there a need for this id? Better to use a class if needed.
Attachment #9256460 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

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

@@ +60,5 @@

   this.classList.add("headerValue");
  •  let headerName = this.getAttribute("headerName");
    
  •  this.valueNode = document.createElement("li");
    
  •  this.valueNode.setAttribute("id", "expanded" + headerName + "Value");
    

Using ids in custom elements is not nice, since at least theoretically one
doesn't know if that element will be used many times within the doc.
Was there a need for this id? Better to use a class if needed.

For aria-labelledby, an id is always needed. The original patch used that, but this version uses aria-label, due to the precedent of hardcoding a ":" for readers even though there isn't one visually, which makes it sort of a lie but does make it clearer it's a label when spoken. Maybe some best practice advice from an a11y person would be useful. Patch updated for the precedent for now.

Attached patch a11yheaderfield.patch (obsolete) — Splinter Review
Attachment #9256460 - Attachment is obsolete: true
Attachment #9256540 - Flags: review?(mkmelin+mozilla)
Attached patch a11yheaderfield.patch (obsolete) — Splinter Review

Store headerName attr for filters.

Attachment #9256540 - Attachment is obsolete: true
Attachment #9256540 - Flags: review?(mkmelin+mozilla)
Attachment #9256541 - Flags: review?(mkmelin+mozilla)
Attached patch a11yheaderfield-test.patch (obsolete) — Splinter Review

Probably a try would also be good.

Attachment #9256560 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9256541 [details] [diff] [review]
a11yheaderfield.patch

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

::: mail/base/content/msgHdrView.inc.xhtml
@@ +339,5 @@
>  
>    <!-- Date -->
>    <html:div id="expandeddateRow" class="message-header-row" hidden="hidden">
> +    <html:div id="expandeddateLabel"
> +         class="message-header-label">&dateField4.label;</html:div>

Here and elsewhere the alignment is one space short. class should vertically align with div here

::: mail/base/content/widgets/mailWidgets.js
@@ +63,4 @@
>        this.setAttribute("context", "copyPopup");
>        this.classList.add("headerValue");
>  
> +      this.valueNode = document.createElement("li");

mail-headerfield is not an ul/ol so it seems wrong to have an li child here
Attachment #9256541 - Flags: review?(mkmelin+mozilla) → review-

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

Comment on attachment 9256541 [details] [diff] [review]
a11yheaderfield.patch

Review of attachment 9256541 [details] [diff] [review]:

::: mail/base/content/msgHdrView.inc.xhtml
@@ +339,5 @@

<!-- Date -->
<html:div id="expandeddateRow" class="message-header-row" hidden="hidden">

  • <html:div id="expandeddateLabel"
  •     class="message-header-label">&dateField4.label;</html:div>
    

Here and elsewhere the alignment is one space short. class should vertically
align with div here

Well, class should line up with id. So the idea is, when the html prefix is eventually removed, it will line up that way.

::: mail/base/content/widgets/mailWidgets.js
@@ +63,4 @@

   this.setAttribute("context", "copyPopup");
   this.classList.add("headerValue");
  •  this.valueNode = document.createElement("li");
    

mail-headerfield is not an ul/ol so it seems wrong to have an li child here

Right, the headerfield custom elements should extend HTMLUListElement. I'm not doing that here solely due to xul contextmenu, which fix is a different piece of work. It also works best with li for this bug and the a11y tree (ie div is not good).

All of that old ported binding code can be reduced by half with a real html renovation.

Flags: needinfo?(mkmelin+mozilla)
Attached patch a11yheaderfield.patch (obsolete) — Splinter Review

Ok, this patch de xuls the base message and message url custom elements, implements context menu for those. 4 subsequent patches fix tag, messageId, newsgroup, email multifield type header rows.

Attachment #9256541 - Attachment is obsolete: true
Attachment #9256560 - Attachment is obsolete: true
Attachment #9256560 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(mkmelin+mozilla)
Attachment #9257069 - Flags: review?(mkmelin+mozilla)
Attached patch a11ytagfield.patch (obsolete) — Splinter Review

Tags row, implements ul, which works nicely with a screen reader (counts the items).

Attachment #9257079 - Flags: review?(mkmelin+mozilla)
Attached patch a11ymessageIdfield.patch (obsolete) — Splinter Review

The messageId rows. Also fixes some linting, datetime wrapping/flex.

Attachment #9257198 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9257069 [details] [diff] [review]
a11yheaderfield.patch

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

::: mail/base/content/msgHdrView.inc.xhtml
@@ +232,5 @@
>      </html:div>
>  
>      <html:div id="expandedfromRow" class="header-row-grow">
> +      <html:div id="expandedfromLabel"
> +           class="message-header-label header-pill-label"

Regarding indention, I think we should indent for what we currently have and not how it might look in the future.
Attachment #9257069 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9257198 [details] [diff] [review]
a11ymessageIdfield.patch

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

Besides the nits, looks good thx! r=mkmelin

::: mail/base/content/msgHdrView.inc.xhtml
@@ +357,5 @@
>      <html:div id="expandedmessage-idLabel"
>           class="message-header-label">&messageIdField4.label;</html:div>
> +    <html:div class="toggle-list-container">
> +      <html:button class="plain-button toggle-list-button"
> +              aria-expanded="false"

indention is off

@@ +359,5 @@
> +    <html:div class="toggle-list-container">
> +      <html:button class="plain-button toggle-list-button"
> +              aria-expanded="false"
> +              aria-labelledby="expandedmessage-idLabel">
> +        <html:img src="chrome://global/skin/icons/arrow-right-12.svg"></html:img>

<html:img> is self closing, so 

<html:img src="chrome://global/skin/icons/arrow-right-12.svg" />

Here and elsewhere in this patch
Attachment #9257198 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9257079 - Flags: review?(mkmelin+mozilla) → review+

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a026e0d6a564f6ee20b941876bca1d22b65d468

Please consider moving to Phabricator, it's really a much nicer review experience.

Target Milestone: --- → 97 Branch
Attached patch a11ynewsgroupfield.patch (obsolete) — Splinter Review

For newsgroups; the new look is "'button", like the crypto button, also fix img selfclosing tags. The next patch will be formatting/indent/whitespace.

Attachment #9257678 - Flags: review?(mkmelin+mozilla)
Attached patch format.patch (obsolete) — Splinter Review

This aligns the html as requested above, and reorganizes the css file (no changes, 2 obsolete rules removed).
The next patch, email headers, will also implement Bug 1743496.

Attachment #9257691 - Flags: review?(mkmelin+mozilla)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/07e5acae5cb1
Enable tags header for screen reader, update test. r=mkmelin
https://hg.mozilla.org/comm-central/rev/fe366592cc41
Enable messageId type (messageId, references, inReplyTo) headers for screen reader. r=mkmelin

I noticed some semantic/accessibility problems with the pushed patches:

  1. The MozMessageHeader are being given the aria role="menu" even though they aren't menus. Not sure what the intention was.
  2. Moreover, message-header-url is a <div role=menu> rather than an <a>. Other values should similarly be <a> elements.
  3. mail-messageids-headerfield was changed to a <ul>, but some should only ever contain one value. E.g. expandedmessage-idRow that shows the Message ID should one ever be one value.
  4. The toggle-list-button has the wrong accessible name (it is currently using the field value). Plus, the <img> inside the toggle-list-button is missing an alt attribute. This should probably be something like "Expand"/"Collapse", depending on aria-expanded state, and this can naturally act as the accessible name of the button.
  5. Its not clear why dateLabel and dateLabelSubject were given a tabindex=0 when it has no interactive behaviour. It just means there is an extra tabbing step for keyboard users. Plus, it is too wide when it receives focus styling.
  6. Similarly, message-header and message-header-value entries are given tabindex=0 without having any interactive behaviour.

In addition, I didn't actually check all the CSS, but I accidentally noticed that expandedSubjectRow has a CSS flex: 4, which overwrites the flex: 1 it has from .header-row-grow. This seems unnecessary.

Comment on attachment 9257691 [details] [diff] [review]
format.patch

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

::: mail/base/content/msgHdrView.inc.xhtml
@@ +234,5 @@
>      <html:div id="expandedfromRow" class="header-row-grow">
>        <html:div id="expandedfromLabel"
> +                class="message-header-label header-pill-label"
> +                valueFrom="&fromField4.label;"
> +                valueAuthor="&author.label;">&fromField4.label;</html:div>

For entries that take up more than one line and have a closing tag, I think you should put the text content on a new line (indented) and the closing tag on another new line (not indented).
Duplicate of this bug: 1745796

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

I noticed some semantic/accessibility problems with the pushed patches:

  1. The MozMessageHeader are being given the aria role="menu" even though they aren't menus. Not sure what the intention was.

Because the fields all have a contextmenu.

  1. Moreover, message-header-url is a <div role=menu> rather than an <a>. Other values should similarly be <a> elements.

Have you tried this? Using an <a> tag with href here, in a mixed html/xul context, will not work as expected - a click will not perform any action, the contextmenu will not be as for a html link. So it doesn't really function like <a> in a web page, although it would be nice if it did. There are also other headers that are "link-like" and don't have an http href, so this also must be considered. Once 3pane becomes content, this can be revisited.

  1. mail-messageids-headerfield was changed to a <ul>, but some should only ever contain one value. E.g. expandedmessage-idRow that shows the Message ID should one ever be one value.

Yes, single items per RFC (messageId and some email type fields) will get role="menu". Not finished here. It is unwarranted and unnecessary to have a separate class/code when there is no visual difference for a single item in a ul.

  1. The toggle-list-button has the wrong accessible name (it is currently using the field value). Plus, the <img> inside the toggle-list-button is missing an alt attribute. This should probably be something like "Expand"/"Collapse", depending on aria-expanded state, and this can naturally act as the accessible name of the button.

For the references header, for example, upon tab focus on the toggle button, Orca reads "References collapsed pushbutton". So I don't seed and actual incorrectness problem; perhaps an alt could have more description, etc.

  1. Its not clear why dateLabel and dateLabelSubject were given a tabindex=0 when it has no interactive behaviour. It just means there is an extra tabbing step for keyboard users. Plus, it is too wide when it receives focus styling.

Without that tabstop, it cannot be read by a screen reader. And why should those be an exception to the behavior of the other fields. The latest patch gives datetime a fit-content.

  1. Similarly, message-header and message-header-value entries are given tabindex=0 without having any interactive behaviour.

No such thing as just message-header. Anyway, see previous.

In addition, I didn't actually check all the CSS, but I accidentally noticed that expandedSubjectRow has a CSS flex: 4, which overwrites the flex: 1 it has from .header-row-grow. This seems unnecessary.

That flex has been updated in the latest patch to 2 2. If you actually try to change the width of the message, you might notice an attempt to wrap subject and datetime (where it is in subject) at different rates, given their different lengths.

(In reply to alta88 from comment #37)

  1. The MozMessageHeader are being given the aria role="menu" even though they aren't menus. Not sure what the intention was.

Because the fields all have a contextmenu.

role="menu" (https://w3c.github.io/aria/#menu) is not meant for elements that have a (context/popup) menu, but for elements that are a menu. There should be no need to set the role at all.

  1. Moreover, message-header-url is a <div role=menu> rather than an <a>. Other values should similarly be <a> elements.

Have you tried this? Using an <a> tag with href here, in a mixed html/xul context, will not work as expected - a click will not perform any action, the contextmenu will not be as for a html link. So it doesn't really function like <a> in a web page, although it would be nice if it did. There are also other headers that are "link-like" and don't have an http href, so this also must be considered. Once 3pane becomes content, this can be revisited.

It acts as a hyper-link, and is presented visually as a hyper-link, so it should be semantically represented as an <a>. It won't give you the browser default behaviour of a <a href="http:...">, but that's not an issue since they already have handlers. Plus, provided you give a valid href attribute (doesn't need to be http), it will be automatically focusable, styled correctly, and you only need to connect to the click signal (which unlike <div> will cover both a mouse click and keyboard Enter).

  1. The toggle-list-button has the wrong accessible name (it is currently using the field value). Plus, the <img> inside the toggle-list-button is missing an alt attribute. This should probably be something like "Expand"/"Collapse", depending on aria-expanded state, and this can naturally act as the accessible name of the button.

For the references header, for example, upon tab focus on the toggle button, Orca reads "References collapsed pushbutton". So I don't seed and actual incorrectness problem; perhaps an alt could have more description, etc.

Please check the accessibility tree, rather than just run it through a single screen reader, especially for accessibility patches. In the accessibility tree you will see the issue with the button's accessible name and the missing alt text.

  1. Its not clear why dateLabel and dateLabelSubject were given a tabindex=0 when it has no interactive behaviour. It just means there is an extra tabbing step for keyboard users. Plus, it is too wide when it receives focus styling.

Without that tabstop, it cannot be read by a screen reader. And why should those be an exception to the behavior of the other fields.

Focus shouldn't be necessary to be readable. Focus is used for interaction. Did you test the screen reader behaviour before your patches, and was it reading the values then? It could be caused by the menu role, or it may be the way you have the screen reader configured.

Attached patch format.patch (obsolete) — Splinter Review

update formatting, fix lint.

Attachment #9257691 - Attachment is obsolete: true
Attachment #9257691 - Flags: review?(mkmelin+mozilla)
Attachment #9257843 - Flags: review?(mkmelin+mozilla)

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

(In reply to alta88 from comment #37)

  1. The MozMessageHeader are being given the aria role="menu" even though they aren't menus. Not sure what the intention was.

Because the fields all have a contextmenu.

role="menu" (https://w3c.github.io/aria/#menu) is not meant for elements that have a (context/popup) menu, but for elements that are a menu. There should be no need to set the role at all.

Your citation does not say that at all.
The menu role is appropriate when a list of menu items is presented in a manner similar to a menu on a desktop application.

and that is exactly what a popup is (dual popup and context for the newsgroup/email field values). Are you saying there isn't/shouldn't be a way to expose context/popup functionality to a screen reader here? If not, then what is it?

  1. Moreover, message-header-url is a <div role=menu> rather than an <a>. Other values should similarly be <a> elements.

Have you tried this? Using an <a> tag with href here, in a mixed html/xul context, will not work as expected - a click will not perform any action, the contextmenu will not be as for a html link. So it doesn't really function like <a> in a web page, although it would be nice if it did. There are also other headers that are "link-like" and don't have an http href, so this also must be considered. Once 3pane becomes content, this can be revisited.

It acts as a hyper-link, and is presented visually as a hyper-link, so it should be semantically represented as an <a>. It won't give you the browser default behaviour of a <a href="http:...">, but that's not an issue since they already have handlers. Plus, provided you give a valid href attribute (doesn't need to be http), it will be automatically focusable, styled correctly, and you only need to connect to the click signal (which unlike <div> will cover both a mouse click and keyboard Enter).

To followup.

  1. The toggle-list-button has the wrong accessible name (it is currently using the field value). Plus, the <img> inside the toggle-list-button is missing an alt attribute. This should probably be something like "Expand"/"Collapse", depending on aria-expanded state, and this can naturally act as the accessible name of the button.

For the references header, for example, upon tab focus on the toggle button, Orca reads "References collapsed pushbutton". So I don't seed and actual incorrectness problem; perhaps an alt could have more description, etc.

Please check the accessibility tree, rather than just run it through a single screen reader, especially for accessibility patches. In the accessibility tree you will see the issue with the button's accessible name and the missing alt text.

It was left as a FIXME with a div (by you) with the same issues so at least this is moving in a better direction by revealing itself as a toggleable button. Leaving as followup.

  1. Its not clear why dateLabel and dateLabelSubject were given a tabindex=0 when it has no interactive behaviour. It just means there is an extra tabbing step for keyboard users. Plus, it is too wide when it receives focus styling.

Without that tabstop, it cannot be read by a screen reader. And why should those be an exception to the behavior of the other fields.

Focus shouldn't be necessary to be readable. Focus is used for interaction. Did you test the screen reader behaviour before your patches, and was it reading the values then? It could be caused by the menu role, or it may be the way you have the screen reader configured.

Yes, and no, it does not read those date values prior to these patches and is a stock installation on linux. Maybe your configuration is different - the sole detailed comment here uses Orca as a reference. There is no difference between date fields and subject field, so your point is quite unclear, unless it's to pick and choose certain fields? I don't agree if so.

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

The one patch had wrong bug number (bug 1745796)
https://hg.mozilla.org/comm-central/rev/f589d5cda09d

Since aleca and henry started the review there it would have been proper to have them continue the reviews.

Re role="menu", apparently there is aria-haspopup="menu" which would seem appropriate

Comment on attachment 9257678 [details] [diff] [review]
a11ynewsgroupfield.patch

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

::: mail/base/content/msgHdrView.inc.xhtml
@@ +299,5 @@
>    <html:div id="expandednewsgroupsRow" class="message-header-row" hidden="hidden">
>      <html:div id="expandednewsgroupsLabel"
> +         class="message-header-label">&newsgroupsField4.label;</html:div>
> +    <html:ul is="message-header-list-newsgroups" id="expandednewsgroupsBox"
> +        headerName="newsgroups">

Please align attributes for what we have at the moment.

::: mail/base/content/widgets/mailWidgets.js
@@ +197,5 @@
> +  class MozMessageNewsgroups extends HTMLUListElement {
> +    constructor() {
> +      super();
> +      this.setAttribute("is", "message-header-list-newsgroups");
> +      this.classList.add("header-value-list");

Why move these to the constructor?
I can't find the reference now, but IIRC one should not change dom/attributes in the constructor
Attachment #9257678 - Flags: review?(mkmelin+mozilla) → review?(henry)
Comment on attachment 9257843 [details] [diff] [review]
format.patch

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

::: mail/base/content/msgHdrView.inc.xhtml
@@ +470,1 @@
>      <html:div is="message-header-url" id="expandedcontent-baseBox"

Would be nice to try using <a> for this
Attachment #9257843 - Flags: review?(mkmelin+mozilla) → review?(henry)

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

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

The one patch had wrong bug number (bug 1745796)
https://hg.mozilla.org/comm-central/rev/f589d5cda09d

Since aleca and henry started the review there it would have been proper to have them continue the reviews.

That bug was merely changing labels to html only. These patches de xul all the custom elements in this widget, and you mostly did the conversion from xbl binding to xul custom elements, that's why.

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

Comment on attachment 9257678 [details] [diff] [review]
a11ynewsgroupfield.patch

Review of attachment 9257678 [details] [diff] [review]:

::: mail/base/content/msgHdrView.inc.xhtml
@@ +299,5 @@

<html:div id="expandednewsgroupsRow" class="message-header-row" hidden="hidden">
<html:div id="expandednewsgroupsLabel"

  •     class="message-header-label">&newsgroupsField4.label;</html:div>
    
  • <html:ul is="message-header-list-newsgroups" id="expandednewsgroupsBox"
  •    headerName="newsgroups">
    

Please align attributes for what we have at the moment.

This is done in format.patch.

::: mail/base/content/widgets/mailWidgets.js
@@ +197,5 @@

  • class MozMessageNewsgroups extends HTMLUListElement {
  • constructor() {
  •  super();
    
  •  this.setAttribute("is", "message-header-list-newsgroups");
    
  •  this.classList.add("header-value-list");
    

Why move these to the constructor?
I can't find the reference now, but IIRC one should not change
dom/attributes in the constructor

Ok.

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

Comment on attachment 9257843 [details] [diff] [review]
format.patch

Review of attachment 9257843 [details] [diff] [review]:

::: mail/base/content/msgHdrView.inc.xhtml
@@ +470,1 @@

 <html:div is="message-header-url" id="expandedcontent-baseBox"

Would be nice to try using <a> for this

I'll do this in a followup. I prefer to complete and land the de xul before tuning the html/css and finer points of a11y.

Attached patch a11ynewsgroupfield.patch (obsolete) — Splinter Review

Move dom code from constructor in messageIds and newsgroups.

Attachment #9257678 - Attachment is obsolete: true
Attachment #9257678 - Flags: review?(henry)
Attachment #9258972 - Flags: review?(mkmelin+mozilla)

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

Comment on attachment 9257691 [details] [diff] [review]
format.patch

Review of attachment 9257691 [details] [diff] [review]:

::: mail/base/content/msgHdrView.inc.xhtml
@@ +234,5 @@

 <html:div id="expandedfromRow" class="header-row-grow">
   <html:div id="expandedfromLabel"
  •            class="message-header-label header-pill-label"
    
  •            valueFrom="&fromField4.label;"
    
  •            valueAuthor="&author.label;">&fromField4.label;</html:div>
    

For entries that take up more than one line and have a closing tag, I think
you should put the text content on a new line (indented) and the closing tag
on another new line (not indented).

So I knew there was a reason not to do that, and it's because all the tests blow up due to the comparison fail due to spaces in the textContent unless closely surrounded by the tags. :/

Attached patch format.patch (obsolete) — Splinter Review
Attachment #9257843 - Attachment is obsolete: true
Attachment #9257843 - Flags: review?(henry)
Attachment #9258993 - Flags: review?(henry)
Comment on attachment 9258972 [details] [diff] [review]
a11ynewsgroupfield.patch

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

::: mail/base/content/widgets/mailWidgets.js
@@ +196,5 @@
>  
> +  class MozMessageNewsgroups extends HTMLUListElement {
> +    constructor() {
> +      super();
> +    }

Since it's only super() left this is not needed - called automatically.

::: mail/themes/shared/mail/messageHeader.css
@@ +431,5 @@
> +/* When a .message-header-value-button (emails/newsgroups) context menu is
> + * opened, set the open attribute for toolbarbutton active appearance and
> + * tweak the bottom to blend in with the popupmenu.
> + */
> +.message-header-value-button[openedby="click"] {

Not too thrilled about this, but I don't have better suggestions.
Attachment #9258972 - Flags: review?(mkmelin+mozilla) → review+

The next patch, email headers, will also implement Bug 1743496.

Please, don't implement something in this bug if is reported in another bug?

Comment on attachment 9258972 [details] [diff] [review]
a11ynewsgroupfield.patch

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

::: mail/themes/shared/mail/messageHeader.css
@@ +431,5 @@
> +/* When a .message-header-value-button (emails/newsgroups) context menu is
> + * opened, set the open attribute for toolbarbutton active appearance and
> + * tweak the bottom to blend in with the popupmenu.
> + */
> +.message-header-value-button[openedby="click"] {

actually, why not just use a class, like opened-click
Attachment #9258972 - Flags: review?(alessandro)
Comment on attachment 9258972 [details] [diff] [review]
a11ynewsgroupfield.patch

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

I see a lot of extra CSS changes in this and the previously approved patches which I'm not sure it's necessary.
Some fields are not aligned anymore, but I guess this is a work in progress that needs other follow ups.

I'm also seeing other issues that should be addressed:
- The twisty button to reveal or collapse a list of addresses is not correctly aligned and styled on hover. 
- The message header being taller than what it needs. I don't know if it's related to the previous patches, but I never noticed it before.
Anyone else is seeing it?
- Please, fix all the wrong indentation that were introduced in the previous patches, and all the missing alt="" tags to images.

It would have been nice being made aware of this patches before, since we talked about this changes in other bugs and both Henry and I are working on updating the message header.
For future front-end and a11y changes, please ask Henry or myself for a review.

I'm kindly overriding Magnus' r+ to review the changes requested.
Thanks for your time and contribution.

::: mail/base/content/msgHdrView.inc.xhtml
@@ +297,5 @@
>  
>    <!-- Newsgroups -->
>    <html:div id="expandednewsgroupsRow" class="message-header-row" hidden="hidden">
>      <html:div id="expandednewsgroupsLabel"
> +         class="message-header-label">&newsgroupsField4.label;</html:div>

nit: please fix the indentation to follow the first attribute location, here and everywhere else.

@@ +361,5 @@
>      <html:div class="toggle-list-container">
>        <html:button class="plain-button toggle-list-button"
>                aria-expanded="false"
>                aria-labelledby="expandedmessage-idLabel">
> +        <html:img src="chrome://global/skin/icons/arrow-right-12.svg" />

nit: please add the `alt=""` attribute here and to all other images.

::: mail/base/content/msgHdrView.js
@@ +1450,2 @@
>   */
>  function OutputNewsgroups(headerEntry, headerValue) {

With these changes now this method is identical to `OutputMessageIds()`.
It would be better to have a generic method that does the same for both, or even directly remove those methods and directly call `updateHeaderValue()`.

::: mail/base/content/widgets/mailWidgets.js
@@ +221,5 @@
>      }
>  
>      buildViews() {
> +      for (let newsgroup of this.mNewsgroups) {
> +        let valueNode = document.createElement("li");

I'm not sure the `li` element is the correct one to use in this scenario.
I would go for an `<address>` here, and we should that also for all other recipients (in another patch).
This is also a clickable element, so we should add `role="button"`, or we could go the extra step and make these `input role="button"` or directly a `button type="button"`.

Henry, what would you recommend here?

@@ +224,5 @@
> +      for (let newsgroup of this.mNewsgroups) {
> +        let valueNode = document.createElement("li");
> +        valueNode.setAttribute("tabindex", "0");
> +        valueNode.classList.add("toolbarbutton-1");
> +        valueNode.classList.add("message-header-value-button");

I would maybe call this class `message-header-address`, since it's generic enough to be used for everything, unless we make this element a button, in which case this class name would be good.

@@ +237,5 @@
> +            .getElementById("newsgroupPopup")
> +            .openPopup(event.target, "after_start", 0, 0, false);
> +          event.preventDefault();
> +          valueNode.setAttribute("open", "true");
> +          valueNode.setAttribute("openedby", "click");

We're trying to avoid adding non standard attributes. We could use CSS here if we want to simulate an active state.
I would add this to the newsgroupPopup onpopupshowed event.

::: mail/themes/shared/mail/messageHeader.css
@@ +401,5 @@
>    color: -moz-activehyperlinktext;
>  }
>  
> +/* Match .crypto-button */
> +.message-header-value-button {

We should keep the styling consistent for all addresses with the same hover effect and sizing of the from, cc, and other fields.
This looks very out of place and it's larger than it needs to be.

@@ -433,5 @@
>  
> -#expandedtoRow .message-header-datetime {
> -  align-self: flex-start;
> -  margin-block: 2px;
> -}

The flex start is only needed for the datetime field in the To row, and the margin block is needed to align it with the pill style of the addresses, please add this back.
Attachment #9258972 - Flags: review?(alessandro)
Attachment #9258972 - Flags: review+
Attachment #9258972 - Flags: feedback+
Attached file . (obsolete) (deleted) —

The attached attempt to use the cpg bully in an attempt to demand technical respect and authority, neither of which has been earned or is deserved, is a violation of the Alta Participation Requirements.

Attachment #9258972 - Attachment is obsolete: true
Attachment #9258993 - Attachment is obsolete: true
Attachment #9258993 - Flags: review?(henry)
Assignee: alta88 → nobody
Status: ASSIGNED → NEW
Attachment #9259241 - Attachment is obsolete: true
The content of attachment 9259241 [details] has been deleted for the following reason:

Requested

Assigning this to Henry to continue the work and fix some issues.

Assignee: nobody → henry

I started putting some fixes together for the main problems introduced by the landed patches, but it is difficult to understand all the changes in the landed patches. Especially the changes to the CSS are hard to follow.

This is making it difficult to catch all the problems that may have been introduced. Plus, I think a lot of the changes that were made are not taking this section in the right direction. I.e. they are converting XUL elements to HTML elements, but not the ones we (eventually) need. So we have potential regressions without much benefit.

I think it might actually be easier to revert all the landed patches (before it becomes too difficult), then we can spend some time coming up with an overall layout design for this area, and then make changes according to the agreed design (potentially cherry picking parts of these patches).

I'm okay with doing a backout, hopefully things are not tangled and it would be straightforward.
Magnus, what do you think>

Flags: needinfo?(mkmelin+mozilla)

It looks like the css changes were mostly more-or-less mechanical changes due to changing name/id/class etc. Anything in particular?

I guess you can back out, and work your way back if you thing that's easier. Perhaps you want to do that locally to see where it ends up?
This bug should not be about re-designing the area though - let's keep it about the technical changes. IIRC there is another bug open for user visible changes. I'm open to backing out from central as well, but realistically much of this is going to be very similar in the end so not sure it's worth the churn.

Flags: needinfo?(mkmelin+mozilla)

Let's reorganize the work and make this bug a bit more useful.

We can do a local backout of the changes introduced and only implement the main purpose of this bug, which is making Subject field screen reader accessible.

The rest of the introduced changes, which don't really belong here, should be done in the previously closed bug 1745796, which will deal with the replacement of those XUL elements with proper HTML elements, without the need of adding that jumbled and unnecessary CSS.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/356cbd061296
Backed out changesets fe366592cc41, 07e5acae5cb1, f589d5cda09d. r=aleca

They are not being accurately used, and the presence of the "control" attribute causes some screen readers to ignore the explicit set aria-label.

The submitted patch focusses on a small change to fix the accessibility labels, whilst keeping the exiting DOM structure and whilst still using XUL. With the upcoming changes to the header area, these custom elements will probably look quite different with no need for an accessibility hack, or even dropped entirely, so I didn't see much use in converting from XUL yet. Moreover, given these upcoming changes, I feel like this patch will be most useful for esr91 which won't see those changes; the patch should only need a few adjustments that I can put together.

Also, here's the try run for the patch: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6b6c9559087d5c9f298664c81b0c6acbae3d1598

Target Milestone: 97 Branch → 99 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1336b4a32e24
Stop settings "control" and "aria-labelledby" attributes of message header labels and values. r=aleca a=mkmelin

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

Similar patch for esr 91. Subject is now correctly read.

NOTE: Orca doesn't work that well with multiple addresses in a field on 91 -- when focusing the first address it will read out all the addresses and the word "label" -- but that was the same before this patch.

Asking for a quick review now since I had to make some changes from the original patch.

Attachment #9263024 - Flags: review?(alessandro)

Comment on attachment 9262623 [details]
Bug 1589861 - Stop settings "control" and "aria-labelledby" attributes of message header labels and values. r=aleca

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Screen reader will not read out the subject when it is focussed. Other fields, like "Website" similarly affected.
Testing completed (on c-c, etc.): Tested with screen readers, and the existing test browser_messageHeader.js which tests the accessible name
Risk to taking this patch (and alternatives if risky): Low/Medium. The code was changed in a few different places, but overall it is now simpler.

NOTE: Only this patch needs to be uplifted to beta.

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

Comment on attachment 9262623 [details]
Bug 1589861 - Stop settings "control" and "aria-labelledby" attributes of message header labels and values. r=aleca

[Triage Comment]
Approved for 98.0b1

Attachment #9262623 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9257198 - Attachment is obsolete: true
Attachment #9257079 - Attachment is obsolete: true
Attachment #9257069 - Attachment is obsolete: true
Comment on attachment 9263024 [details] [diff] [review]
bug1589861-esr91.patch

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

Looks good, thanks.
You can request an approval-comm-esr91 without setting checking-needed-tb.
Attachment #9263024 - Flags: review?(alessandro) → review+

Comment on attachment 9263024 [details] [diff] [review]
bug1589861-esr91.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Screen reader will not read out the subject when it is focussed. Other fields, like "Website" similarly affected.
Testing completed (on c-c, etc.): Tested with screen readers, and the existing test browser_messageHeader.js which tests the accessible name
Risk to taking this patch (and alternatives if risky): Low/Medium. The code was changed in a few different places, but overall it is now simpler.

Attachment #9263024 - Flags: approval-comm-esr91?

Comment on attachment 9263024 [details] [diff] [review]
bug1589861-esr91.patch

[Triage Comment]
Approved for esr91

Attachment #9263024 - Flags: approval-comm-esr91? → approval-comm-esr91+
See Also: → 1764652
You need to log in before you can comment on or make changes to this bug.