1498606 - [de-xbl] Remove mail-messageid binding.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Description

[Arshad Khan [:arshad]]

https://searchfox.org/comm-central/source/mail/base/content/mailWidgets.xml#923

Comment 1

[Arshad Khan [:arshad]]

Attachment: mail-messageid.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9016932 [details] [diff] [review]
mail-messageid.patch

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

You lost the context menu, which is the main reason for this binding to exits. 

Also, looks like messageIdDisplayImage is just used for padding so we can drop that and inline the label text instead, leading to quite a bit of simplification

Comment 3

[Arshad Khan [:arshad]]

Attachment: mail-messageid.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9017144 [details] [diff] [review]
mail-messageid.patch

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

Please test your patches.
This one is not working, apparently since the message id fields have < and > in them and then probably some escaping needs to take place. With the patch, all the fields are invisible

::: mail/base/content/mailWidgets.js
@@ +130,5 @@
> +    this._updateAttributes();
> +  }
> +
> +  _updateAttributes() {
> +    this.textContent =  this.label || "";

there's a double space here

@@ +134,5 @@
> +    this.textContent =  this.label || "";
> +  }
> +
> +  set label(val) {
> +    this.setAttribute("label", val);

for val null, please remove attribute

Comment 5

[Arshad Khan [:arshad]]

Attachment: mail-messageid.patch

Comment 6

[Arshad Khan [:arshad]]

Comment on attachment 9017833 [details] [diff] [review]
mail-messageid.patch

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

::: mail/base/content/mailWidgets.js
@@ +124,5 @@
> +    this.classList.add("messageIdDisplayButton");
> +    this.setAttribute("context", "messageIdContext");
> +    this._updateAttributes();
> +
> +    this.addEventListener("click", (event) => {

Should we remove the event listener? can i case arise where we want to do this?

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9017833 [details] [diff] [review]
mail-messageid.patch

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

This is working now. r=mkmelin with the small change below

The event listener should be kept. Clicking on message ids is fairly useful for instance for References, so you can go and open the original message this message was a reply to.

::: mail/base/content/mailWidgets.js
@@ +138,5 @@
> +    this.textContent = this.label || "";
> +  }
> +
> +  set label(val) {
> +    if (val === null || val === undefined) {

use

if (val == null)

instead. this will match both null and undefined

Comment 8

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73f75351f7bea768c888416ce23a89b34efde138

Comment 9

[Arshad Khan [:arshad]]

Attachment: mail-messageid.patch

Comment 10

[Arshad Khan [:arshad]]

Attachment: mail-messageid.patch

Comment 11

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a9f525b48d56f4e1bc6dd51c6106d499f20d0188

Comment 12

[Arshad Khan [:arshad]]

hopefully with correct tip - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f98c8de6ff150374517c0dba8bada2d833104812

Comment 13

[Jorg K (CEST = GMT+2)]

Yes, things have been bumpy today :-(

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/baf2d45077353ff89255ffedcfea660bda226e4d
Bug 1498606 - Migrate mail-messageid binding to custom element. r=mkmelin DONTBUILD