This will improve row alignment. It will also be possible for users to add a separator, eg : using

.message-header-label:after {
  content: attr(value)":";
}

which isn't possible with an xul label.

Of course. It can also be just label in that case, but it cannot be just label in the xhtml file (there, an instanceOf HTMLElement returns false). It was done so for hardcoded and generated nodes to be consistent until the global namespace eventually changes.

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

Comment on attachment 9255080 [details] [diff] [review]
htmllabel.patch

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

Thanks for proposing these changes, indeed we need to convert those XUL
label into HTML labels.
Unfortunately, this patch is wrong and it needs more work alongside other
areas in order to land.

These are the most glaring issues I found:

The <html:label> is not a self closing tag, so it should be closed with
</html:label>.

Correct, but prior to actual textContent/fluent it's not important.

Also that element doesn't have a value attribute, so those strings should
be converted into fluent strings and added as regular text.

It is not in the scope of this patch to change over to fluent. That's why the strings were not inlined in <label> tags, as that work logically belongs in a fluent conversion.

A11y and screen readers don't work with this since we can't use an HTML
label with a XUL element, as those header fields are still that blob nested
XUL elements.
With this patch, the subject line is read as "Undefined: <text of the
subject>", instead is usually read as "Subject: <text of the subject>".
Same issue with the recipients, as they're usually read as "To:
<recipient-email>", "Cc: <recipient-email>", etc. But now they're read as
"[Attr after] <recipient-email>".

I didn't expect this to be noticed. The simple fix for the typo is to add .value to the attributes.value object. Given that, please verify whether the above comment is correct.

The vertical alignment of the rows holding addresses is slightly off for me,
and the text printed as pseudo element seems to not entirely respect line
height and flex alignment.

This patch requires the patch in the blocking bug, was it applied?

I think these changes should be done at the same time of changing those
header fields to HTML Custom Elements.

