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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

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

Comment 2

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 3

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 4

[Tim Nguyen :ntim]

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.

Comment 5

[Arshad Khan [:arshad]]

oops. thanks ntim.

Comment 6

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 7

[Arshad Khan [:arshad]]

Magnus, could you please take a look at the comments I made for some changes in the attachemnt?

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

This patch also not work because of the same NotSupported errors.. Addressed the nits tho.

Comment 10

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 11

[Arshad Khan [:arshad]]

(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.

Comment 12

[Arshad Khan [:arshad]]

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

Comment 13

[Magnus Melin [:mkmelin]]

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.

Comment 14

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress-exp1.patch

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?

Comment 15

[Arshad Khan [:arshad]]

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 16

[Magnus Melin [:mkmelin]]

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?

Comment 17

[Arshad Khan [:arshad]]

(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?

Comment 18

[Magnus Melin [:mkmelin]]

(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?

Comment 19

[Arshad Khan [:arshad]]

(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

Comment 20

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_1.patch

Comment 21

[Arshad Khan [:arshad]]

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

Comment 22

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 23

[Arshad Khan [:arshad]]

(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?

Comment 24

[Magnus Melin [:mkmelin]]

(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.)

Comment 25

[Arshad Khan [:arshad]]

(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 26

[Arshad Khan [:arshad]]

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?

Comment 27

[Magnus Melin [:mkmelin]]

correct

Comment 28

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_1.patch

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

Comment 29

[Magnus Melin [:mkmelin]]

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()
    );

Comment 30

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_2.patch

Comment 31

[Magnus Melin [:mkmelin]]

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

Comment 32

[Arshad Khan [:arshad]]

(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.

Comment 33

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_3.patch

Comment 34

[Magnus Melin [:mkmelin]]

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

Comment 35

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f6d8ba2898f9
Convert mail-emailaddress binding to custom element. r=mkmelin

Comment 37

[Jorg K (CEST = GMT+2)]

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?

Comment 38

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/5489fbd9959b2f3e5109c6c8af10d5bd7a406b01

Comment 39

[Arshad Khan [:arshad]]

I looked at the try run and I got confused that the test fails were pre existing. I ll be careful.

Comment 40

[Arshad Khan [:arshad]]

Magnus, we need to fix Bug 1497656 before landing this patch else from/to url field will also be missing in new window.

Comment 41

[Jorg K (CEST = GMT+2)]

(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 :-(

Comment 42

[Magnus Melin [:mkmelin]]

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

Comment 43

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 44

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 45

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 46

[Arshad Khan [:arshad]]

try run after fixin tests - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eb72c22f64a89da3c5ca79a1584b7a998be3f699

Comment 47

[Arshad Khan [:arshad]]

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?

Comment 48

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 49

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 50

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

Comment 51

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

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

Please review the test files Magnus.

Comment 52

[Magnus Melin [:mkmelin]]

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

Comment 53

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress.patch

hey cann you please check the test again quickly?

Comment 54

[Magnus Melin [:mkmelin]]

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

Comment 55

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_1.patch

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

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020931 [details] [diff] [review]
mail-emailaddress_1.patch

Final check, no?

Comment 57

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_2.patch

Comment 58

[Arshad Khan [:arshad]]

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?

Comment 59

[Magnus Melin [:mkmelin]]

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)

Comment 60

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #59)
> And sent to try now: ...
Note the current test failures, see bug 1502997 comment 10.

Comment 61

[Magnus Melin [:mkmelin]]

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.)

Comment 62

[Arshad Khan [:arshad]]

Attachment: mail-emailaddress_3.patch

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.

Comment 63

[Arshad Khan [:arshad]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1491698#c62

Comment 64

[Magnus Melin [:mkmelin]]

Looks good to me. If the error you found turned out to be nothing, this is ready to go

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0363b1013217
Convert mail-emailaddress to custom element; r=mkmelin