Closed Bug 1491698 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: Mail Window Front End, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 65.0

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 22 obsolete files)

26.87 KB, patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
Assignee: nobody → arshdkhn1
Attached 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.
Attached 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)
Attached 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
Attached 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)
Attached 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
Attached 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)
Attached 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)
Attached 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.
Attached 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+
Attached 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: 6 years 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
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9015519 - Attachment is obsolete: true
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9017151 - Attachment is obsolete: true
Attached 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)
Attached 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)
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018873 - Attachment is obsolete: true
Attachment #9018873 - Flags: review?(mkmelin+mozilla)
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
Attachment #9018876 - Attachment is obsolete: true
Attached patch mail-emailaddress.patch (obsolete) — Splinter Review
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-
Attached 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)
Attached 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+
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: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Depends on: 1514951
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: