Content of "Subject" field in headers pane not read by screen readers
Categories
(Thunderbird :: Disability Access, defect, P1)
Tracking
(thunderbird_esr91+ fixed, thunderbird98 fixed)
People
(Reporter: k.kolev1985, Assigned: henry-x)
References
Details
(Whiteboard: [works for braille but not speech synth])
Attachments
(3 files, 13 obsolete files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
rjl
:
approval-comm-beta+
|
Details |
(deleted),
patch
|
aleca
:
review+
wsmwk
:
approval-comm-esr91+
|
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:
- Launch a screen reader like NVDA or Narrator.
- Launch Thunderbird 68.
- Make sure that the message preview pane is enabled.
- Navigate to/select a folder containing a few messages.
- Select a message with a filled "Subject" field from the list, so it appears in the message preview pane.
- 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.
Comment 1•5 years ago
|
||
Alex and Jean-Philippe, there's more and even more.
Comment 2•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 4•5 years ago
|
||
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)
- "Subject" (row header)
- [nameless] (table row)
- "To" (row header)
- "To" (label)
- [nameless] (table cell)
- "To" (unknown)
- "To: john@doe.tld" (label)
- "To: john@doe.tld" (unknown)
- "Me" (label)
- "Add to Address Book" (image)
- "To: john@doe.tld" (unknown)
- "To: john@doe.tld" (label)
- "To" (unknown)
- "To" (row header)
- [nameless] (table row)
- [nameless] (table)
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).
Comment 5•5 years ago
|
||
Did you try a nightly build where bug 1589859 has landed?
Comment 6•5 years ago
|
||
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.
Comment 7•5 years ago
|
||
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.
Comment 8•5 years ago
|
||
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)
Comment 9•5 years ago
|
||
Actually, it seems there is a (mozilla internal) role="label".
Comment 10•5 years ago
|
||
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").
Comment 11•4 years ago
|
||
I guess if role="heading" helps, we could just add that.
Comment 12•4 years ago
|
||
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?
Comment 14•4 years ago
|
||
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?
Updated•4 years ago
|
Comment 15•4 years ago
|
||
-
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.
-
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.
Comment 16•3 years ago
|
||
This patch does the following:
- 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)
- Convert label to div; inline textContent dtds.
- 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.
Comment 17•3 years ago
|
||
Updated•3 years ago
|
Comment 18•3 years ago
|
||
(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.
Comment 19•3 years ago
|
||
Comment 20•3 years ago
|
||
Store headerName attr for filters.
Comment 21•3 years ago
|
||
Probably a try would also be good.
Comment 22•3 years ago
|
||
Comment 23•3 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #22)
Comment on attachment 9256541 [details] [diff] [review]
a11yheaderfield.patchReview 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.
Comment 24•3 years ago
|
||
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.
Comment 25•3 years ago
|
||
Tags row, implements ul, which works nicely with a screen reader (counts the items).
Comment 26•3 years ago
|
||
The messageId rows. Also fixes some linting, datetime wrapping/flex.
Comment 27•3 years ago
|
||
Comment 28•3 years ago
|
||
Updated•3 years ago
|
Comment 29•3 years ago
|
||
Please consider moving to Phabricator, it's really a much nicer review experience.
Comment 30•3 years ago
|
||
For newsgroups; the new look is "'button", like the crypto button, also fix img selfclosing tags. The next patch will be formatting/indent/whitespace.
Comment 31•3 years ago
|
||
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.
Comment 32•3 years ago
|
||
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
Comment 33•3 years ago
|
||
The one patch had wrong bug number (bug 1745796)
https://hg.mozilla.org/comm-central/rev/f589d5cda09d
Assignee | ||
Comment 34•3 years ago
|
||
I noticed some semantic/accessibility problems with the pushed patches:
- The
MozMessageHeader
are being given the ariarole="menu"
even though they aren't menus. Not sure what the intention was. - Moreover,
message-header-url
is a<div role=menu>
rather than an<a>
. Other values should similarly be<a>
elements. mail-messageids-headerfield
was changed to a<ul>
, but some should only ever contain one value. E.g.expandedmessage-idRow
that shows theMessage ID
should one ever be one value.- The
toggle-list-button
has the wrong accessible name (it is currently using the field value). Plus, the<img>
inside thetoggle-list-button
is missing analt
attribute. This should probably be something like "Expand"/"Collapse", depending onaria-expanded
state, and this can naturally act as the accessible name of the button. - Its not clear why
dateLabel
anddateLabelSubject
were given atabindex=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. - Similarly,
message-header
andmessage-header-value
entries are giventabindex=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.
Assignee | ||
Comment 35•3 years ago
|
||
Comment 37•3 years ago
|
||
(In reply to Henry Wilkes [:henry] from comment #34)
I noticed some semantic/accessibility problems with the pushed patches:
- The
MozMessageHeader
are being given the ariarole="menu"
even though they aren't menus. Not sure what the intention was.
Because the fields all have a contextmenu.
- 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.
mail-messageids-headerfield
was changed to a<ul>
, but some should only ever contain one value. E.g.expandedmessage-idRow
that shows theMessage 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.
- The
toggle-list-button
has the wrong accessible name (it is currently using the field value). Plus, the<img>
inside thetoggle-list-button
is missing analt
attribute. This should probably be something like "Expand"/"Collapse", depending onaria-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.
- Its not clear why
dateLabel
anddateLabelSubject
were given atabindex=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.
- Similarly,
message-header
andmessage-header-value
entries are giventabindex=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 CSSflex: 4
, which overwrites theflex: 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.
Assignee | ||
Comment 38•3 years ago
|
||
(In reply to alta88 from comment #37)
- The
MozMessageHeader
are being given the ariarole="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.
- 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).
- The
toggle-list-button
has the wrong accessible name (it is currently using the field value). Plus, the<img>
inside thetoggle-list-button
is missing analt
attribute. This should probably be something like "Expand"/"Collapse", depending onaria-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.
- Its not clear why
dateLabel
anddateLabelSubject
were given atabindex=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.
Comment 39•3 years ago
|
||
update formatting, fix lint.
Comment 40•3 years ago
|
||
(In reply to Henry Wilkes [:henry] from comment #38)
(In reply to alta88 from comment #37)
- The
MozMessageHeader
are being given the ariarole="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 therole
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?
- 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 validhref
attribute (doesn't need to be http), it will be automatically focusable, styled correctly, and you only need to connect to theclick
signal (which unlike<div>
will cover both a mouse click and keyboard Enter).
To followup.
- The
toggle-list-button
has the wrong accessible name (it is currently using the field value). Plus, the<img>
inside thetoggle-list-button
is missing analt
attribute. This should probably be something like "Expand"/"Collapse", depending onaria-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.
- Its not clear why
dateLabel
anddateLabelSubject
were given atabindex=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.
Comment 41•3 years ago
|
||
(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.
Comment 42•3 years ago
|
||
Re role="menu", apparently there is aria-haspopup="menu" which would seem appropriate
Comment 43•3 years ago
|
||
Comment 44•3 years ago
|
||
Comment 45•3 years ago
|
||
(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/f589d5cda09dSince 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.patchReview 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.patchReview 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.
Comment 46•3 years ago
|
||
Move dom code from constructor in messageIds and newsgroups.
Comment 47•3 years ago
|
||
(In reply to Henry Wilkes [:henry] from comment #35)
Comment on attachment 9257691 [details] [diff] [review]
format.patchReview 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. :/
Comment 48•3 years ago
|
||
Comment 49•3 years ago
|
||
Comment 50•3 years ago
|
||
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 51•3 years ago
|
||
Comment 52•3 years ago
|
||
Comment 53•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 54•3 years ago
|
||
Comment 55•3 years ago
|
||
Assigning this to Henry to continue the work and fix some issues.
Assignee | ||
Comment 56•3 years ago
|
||
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).
Comment 57•3 years ago
|
||
I'm okay with doing a backout, hopefully things are not tangled and it would be straightforward.
Magnus, what do you think>
Comment 58•3 years ago
|
||
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.
Comment 59•3 years ago
|
||
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.
Assignee | ||
Comment 60•3 years ago
|
||
Assignee | ||
Comment 61•3 years ago
|
||
Try run for the back out: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a3e8ba1d13049243d16d30c756ae7a5b566b465d Seems ok.
Comment 62•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/356cbd061296
Backed out changesets fe366592cc41, 07e5acae5cb1, f589d5cda09d. r=aleca
Assignee | ||
Comment 63•3 years ago
|
||
They are not being accurately used, and the presence of the "control" attribute causes some screen readers to ignore the explicit set aria-label.
Assignee | ||
Comment 64•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 65•3 years ago
|
||
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
Assignee | ||
Comment 66•3 years ago
|
||
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.
Assignee | ||
Comment 67•3 years ago
|
||
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.
Comment 68•3 years ago
|
||
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
Comment 69•3 years ago
|
||
bugherder uplift |
Thunderbird 98.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/5b5ed57a340e
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 70•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 71•3 years ago
|
||
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.
Comment 72•3 years ago
|
||
Comment on attachment 9263024 [details] [diff] [review]
bug1589861-esr91.patch
[Triage Comment]
Approved for esr91
Comment 73•3 years ago
|
||
bugherder uplift |
Thunderbird 91.7.0:
https://hg.mozilla.org/releases/comm-esr91/rev/288665ae15bc
Comment 74•3 years ago
|
||
bugherder uplift |
Thunderbird 91.7.0:
https://hg.mozilla.org/releases/comm-esr91/rev/c936367e9d73
Updated•3 years ago
|
Description
•