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)
Thunderbird
Mail Window Front End
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Assignee: nobody → arshdkhn1
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #9009471 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
oops. thanks ntim.
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #9009472 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•6 years ago
|
||
Magnus, could you please take a look at the comments I made for some changes in the attachemnt?
Flags: needinfo?(mkmelin+mozilla)
Comment 8•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 9•6 years ago
|
||
This patch also not work because of the same NotSupported errors.. Addressed the nits tho.
Attachment #9011448 -
Attachment is obsolete: true
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9012144 -
Attachment is obsolete: true
Attachment #9014321 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Assignee | ||
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
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)
Assignee | ||
Comment 15•6 years ago
|
||
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•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
(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•6 years ago
|
||
(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?
Assignee | ||
Comment 19•6 years ago
|
||
(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
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #9014321 -
Attachment is obsolete: true
Attachment #9014447 -
Attachment is obsolete: true
Assignee | ||
Comment 21•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Attachment #9015015 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 22•6 years ago
|
||
Attachment #9015015 -
Attachment is obsolete: true
Attachment #9015015 -
Flags: review?(mkmelin+mozilla)
Attachment #9015020 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 23•6 years ago
|
||
(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•6 years ago
|
||
(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.)
Assignee | ||
Comment 25•6 years ago
|
||
(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.
Assignee | ||
Comment 26•6 years ago
|
||
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•6 years ago
|
||
correct
Assignee | ||
Comment 28•6 years ago
|
||
Attachment #9015020 -
Attachment is obsolete: true
Attachment #9015020 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9015077 -
Flags: review?(mkmelin+mozilla)
Comment 29•6 years ago
|
||
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)
Assignee | ||
Comment 30•6 years ago
|
||
Attachment #9015077 -
Attachment is obsolete: true
Attachment #9015208 -
Flags: review?(mkmelin+mozilla)
Comment 31•6 years ago
|
||
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)
Assignee | ||
Comment 32•6 years ago
|
||
(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.
Assignee | ||
Comment 33•6 years ago
|
||
Attachment #9015208 -
Attachment is obsolete: true
Attachment #9015503 -
Flags: review?(mkmelin+mozilla)
Comment 34•6 years ago
|
||
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+
Assignee | ||
Comment 35•6 years ago
|
||
Attachment #9015503 -
Attachment is obsolete: true
Attachment #9015519 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 36•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f6d8ba2898f9
Convert mail-emailaddress binding to custom element. r=mkmelin
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 64.0
Comment 37•6 years ago
|
||
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 → ---
Comment 38•6 years ago
|
||
Assignee | ||
Comment 39•6 years ago
|
||
I looked at the try run and I got confused that the test fails were pre existing. I ll be careful.
Assignee | ||
Comment 40•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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
Assignee | ||
Comment 43•6 years ago
|
||
Attachment #9015519 -
Attachment is obsolete: true
Assignee | ||
Comment 44•6 years ago
|
||
Attachment #9017151 -
Attachment is obsolete: true
Assignee | ||
Comment 45•6 years ago
|
||
Attachment #9018845 -
Attachment is obsolete: true
Assignee | ||
Comment 46•6 years ago
|
||
try run after fixin tests - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eb72c22f64a89da3c5ca79a1584b7a998be3f699
Assignee | ||
Comment 47•6 years ago
|
||
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)
Assignee | ||
Comment 48•6 years ago
|
||
Attachment #9018846 -
Attachment is obsolete: true
Attachment #9018846 -
Flags: review?(mkmelin+mozilla)
Attachment #9018873 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 49•6 years ago
|
||
Attachment #9018873 -
Attachment is obsolete: true
Attachment #9018873 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 50•6 years ago
|
||
Attachment #9018876 -
Attachment is obsolete: true
Assignee | ||
Comment 51•6 years ago
|
||
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 52•6 years ago
|
||
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-
Assignee | ||
Comment 53•6 years ago
|
||
hey cann you please check the test again quickly?
Attachment #9018935 -
Attachment is obsolete: true
Attachment #9018994 -
Flags: review?(mkmelin+mozilla)
Comment 54•6 years ago
|
||
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-
Assignee | ||
Comment 55•6 years ago
|
||
Attachment #9018994 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9020931 -
Flags: review+
Comment 56•6 years ago
|
||
Comment on attachment 9020931 [details] [diff] [review]
mail-emailaddress_1.patch
Final check, no?
Attachment #9020931 -
Flags: review+ → review?(mkmelin+mozilla)
Assignee | ||
Comment 57•6 years ago
|
||
Attachment #9020931 -
Attachment is obsolete: true
Attachment #9020931 -
Flags: review?(mkmelin+mozilla)
Attachment #9021119 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 58•6 years ago
|
||
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 59•6 years ago
|
||
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+
Comment 60•6 years ago
|
||
(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•6 years ago
|
||
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)
Assignee | ||
Comment 62•6 years ago
|
||
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+
Assignee | ||
Comment 63•6 years ago
|
||
Flags: needinfo?(mkmelin+mozilla)
Comment 64•6 years ago
|
||
Looks good to me. If the error you found turned out to be nothing, this is ready to go
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 65•6 years ago
|
||
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 ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 65.0
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•