[de-xbl] Convert mail-emailaddress to custom element

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 65.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 22 obsolete attachments)

26.87 KB, patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Assignee: nobody → arshdkhn1
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9009471 - Attachment is obsolete: true
Comment on attachment 9009472 [details] [diff] [review]
mail-emailaddress.patch

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

::: mail/base/content/mailWidgets.js
@@ +100,5 @@
> +        return this.getPart("emailValue").getAttribute("disabled");
> +    }
> +
> +    getPart(aPartId) {
> +        return document.getAnonymousElementByAttribute(this, "anonid", aPartId);

getAnonymousElementByAttribute doesn't work on CE.
oops. thanks ntim.
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9009472 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Magnus, could you please take a look at the comments I made for some changes in the attachemnt?
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9011448 [details] [diff] [review]
mail-emailaddress.patch

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

Please change to 2 space indents

::: mail/base/content/mailWidgets.js
@@ +4,5 @@
> +
> +/* global MozXULElement */
> +
> +class MozMailEmailaddress extends MozXULElement {
> +    static get observedAttributes() {

unused?

@@ +26,5 @@
> +                <image class="emailStar" anonid="emailStar" context="emailAddressPopup" onmousedown="event.preventDefault();" onclick="onClickEmailStar(event, this.parentNode.parentNode);"></image>
> +                <image class="emailPresence" anonid="emailPresence" onmousedown="event.preventDefault();" onclick="onClickEmailPresence(event, this.parentNode.parentNode);"></image>
> +            </hbox>
> +            `)
> +        );

is this allowed, and working inside the constructor? why not in the connectedCallback?

@@ +61,5 @@
> +
> +        this.updateNodeAttributes(emailDisplayButton, emailDisplayButtonObservedAttrs);
> +        this.updateNodeAttributes(emailLabel, emailLabelObservedAttrs);
> +        this.updateNodeAttributes(emailStar, emailStarObservedAttrs);
> +        this.updateNodeAttributes(emailPresence, emailPresenceAttrs);

for all of these it would be much more readable to just inline it all. 
I mean like

this.updateNodeAttributes(this.querySelector(".emailDisplayButton"), ["hascard", "aria-label"]);

@@ +66,5 @@
> +    }
> +
> +    updateNodeAttributes(attrNode, attrs) {
> +        attrs.forEach(attr => {
> +            if ((typeof attr === "string") && this.hasAttribute(attr)) {

how can attr be anytying but a string?

::: mail/base/content/mailWidgets.xml
@@ +503,1 @@
>      </content>

line up attributes (anonid and class beneath each others)
but why does this change to <html:... ?

@@ +572,3 @@
>              // <mail-emailaddress>, screen readers don't know to automagically
>              // prepend the label when reading the tag, so we force this.
>              // Furthermore, at least on Mac, there is no JS labelElement

you have a mac right? I suspect this is nonsense. Otherwise tests would be failing.

::: mail/base/content/msgHdrView.js
@@ +1241,4 @@
>    var index = 0;
>    if (headerEntry.useToggle)
>      headerEntry.enclosingBox.resetAddressView(); // make sure we start clean
> +

remove empty change
Flags: needinfo?(mkmelin+mozilla)
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
This patch also not work because of the same NotSupported errors.. Addressed the nits tho.
Attachment #9011448 - Attachment is obsolete: true
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9012144 - Attachment is obsolete: true
Attachment #9014321 - Flags: review?(mkmelin+mozilla)
(In reply to Magnus Melin [:mkmelin] from comment #8)
> Comment on attachment 9011448 [details] [diff] [review]
> mail-emailaddress.patch
> 
> Review of attachment 9011448 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +66,5 @@
> > +    }
> > +
> > +    updateNodeAttributes(attrNode, attrs) {
> > +        attrs.forEach(attr => {
> > +            if ((typeof attr === "string") && this.hasAttribute(attr)) {
> 
> how can attr be anytying but a string?

if you see the update method you ll find out I am passing either string or an object to updateattributes method.. the reason is the observedAttribute is mapped to some other attributes of child element.. for ex - label attribute is mapped to value of <label /> element.. If you dont like the way I am setting up attributes, please lemme know a better way to do this... I ll be very happy to change it.
Blocks: 1496354
No longer blocks: 1496354
Comment on attachment 9014321 [details] [diff] [review]
mail-emailaddress.patch

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

::: mail/base/content/mailWidgets.js
@@ +41,5 @@
> +    ];
> +  }
> +
> +  connectedCallback() {
> +    const hbox = document.createElement("hbox");

can we drop the hbox, and let it be "this" instead?

@@ +42,5 @@
> +  }
> +
> +  connectedCallback() {
> +    const hbox = document.createElement("hbox");
> +    hbox.setAttribute("class", "emailDisplayButton");

if so, use this.classList.add("class")

@@ +43,5 @@
> +
> +  connectedCallback() {
> +    const hbox = document.createElement("hbox");
> +    hbox.setAttribute("class", "emailDisplayButton");
> +    hbox.setAttribute("align", "center");

odd. probably not needed

@@ +46,5 @@
> +    hbox.setAttribute("class", "emailDisplayButton");
> +    hbox.setAttribute("align", "center");
> +    hbox.setAttribute("context", "emailAddressPopup");
> +    hbox.setAttribute("popup", "emailAddressPopup");
> +    hbox.setAttribute("flex", "1");

flex should move to the caller then

@@ +48,5 @@
> +    hbox.setAttribute("context", "emailAddressPopup");
> +    hbox.setAttribute("popup", "emailAddressPopup");
> +    hbox.setAttribute("flex", "1");
> +    hbox.setAttribute("role", "textbox");
> +    hbox.setAttribute("aria-readonly", "true");

and these can go

@@ +57,5 @@
> +    const emailStarImage = document.createElement("image");
> +    emailStarImage.setAttribute("class", "emailStar");
> +    emailStarImage.setAttribute("context", "emailAddressPopup");
> +    emailStarImage.onmousedown = (event) => event.preventDefault();
> +    emailStarImage.onclick = (event) => onClickEmailStar(event, this);

perhaps prefer addEventListener while we're at it

@@ +147,5 @@
> +          attrNode.setAttribute(attr, this.getAttribute(attr));
> +        } else {
> +          attrNode.removeAttribute(attr);
> +        }
> +      } else if (typeof attr === "object") {

yep please refactor this, it's utterly unreadable. Maybe you need a separate _updateNodeAttributes2(attrNode, attr, mapped) function (with better names of course).


In general i really dislike having var-type arguments.
Attachment #9014321 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress-exp1.patch (obsolete) — Splinter Review
Magnus, can you tell why clicking on the menuitem of popup doesn't work as expected? and what are the other issues with this patch?
Attachment #9014447 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9014447 [details] [diff] [review]
mail-emailaddress-exp1.patch

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

::: mail/base/content/msgHdrView.js
@@ +1470,4 @@
>  
>  function setupEmailAddressPopup(emailAddressNode)
>  {
> +  emailAddressNode = emailAddressNode.closest(".emailDisplayButton");

sometimes, document.popup node is label element.. I guess this is because sometimes label element is the one picked up as the elment causing the popup show..
Comment on attachment 9014447 [details] [diff] [review]
mail-emailaddress-exp1.patch

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

Didn't try it out yet. Remember to update the commit msg

::: mail/base/content/mailWidgets.js
@@ +44,5 @@
>    connectedCallback() {
> +    this.classList.add("emailDisplayButton");
> +    this.setAttribute("context", "emailAddressPopup");
> +    this.setAttribute("popup", "emailAddressPopup");
> +    this.setAttribute("flex", "1");

please put the flex="1" in the caller instead

::: mail/base/content/msgHdrView.js
@@ +1470,4 @@
>  
>  function setupEmailAddressPopup(emailAddressNode)
>  {
> +  emailAddressNode = emailAddressNode.closest(".emailDisplayButton");

You probably want to include "this" node too?
Attachment #9014447 - Flags: feedback?(mkmelin+mozilla)
(In reply to Magnus Melin [:mkmelin] from comment #16)
> Comment on attachment 9014447 [details] [diff] [review]
> mail-emailaddress-exp1.patch
> 
> Review of attachment 9014447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Didn't try it out yet. Remember to update the commit msg
> 
> ::: mail/base/content/mailWidgets.js
> @@ +44,5 @@
> >    connectedCallback() {
> > +    this.classList.add("emailDisplayButton");
> > +    this.setAttribute("context", "emailAddressPopup");
> > +    this.setAttribute("popup", "emailAddressPopup");
> > +    this.setAttribute("flex", "1");
> 
> please put the flex="1" in the caller instead
you mean on the xul tag or if it is created by js then add another line to set flex attr?
> ::: mail/base/content/msgHdrView.js
> @@ +1470,4 @@
> >  
> >  function setupEmailAddressPopup(emailAddressNode)
> >  {
> > +  emailAddressNode = emailAddressNode.closest(".emailDisplayButton");
> 
> You probably want to include "this" node too?
I am not sure what do you mean?
(In reply to Arshad Khan [:arshad] from comment #17)

> > please put the flex="1" in the caller instead
> you mean on the xul tag or if it is created by js then add another line to

The custom element tag yes

> > ::: mail/base/content/msgHdrView.js
> > @@ +1470,4 @@
> > >  
> > >  function setupEmailAddressPopup(emailAddressNode)
> > >  {
> > > +  emailAddressNode = emailAddressNode.closest(".emailDisplayButton");
> > 
> > You probably want to include "this" node too?
> I am not sure what do you mean?

That the original argument emailAddressNode can also be the target? Or can't it?
(In reply to Magnus Melin [:mkmelin] from comment #18)
> (In reply to Arshad Khan [:arshad] from comment #17)

> That the original argument emailAddressNode can also be the target? Or can't
> it?

closest will then return itself.. for ex - document.querySelector("#item").closest("#item") will return iteslf back unlike querySelectors which returns null if the item that you r searchin is the node iteself onwhich querySelector is called
Posted patch mail-emailaddress_1.patch (obsolete) — Splinter Review
Attachment #9014321 - Attachment is obsolete: true
Attachment #9014447 - Attachment is obsolete: true
Comment on attachment 9015015 [details] [diff] [review]
mail-emailaddress_1.patch

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

::: mail/base/content/messenger.css
@@ +27,3 @@
>  .emailDisplayButton {
>    -moz-user-focus: normal;
> +  margin: 2px 0;

to make up for the height difference given by hbox that got removed by this patch
Attachment #9015015 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9015015 - Attachment is obsolete: true
Attachment #9015015 - Flags: review?(mkmelin+mozilla)
Attachment #9015020 - Flags: review?(mkmelin+mozilla)
(In reply to Magnus Melin [:mkmelin] from comment #8)
> Comment on attachment 9011448 [details] [diff] [review]
> mail-emailaddress.patch
> 
> Review of attachment 9011448 [details] [diff] [review]:
> -----------------------------------------------------------------
 
> ::: mail/base/content/msgHdrView.js
> @@ +1241,4 @@
> >    var index = 0;
> >    if (headerEntry.useToggle)
> >      headerEntry.enclosingBox.resetAddressView(); // make sure we start clean
> > +
> 
> remove empty change

What do you mean by empty change?
(In reply to Arshad Khan [:arshad] from comment #23)
> What do you mean by empty change?

There was a hunk in that patch with no real change. (Maybe it added a new line in an unrelated place.)
(In reply to Magnus Melin [:mkmelin] from comment #24)
> (In reply to Arshad Khan [:arshad] from comment #23)
> > What do you mean by empty change?
> 
> There was a hunk in that patch with no real change. (Maybe it added a new
> line in an unrelated place.)

i thought you asked me to remove the whole toggle code.. i need to add it now.
Comment on attachment 9015020 [details] [diff] [review]
mail-emailaddress.patch

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

::: mail/base/content/msgHdrView.js
@@ -1239,4 @@
>                               .parseHeadersWithArray(emailAddresses, addresses,
>                                                      names, fullNames);
>    var index = 0;
> -  if (headerEntry.useToggle)

this should not be removed right?
correct
Attachment #9015077 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9015077 [details] [diff] [review]
mail-emailaddress_1.patch

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

::: mail/base/content/mailWidgets.js
@@ +115,5 @@
> +    this._update();
> +  }
> +
> +  set label(val) {
> +    this.querySelector(".emailDisplayButton").setAttribute("label", val);

this is nonsense, and also would not work. emailDisplayButton is "this" now. same for thee other getters/setters

@@ +142,5 @@
> +    return this.querySelector(".emailDisplayButton").getAttribute("disabled");
> +  }
> +
> +  _update() {
> +    if (!this.isConnected || !this._areChildrenAppended) {

you don't need both checks I assume

@@ +179,5 @@
> +    const emailPresenceImage = this.querySelector(".emailPresence");
> +
> +    emailStarImage.addEventListener(
> +      "mousedown", (event) => event.preventDefault()
> +    );

a more standard way to wrap these is

    emailStarImage.addEventListener("mousedown", (event) =>
      event.preventDefault()
    );
Attachment #9015077 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress_2.patch (obsolete) — Splinter Review
Attachment #9015077 - Attachment is obsolete: true
Attachment #9015208 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9015208 [details] [diff] [review]
mail-emailaddress_2.patch

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

It's working, but lets do one more iteration.

::: mail/base/content/mailWidgets.js
@@ +179,5 @@
> +    const emailPresenceImage = this.querySelector(".emailPresence");
> +
> +    emailStarImage.addEventListener("mousedown", (event) => (
> +      event.preventDefault()
> +    ));

looks like this works a bit by accident. it's supposed to be

=> {

});

not just parenthesis

@@ +183,5 @@
> +    ));
> +
> +    emailStarImage.addEventListener("click", (event) => (
> +      onClickEmailStar(event, this)
> +    ));

missing ;. 
in the other listeners too

::: mail/base/content/mailWidgets.xml
@@ +452,5 @@
>  
>    <binding id="mail-emailheaderfield">
>      <content>
> +      <mail-emailaddress class="headerValue" containsEmail="true"
> +                         anonid="emailAddressNode"

remove anonid
(and put it all on the same line, should fit fine)

::: mail/base/content/msgHdrView.js
@@ +1476,2 @@
>    var emailAddressPlaceHolder = document.getElementById("emailAddressPlaceHolder");
> +  var emailAddress = emailAddressNode.querySelector(".emaillabel").value ;

there's an extra space before ;

but this should be

let emailAddress = emailAddressNode.label;

Isn't that the point of having the getters?

@@ +1605,1 @@
>    if (emailAddressNode.cardDetails.card)

There's for sure lot of code like the cardDetails handling that belongs the the CE class. But let's handle such cleanup later
Attachment #9015208 - Flags: review?(mkmelin+mozilla)
(In reply to Magnus Melin [:mkmelin] from comment #31)
> Comment on attachment 9015208 [details] [diff] [review]
> mail-emailaddress_2.patch
> 
> Review of attachment 9015208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's working, but lets do one more iteration.
> 
> ::: mail/base/content/mailWidgets.js
> @@ +179,5 @@
> > +    const emailPresenceImage = this.querySelector(".emailPresence");
> > +
> > +    emailStarImage.addEventListener("mousedown", (event) => (
> > +      event.preventDefault()
> > +    ));
> 
> looks like this works a bit by accident. it's supposed to be
> 
> => {
> 
> });

it is not accident. (e) => (a) and (e) => {a} are both anon function.. first returns a while the other dont return anything. I think it would be better to just use {} instead of ();

> not just parenthesis
> 
> @@ +183,5 @@
> > +    ));
> > +
> > +    emailStarImage.addEventListener("click", (event) => (
> > +      onClickEmailStar(event, this)
> > +    ));
> 
> missing ;. 
> in the other listeners too

When you use () in anon functions, you dont add ; at the end.

> @@ +1605,1 @@
> >    if (emailAddressNode.cardDetails.card)
> 
> There's for sure lot of code like the cardDetails handling that belongs the
> the CE class. But let's handle such cleanup later

hmm, lemme look at that and try to get that in if possible.
Posted patch mail-emailaddress_3.patch (obsolete) — Splinter Review
Attachment #9015208 - Attachment is obsolete: true
Attachment #9015503 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9015503 [details] [diff] [review]
mail-emailaddress_3.patch

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

Looks good, r=mkmelin

::: mail/base/content/msgHdrView.js
@@ +1478,2 @@
>    emailAddressNode.setAttribute("selected", "true");
>    emailAddressPlaceHolder.setAttribute("label", emailAddress);

nit: we don't need the tmp variable you can just do 

emailAddressPlaceHolder.setAttribute("label", emailAddressNode.label);

while there, move emailAddressPlaceHolder delcaration down to before it's used
Attachment #9015503 - Flags: review?(mkmelin+mozilla) → review+
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9015503 - Attachment is obsolete: true
Attachment #9015519 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f6d8ba2898f9
Convert mail-emailaddress binding to custom element. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You had a try run in comment #28 with clear failures in
message-header/test-message-header.js and
multiple-identities/test-display-names.js

These failures are not visible on the C-C after landing that patch. Did you actually *look* at the try run? Do I really have to check everything before landing a patch?
Status: RESOLVED → REOPENED
Flags: needinfo?(arshdkhn1)
Resolution: FIXED → ---
Target Milestone: Thunderbird 64.0 → ---
Depends on: 1497656
Flags: needinfo?(arshdkhn1)
I looked at the try run and I got confused that the test fails were pre existing. I ll be careful.
Magnus, we need to fix Bug 1497656 before landing this patch else from/to url field will also be missing in new window.
(In reply to Arshad Khan [:arshad] from comment #39)
> I looked at the try run and I got confused that the test fails were pre
> existing. I ll be careful.
Usually opt builds are completely green, I try to keep it that way. You can always compare with the last push on the C-C tree. Otherwise there's: https://mzl.la/2gS72WO (bugs with whiteboard "Thunderbird-testfailure"). If in doubt: Ask.

Note: Good that you didn't work here about two years ago; there were always test failures, nothing was ever green and they were mostly unreported :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=721679a692d4ad299885cd606d6eb29da9647e44&selectedJob=203870363

In the log plenty of getAnonymousElementByAttribute errors like below. Since there is no anonymous content anymore you need to update the test so it can find what it's looking for

14:16:31     INFO -  SUMMARY-UNEXPECTED-FAIL | test-display-names.js | test-display-names.js::test_single_identity
14:16:31     INFO -    EXCEPTION: mc.window.document.getAnonymousElementByAttribute(...) is null; can't access its "value" property
14:16:31     INFO -      at: test-display-names.js line 95
14:16:31     INFO -         help_test_display_name test-display-names.js:95 15
14:16:31     INFO -         test_single_identity test-display-names.js:108 3
14:16:31     INFO -         Runner.prototype.wrapper frame.js:584 9
14:16:31     INFO -         Runner.prototype._runTestModule frame.js:654 9
14:16:31     INFO -         Runner.prototype.runTestModule frame.js:700 3
14:16:31     INFO -         Runner.prototype.runTestDirectory frame.js:524 7
14:16:31     INFO -         runTestDirectory frame.js:706 3
14:16:31     INFO -         Bridge.prototype._execFunction server.js:177 10
14:16:31     INFO -         Bridge.prototype.execFunction server.js:181 16
14:16:31     INFO -         Session.prototype.receive server.js:282 3
14:16:31     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

Like https://searchfox.org/comm-central/source/mail/test/mozmill/multiple-identities/test-display-names.js#95
Blocks: 1498594
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9015519 - Attachment is obsolete: true
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9017151 - Attachment is obsolete: true
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018845 - Attachment is obsolete: true
Comment on attachment 9018846 [details] [diff] [review]
mail-emailaddress.patch

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

hey, i changed the test files. I think the test will passs now, I tried runnin the try run for linux and everythin was green. Can you please look at the test files changes and lemme know whther the correct element is being targetted or not?
Attachment #9018846 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018846 - Attachment is obsolete: true
Attachment #9018846 - Flags: review?(mkmelin+mozilla)
Attachment #9018873 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018873 - Attachment is obsolete: true
Attachment #9018873 - Flags: review?(mkmelin+mozilla)
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018876 - Attachment is obsolete: true
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22b207284d71e02aa33faa609a5a64cc621de24a

Please review the test files Magnus.
Attachment #9018906 - Attachment is obsolete: true
Attachment #9018935 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9018935 [details] [diff] [review]
mail-emailaddress.patch

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

::: mail/base/content/mailWidgets.js
@@ +220,5 @@
> +  }
> +
> +  get disabled() {
> +    return this.getAttribute("disabled");
> +  }

I've been thinking about exposing the former properties as getters/setters.
Usually it seem not to make sense. If someone wants to set a property, they can set the attribute. 
Then we don't have to add a bunch of unused code. In xbl, the properties were more commonly needed as there setting an attribute wouldn't mean anything unless you hooked it up to do things.

So please remove get/set for all these three

@@ +248,5 @@
> +  _updateNodeAttributes(attrNode, attr, mappedAttr) {
> +    mappedAttr = mappedAttr || attr;
> +
> +    if (this.hasAttribute(mappedAttr)) {
> +      attrNode.setAttribute(attr, this.getAttribute(mappedAttr));

this should still handle the case for removal, in case this.getAttribute(mappedAttr) == null

::: mail/base/content/msgHdrView.js
@@ +1625,5 @@
>   *                     otherwise only the email address
>   */
>  function CopyEmailNewsAddress(addressNode, aIncludeName = false)
>  {
> +  addressNode = addressNode.closest("mail-emailaddress");

you broke all the newsgroup context items with this patch. need to check the "newsgroup" attribute before going forwards in this method

::: mail/test/mozmill/message-header/test-message-header.js
@@ +810,1 @@
>    let addrs = toDescription.getElementsByTagName('mail-emailaddress');

make mozmill-one SOLO_TEST=message-header/test-message-header.js fails miserably


SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor
  EXCEPTION: toDescription.getElementsByTagName is not a function
    at: test-message-header.js line 810
Attachment #9018935 - Flags: review?(mkmelin+mozilla) → review-
Posted patch mail-emailaddress.patch (obsolete) — Splinter Review
hey cann you please check the test again quickly?
Attachment #9018935 - Attachment is obsolete: true
Attachment #9018994 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9018994 [details] [diff] [review]
mail-emailaddress.patch

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

Nope, still fails. Please try in a VM as we discussed.

SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_clicking_star_opens_inline_contact_editor
  EXCEPTION: toDescription.getElementsByTagName is not a function
    at: test-message-header.js line 810
       subtest_more_widget_star_click test-message-header.js:810 15
       test_clicking_star_opens_inline_contact_editor test-message-header.js:384 3
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-message-header.js::test_msg_id_context_menu
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_address_book_switch_disabled_on_contact_in_mailing_list
  EXCEPTION: toDescription.getElementsByTagName is not a function
    at: test-message-header.js line 810
       subtest_more_widget_star_click test-message-header.js:810 15
       test_address_book_switch_disabled_on_contact_in_mailing_list test-message-header.js:472 3
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-message-header.js::test_add_contact_from_context_menu
SUMMARY-PASS | test-message-header.js::test_that_msg_without_date_clears_previous_headers
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget
  EXCEPTION: Argument 1 of Window.getComputedStyle does not implement interface Element.
    at: test-message-header.js line 727
       help_get_num_lines test-message-header.js:727 15
       subtest_more_widget_display test-message-header.js:737 18
       test_more_widget test-message-header.js:650 3
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_show_all_header_mode
  EXCEPTION: Argument 1 of Window.getComputedStyle does not implement interface Element.
    at: test-message-header.js line 727
       help_get_num_lines test-message-header.js:727 15
       subtest_more_widget_display test-message-header.js:737 18
       test_show_all_header_mode test-message-header.js:699 3
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget_with_maxlines_of_3
  EXCEPTION: Argument 1 of Window.getComputedStyle does not implement interface Element.
    at: test-message-header.js line 727
       help_get_num_lines test-message-header.js:727 15
       subtest_more_widget_display test-message-header.js:737 18
       test_more_widget test-message-header.js:650 3
       test_more_widget_with_maxlines_of_3 test-message-header.js:836 3
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_more_widget_with_disabled_more
  EXCEPTION: Argument 1 of Window.getComputedStyle does not implement interface Element.
    at: test-message-header.js line 727
       help_get_num_lines test-message-header.js:727 15
       test_more_widget_with_disabled_more test-message-header.js:873 21
       Runner.prototype.wrapper frame.js:584 9
       Runner.prototype._runTestModule frame.js:654 9
       Runner.prototype.runTestModule frame.js:700 3
       Runner.prototype.runTestFile frame.js:533 3
       runTestFile frame.js:712 3
       Bridge.prototype._execFunction server.js:177 10
       Bridge.prototype.execFunction server.js:181 16
       Session.prototype.receive server.js:282 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-PASS | test-message-header.js::test_toolbar_collapse_and_expand
SUMMARY-PASS | test-message-header.js::teardownModule
Attachment #9018994 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9020931 - Flags: review+
Comment on attachment 9020931 [details] [diff] [review]
mail-emailaddress_1.patch

Final check, no?
Attachment #9020931 - Flags: review+ → review?(mkmelin+mozilla)
Posted patch mail-emailaddress_2.patch (obsolete) — Splinter Review
Attachment #9020931 - Attachment is obsolete: true
Attachment #9020931 - Flags: review?(mkmelin+mozilla)
Attachment #9021119 - Flags: review?(mkmelin+mozilla)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e62fa11eb2c4d3d14ff4ab5a6ddd64301ef8178c

Magnus, could you please take a look at this patch locally and run the test?
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9021119 [details] [diff] [review]
mail-emailaddress_2.patch

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

I've made the changes below. And sent to try now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f3334ebd54abbb864caf2888938be495e4330d0c

::: mail/base/content/mailWidgets.js
@@ +157,5 @@
> +class MozMailEmailaddress extends MozXULElement {
> +  static get observedAttributes() {
> +    return [
> +      "hascard",
> +      "aria-label",

this isn't needed and does nothing afaict

::: mail/base/content/msgHdrView.js
@@ +1469,5 @@
>  
>  function hideEmailNewsPopup(addressNode)
>  {
> +  addressNode = addressNode.hasAttribute("newsgroup") ?
> +                  addressNode.closest("mail-newsgroup") :

nit: no space before :. (here and elsewhere in the patch)
Attachment #9021119 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin [:mkmelin] from comment #59)
> And sent to try now: ...
Note the current test failures, see bug 1502997 comment 10.
Try looks good, but Arshad says he found something that's wrong and is working on it.

(Regarding the spaces, maybe the space before is common, so we can keep them.)
Flags: needinfo?(mkmelin+mozilla)
Magnus could you please confirm one time, that the changes this patch has are the same as patch for which you'd run the try run so that i can add checkin keyword. Thanks.
Attachment #9021119 - Attachment is obsolete: true
Attachment #9021167 - Flags: review+
https://bugzilla.mozilla.org/show_bug.cgi?id=1491698#c62
Flags: needinfo?(mkmelin+mozilla)
Looks good to me. If the error you found turned out to be nothing, this is ready to go
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0363b1013217
Convert mail-emailaddress to custom element; r=mkmelin
Status: REOPENED → RESOLVED
Closed: 10 months ago9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Depends on: 1514951
You need to log in before you can comment on or make changes to this bug.