That certainly is not necessary, nor the practice in the incrementalist approach dealing with the headers widget, or indeed anywhere. Neither the de-xbl bug, Bug 1556261 had a requirement to 1) de xul completely, 2) convert to fluent, 3) ensure row alignment (a new thing since we're being pixel perfect). So I find this contradictory and confusing, or worse.

Sorry Henry for hijacking the review.

::: mail/base/content/msgHdrView.js
@@ -1257,5 @@

   // Create new collapsed row.

•  newRowNode = document.createElementNS(
  

•    "http://www.w3.org/1999/xhtml",
  

•    "div"
  

•  );
  

Please don't remove this as it's needed in order to create the correct
html:div element, otherwise this will create a <div>` which in the XUL
context it will handled like a box.

createElement() was changed long ago now to default to the html namespace, there is no difference; a <div> created both ways is identical. Please explain what "handled like a box" means.

@@ +1260,5 @@

   newRowNode.classList.add("message-header-row");
   newRowNode.hidden = true;

   // Create and append the label which contains the header name.

•  let newLabelNode = document.createElement("html:label");
  

This is wrong, it should be
document.createElementNS("http://www.w3.org/1999/xhtml", "label")

Namespace unnecessary as above, "label" is fine too; see comment 3 for using a mnemonic temporarily for the transitory nature.

::: mail/themes/shared/mail/messageHeader.css
@@ -290,3 @@

margin-inline: 6px 8px;
text-align: end;

• flex-shrink: 0;

If you remove the not-shrinking option, labels like Reply To, Message ID,
etc, will wrap text early when not necessary.

That rule has no effect due to syncGridColumnWidths() setting a minWidth on the labels. Such a practice is incorrect, however, and should be removed in the spirit of Bug 1745619. There are several failures to leverage css correctly in the codebase and rely on calculations instead. For this case, where multiword short text should not wrap, the proper text centric rule is white-space: nowrap; or similar.

Of course, if #messageHeader container were a grid with 2 columns (one for label and one for the rest of the row) then css would do all the work. Not in the scope of this patch to fix that.

@@ +291,5 @@

}

+.message-header-label:after {

• content: attr(value);
  +}

I'm not sure I like this. Pseudo elements should be used for showing
temporary or extra floating or inline elements, not static text that should
be part of the regular DOM.

This is a common pattern in Fx. Can you cite a best practices source for this contention?

That said, this is an incremental step toward a patch with a fluent textContent conversion.

1. close <label> tags
2. use "label" in dynamic creation
3. fix aria-label attributes.value.value typo

True, the custom xul elements are not labelable elements. The goal here is to replace xul label with an interim html element for the header label, with the smallest incremental code change, pending (another) rearchitecture of the widget in pure html.

Probably best to just use div. aria-labelledby already handles the appropriate association between the elements.

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

Probably best to just use div. aria-labelledby already handles the appropriate association between the elements.

Even then, I'm not sure using aria-label and aria-labelledby makes sense. E.g. the "To" field's accessibility tree is currently

label: "To"
nothing: "To"
  label: "To: my-address@server , To: other@server"
    nothing: "To: my-address@server"
      label: "To: my-address@server"
      pushbutton: "Address is in the Address Book"
    label: ","
    nothing: "To: other@server"
      label: "To: other@server"
      pushbutton: "Address is in the Address Book"
  label: "1 more"

I'm not sure it makes sense to make small changes without taking the end structure into account. E.g. this field should probably be something along the lines of an <ul> or listbox.

Years ago, I had an extension that rewrote this widget in <table>. Is that the end structure? Or is it <dl>, or a series of <div> with css grid.

This is fundamentally a two column grid, key-value. Some of the value columns have a bit of extra complexity (toolbar, cryptobox, date).

@henry - So what should the tree look like for multiple To fields? It looks worse with <div>, but that's probably due to those hacks that can be removed.

This is the accessibility tree for the To row with 3 addresses, with all aria (and other) hacks removed and the header label as a <div>. Is it correct?

Years ago, I had an extension that rewrote this widget in <table>. Is that the end structure? Or is it <dl>, or a series of <div> with css grid.

Based on bug 1556261, and attachment 9243995 [details], I don't think this'll be a table.

@henry - So what should the tree look like for multiple To fields? It looks worse with <div>, but that's probably due to those hacks that can be removed.

An unordered list or a listbox are obvious choices for a list of recipients. But this might end up as something else.

This is the accessibility tree for the To row with 3 addresses, with all aria (and other) hacks removed and the header label as a <div>. Is it correct?

I wouldn't say this is "correct" because the label role is misused and the addresses are not linked semantically to the "To".

Basically, this section would benefit from a much larger restructure, which needs to be thought out to get the right semantics (and hence, a clean accessibility tree). So I think there isn't much point in using some one-to-one html replacement for the xul:labels when this isn't addressing a defect, and isn't a step towards the needed restructuring so will likely get replaced.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f589d5cda09d
Convert xul label to html label for message header labels. r=mkmelin

(In reply to Pulsebot from comment #14)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/f589d5cda09d
Convert xul label to html label for message header labels. r=mkmelin

This patch was pushed before it was reviewed or approved, can you reverse it?

The landed patch isn't the same as in this bug. So probably another patch with the wrong commit message.

(In reply to Richard Marti (:Paenglab) from comment #16)

The landed patch isn't the same as in this bug. So probably another patch with the wrong commit message.

It seems related though? And the commit message is the same, although it is misleading since there are no "html label"s in the committed patch.

Plus, quickly looking at the patch there are some formatting issues with the html which break the conventions.

My comment wasn't against a back-out. It was only a remark that the landed patch doesn't relate to this bug. A back-out is needed because of the wrong bug in the commit message.

(In reply to Richard Marti (:Paenglab) from comment #18)

My comment wasn't against a back-out. It was only a remark that the landed patch doesn't relate to this bug. A back-out is needed because of the wrong bug in the commit message.

Sorry, I wasn't being clear. I was just confused about it.

It seems the work was moved to bug 1589861, without referencing this bug or changing the commit message.

Ah yes. Added a comment to bug 1589861.

Marking this one as duplicate since the patch was moved into the other bug. If anyone else can think of something more appropriate, feel free to change it.

Gently pinging Henry on this to highlight its priority and objective for 102

Rev. f589d5cda09d (comment #14) was backed out in bug 1589861 comment #62.

This bug is exclusively focused on replacing the <label>s, which are XUL elements and different from the HTML labels, which shouldn't be used at all since this is not a form.
That backout was from a wrongly done series of patches.

I'm removing the NI since this is not a priority anymore as the message header work was completed for 102.
We can handle this as a regular task and define a proper a11y tree for these elements after the 102 release shenanigans are over.

This was reopened to address msgHeadersToolbar in edit message view, but that should be a new bug. This current bug addresses the Message Reader UI.