[de-xbl] convert mail-messageids-headerfield to custom element
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 9 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
Let's convert mail-messageids-headerfield to a custom element.
This widget is used to show/link messages in the message header. I think shown by default for nntp messages, but if you set the mailnews.headers.showReferences pref to true they show up for mails too (when it's a reply/fwd).
E.g. mail-newsgroups-headerfield will be similar - https://searchfox.org/comm-
central/source/mail/base/content/mailWidgets.js#102, and you should probably add this new widget to that file.
See usage here: https://searchfox.org/comm-central/search?q=mail-messageids-headerfield&path= - the things in suite/ is not used by thunderbird (only seamonkey) and you can just ignore it.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
Comment on attachment 9048722 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch This patch is empty
Assignee | ||
Comment 3•6 years ago
|
||
Oh wow, the patch is completely empty, sorry about that.
After doing hg qrefresh, I did hg export qtip > ~/file-name.patch, but the file came out empty.
What did I do wrong?
Reporter | ||
Comment 4•6 years ago
|
||
Maybe the patch wasn't applied? hg qseries shows the applied ones in color
Assignee | ||
Comment 5•6 years ago
|
||
Yes, it shows the name of the patch in color.
If I trigger hg qdiff, nothing shows up.
Assignee | ||
Comment 6•6 years ago
|
||
Here's a quick report from my terminal, just to see if I'm doing something wrong or messed up my patch.
β comm default hg qpop --all
popping de-xbl-mail-messageids-headerfield
patch queue now empty
β comm default hg pull -u
pulling from https://hg.mozilla.org/comm-central
searching for changes
adding changesets
adding manifests
adding file changes
added 1 changesets with 1 changes to 1 files
new changesets e8be57e6a6c3
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
updated to "85445aba0877: Bug 1531901 - Convert mail-messageids-headerfield to custom element"
1 other heads for branch "default"
β comm default hg qpush
applying de-xbl-mail-messageids-headerfield
patch de-xbl-mail-messageids-headerfield is empty
now at: de-xbl-mail-messageids-headerfield
Reporter | ||
Comment 7•6 years ago
|
||
I'm guessing you don't have any changes there then.
Or maybe you have/had? Unless you did a hg qrefresh they don't get marked as going into the patch. For new files you also of course have to hg add them.
Assignee | ||
Comment 8•6 years ago
|
||
I did a qrefresh
and attached a commit message.
hg qrefresh -m "Bug 1531901 - Convert mail-messageids-headerfield to custom element"
After doing this the patch started showing up as empty.
I guess this i where I messed it up.
Assignee | ||
Comment 9•6 years ago
|
||
Reporter | ||
Comment 10•6 years ago
|
||
Comment on attachment 9048949 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9048949 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +171,4 @@ > } > } > > +class MozMailMessageidsHeaderfiled extends MozXULElement { typo: it's field, not filed Please as you go add jsdoc type documentation about elements. See an example from MozMenulistEditable @@ +190,5 @@ > + }); > + this.iconNode.appendChild(this.toggleIcon); > + this.appendChild(this.iconNode); > + > + this.headerNode = document.createElement("hbox"); XBL didn't use to be able to be it's own node, but now that this becomes a custom element you're likely able to remove this extra hbox internal element and let the CE itself be the element. Like MozMailNewsgroupsHeaderfield. (Some css then likely needd to be adjusted accordingly.) @@ +205,5 @@ > + toggleWrap() { > + const headerValue = this.headerValue, > + messageIdNodes = headerValue.childNodes, > + showFullMessageIds = !this.showFullMessageIds, > + messageIds = this.mMessageIds; generally declare all variables separately, and preferably when first used (where that's possible) doesn't look like there is any benefit of these intermediary variable in this case @@ +228,5 @@ > + const headerValue = this.headerValue, > + messageIdNodes = headerValue.childNodes, > + numMessageIds = this.mMessageIds.length; > + > + while (messageIdNodes.length > numMessageIds * 2 - 1) add braces for all loops @@ +233,5 @@ > + headerValue.lastChild.remove(); > + > + this.toggleIcon.hidden = numMessageIds <= 1; > + > + for (let index = 0; index < numMessageIds; index++) { while we're here, make it i instead of index @@ +253,5 @@ > + } > + } > + } > + > + _updateMessageIdNode(aMessageIdNode, aIndex, aMessageId, aLastId) { trying to get away from the a (meaning arg) convention, so at least for small usages, remove that while converting @@ +565,4 @@ > customElements.define("mail-newsgroup", MozMailNewsgroup); > customElements.define("mail-newsgroups-headerfield", MozMailNewsgroupsHeaderfield); > customElements.define("mail-messageid", MozMailMessageid); > +customElements.define("mail-messageids-headerfield", MozMailMessageidsHeaderfiled); you can put this right after the MozMailMessageidsHeaderfiled class body. planning to move them all after their classes shortly
Assignee | ||
Comment 11•6 years ago
|
||
Thanks for the detailed review, everything should be fixed.
I created a new class called .emailToggleHeaderfield
for the toggle icon and used the new twisty-collapsed.svg
icon which I noticed has been replacing the previous image.
Let me know if the CSS styling and location are correct.
Reporter | ||
Comment 12•6 years ago
|
||
Comment on attachment 9049305 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9049305 [details] [diff] [review]: ----------------------------------------------------------------- Related css for this should live in messageHeader.css There is (as for much of our css) a shared messageHeader.css which contains platform independent styling. That get's imported by the per-platform specific messageHeader.css where you'd put anything that is differnt between platforms https://searchfox.org/comm-central/search?q=messageheader.css&case=false®exp=false&path=mail%2Ftheme
Assignee | ||
Comment 13•6 years ago
|
||
I moved the CSS in the proper shared file and created the style variation for MacOS.
I'm uploading this patch here so I can import it on my Mac and be sure the CSS works properly.
I don't have a Windows computer to test if the shared style works correctly there.
Assignee | ||
Comment 14•6 years ago
|
||
Tested on osx and applied a quick CSS fix.
Ready to be reviewed.
Comment 15•6 years ago
|
||
Is the jumping to the top of the twisty needed when expanded? Can't we leave it centred like the twisties in the trees?
Reporter | ||
Comment 16•6 years ago
|
||
Comment on attachment 9049359 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9049359 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good Tests still need to be adjusted. One is test-message-toolbar.js but there can be others too. From the obj-dir, run make mozmill-one SOLO_TEST=message-header/test-message-header.js You'll see SUMMARY-PASS | test-message-header.js::test_clicking_star_opens_inline_contact_editor SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_msg_id_context_menu EXCEPTION: anonNodes is null at: test-window-helpers.js line 915 _get_anon_element_by_id_and_query test-window-helpers.js:915 29 _get_anon_elementid test-window-helpers.js:935 33 test_msg_id_context_menu test-message-header.js:437 20 Runner.prototype.wrapper frame.jsm:556 7 Runner.prototype._runTestModule frame.jsm:626 14 Runner.prototype.runTestModule frame.jsm:665 8 Runner.prototype.runTestFile frame.jsm:525 8 runTestFile frame.jsm:677 10 Bridge.prototype._execFunction server.js:190 15 Bridge.prototype.execFunction server.js:195 21 Session.prototype.receive server.js:313 6 AsyncRead.prototype.onDataAvailable server.js:78 16 ... failure to find things the same way here: https://searchfox.org/comm-central/rev/8a64171478a67b80775e23c37992cdf1dbd99e54/mail/test/mozmill/message-header/test-message-header.js#437 mc.aid means anonymous id (anonid, which is what XBL use for internal content) You should be able to just change it to mc.eid to make it work ::: mail/base/content/mailWidgets.js @@ +178,5 @@ > + * mailnews.headers.showReferences pref to true in the mailnews/mailnews.js file. > + * @extends {MozXULElement} > + */ > +class MozMailMessageidsHeaderfield extends MozXULElement { > + connectedCallback() { probably good to call delayConnectedCallback() here note that connectedCallback() can be called more than once, so you need a if (this.hasChildNodes()) { return; } @@ +242,5 @@ > + } > + } > + > + _updateMessageIdNode(messageIdNode, index, messageId, lastId) { > + const showFullMessageIds = this.showFullMessageIds; you don't need this temp variable
Reporter | ||
Comment 17•6 years ago
|
||
Comment on attachment 9049359 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9049359 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +200,5 @@ > + > + _toggleWrap() { > + for (let i = 0; i < this.headerValue.childNodes.length; i += 2) { > + if (!this.showFullMessageIds) { > + this.toggleIcon.setAttribute("open", "true"); forgot to add, it would be nice to while we touch this do the more webby thing and just add it as a class. this.toggleIcon.classList.add("open") ::: mail/themes/shared/mail/messageHeader.css @@ +273,5 @@ > +.emailToggleHeaderfield:-moz-locale-dir(rtl) { > + background: url("chrome://global/skin/icons/twisty-collapsed-rtl.svg"); > +} > + > +.emailToggleHeaderfield[open] { ... and here then emailToggleHeaderfield.open {
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #15)
Is the jumping to the top of the twisty needed when expanded? Can't we leave it centred like the twisties in the trees?
Thanks for pointing that out.
Actually, the chevron should always keep its position no matter the amount of IDs visualized or how tall the container gets, as it's a bit weird and misleading to let a clickable item move around after an action.
I updated the CSS
to have the chevron always properly top aligned.
I'll upload a new patch soon.
Assignee | ||
Comment 19•6 years ago
|
||
I updated the CSS
and I'm taking care of the tests.
Since we're here, should we consider removing those old twisty images to the right of the message IDs?
Are those there for any specific reason I'm missing?
Comment 20•6 years ago
|
||
I think they are only to symbolize that a click let TB jump to this message. Not very obvious and maybe a tooltip saying "Go to this message" could be better.
Assignee | ||
Comment 21•6 years ago
|
||
I fixed the reported issues and updated the CSS
to remove that little arrow pointing down.
A tooltip with the message ID appears if the link is collapsed and not visible.
I couldn't fix the tests as I'm still trying to troubleshoot why mozmill
doesn't work for me, either on macOS or Ubuntu.
I've been trying to troubleshoot it with the help of others on the IRC chat, hopefully I can figure it out soon, but at least I wanted to upload the updated patch.
Assignee | ||
Comment 22•6 years ago
|
||
I'm not sure why, but it looks like my tests are passing:
INFO Passed: 14
INFO Failed: 0
INFO Skipped: 1
The test_clicking_star_opens_inline_contact_editor()
passes for me.
...
SUMMARY-PASS | test-message-header.js::test_more_button_with_many_recipients
SUMMARY-PASS | test-message-header.js::test_clicking_star_opens_inline_contact_editor
SUMMARY-PASS | test-message-header.js::test_msg_id_context_menu
SUMMARY-PASS | test-message-header.js::test_address_book_switch_disabled_on_contact_in_mailing_list
...
Assignee | ||
Comment 23•6 years ago
|
||
I pulled all the resent updates and tried to run the test without changing anything on my patch, and once again, no error showed up. I wonder if this could be related of the mozmill
Bug 1533922 on MacOS affecting the outcome of the test.
I'll try to run some tests on Linux and see if it changes something.
Assignee | ||
Comment 24•6 years ago
|
||
Ok, I can confirm that the test was properly failing on Linux, but passing on MacOS. I'll keep using Linux for future tests until that bug is solved and part of comm-central.
mc.aid means anonymous id (anonid, which is what XBL use for internal content)
You should be able to just change it to mc.eid to make it work
That was right, but the test was still failing because I was wrapping the messages around a label
tag (silly me).
Other than being completely wrong semantically, it seems that it prevents the eid
helper method from properly looping through children elements if it encounters a label first.
Anyway, I changed the message wrapper into a regular div
and now everything works.
Reporter | ||
Comment 25•6 years ago
|
||
Comment on attachment 9050147 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9050147 [details] [diff] [review]: ----------------------------------------------------------------- Almost there I think. Please provide a proper commit message, like Bug 1531901 - convert the mail-messageids-headerfield binding to custom element. r?mkmelin ::: mail/base/content/mailWidgets.js @@ +174,5 @@ > +/** > + * MozMailMessageidsHeaderfield is a widget used to show/link messages in the message header. > + * Shown by default for nntp messages, not for regular emails. > + * Make it visible for all the emails in case of Reply or Fwd by settting the > + * mailnews.headers.showReferences pref to true in the mailnews/mailnews.js file. wouldn't discuss how to set it, and people would usually do that through the config editor (found under preferences | advanced) @@ +181,5 @@ > +class MozMailMessageidsHeaderfield extends MozXULElement { > + connectedCallback() { > + if (this.hasChildNodes()) { > + return; > + } I think this should also do a if (this.delayConnectedCallback()) { return; } That should be used as general practice unless we know the element won't have children or if the connectedCallback doesn't do anything with children.
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #25)
I think this should also do a
if (this.delayConnectedCallback()) {
return;
}
Would it be OK to write these two conditions together? Or you prefer to avoid it and leave them separate?
if (this.delayConnectedCallback() || this.hasChildNodes()) {
return;
}
Assignee | ||
Comment 27•6 years ago
|
||
Updated the patch with the commit message and the extra condition.
I want to also point out that I slightly updated the CSS to respect the width of the message window and let the message IDs properly stack when opened.
This was necessary after removing the hbox
container.
+++ b/mail/themes/linux/mail/messageHeader.css
@@ -249,6 +249,7 @@
margin-inline-start: 3px !important;
border: none !important;
background-color: transparent;
+ display: block;
}
.headerValueUrl {
@@ -277,6 +278,7 @@
text-decoration: underline;
margin: 0;
background-color: transparent;
+ display: inline;
}
Reporter | ||
Comment 28•6 years ago
|
||
Comment on attachment 9050836 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9050836 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +193,5 @@ > + this._toggleWrap(); > + }); > + this.appendChild(this.toggleIcon); > + > + this.headerValue = document.createElement("div"); sorry I didn't notice this before, but this is wrong. what you create here is a <xul:div> element (document.createElement uses the document namespace to create the element). But <xul:div> is not a thing. What you'd use is probably an hbox (like in the xbl binding originally) ::: mail/themes/linux/mail/messageHeader.css @@ +249,4 @@ > margin-inline-start: 3px !important; > border: none !important; > background-color: transparent; > + display: block; these changes are now in the wrong file if they are needed. you need to put them in the shared messageHeader.css instead of the linux specific
Assignee | ||
Comment 29•6 years ago
|
||
Thanks for the review.
Regarding the hbox
, am I allowed to use it?
I had it before and you recommended to remove it. That one probably doesn't count because it was an extra unnecessary container.
I just want to confirm that, even if we're stripping away xbl
, we can still keep using xul
tags like hbox
or vbox
.
Reporter | ||
Comment 30•6 years ago
|
||
Yes hbox and other xul tags are fine. What I meant earlier was that you now have the custom element which is itself a container element. With xbl that wasn't the case so the content of the xbl had to have a container for all it's internal elements.
Reporter | ||
Comment 32•6 years ago
|
||
Comment on attachment 9051075 [details] [diff] [review] de-xbl-mail-messageids-headerfield.patch Review of attachment 9051075 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin Please send it off for a try run to see you didn't break any mozmill tests. Let's you test out your new hg privileges. Post the try run link here in the bug. If it looks good (no new failures), you add the checkin-needed keyword
Assignee | ||
Comment 33•6 years ago
|
||
Here's the link of my try push
There's one test failing, but if I'm reading the report correctly, it should be an old one.
Jorgk told me about the graph and suggested to always rebase before pushing.
Should I rebase and try again or is this good for checkin-needed?
Comment 35•6 years ago
|
||
Yes, that debug crash is an old one. I'll shout anyone a free lunch if they can fix it. Bug 1377692.
Comment 36•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e77ea756f198
convert the mail-messageids-headerfield binding to custom element. r=mkmelin
Comment 37•6 years ago
|
||
Alex, do you really want your chat name in the HG header?
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #37)
Alex, do you really want your chat name in the HG header?
Is that wrong? I put it there so it would be easier for you guys to contact me on IRC.
Better put the full name?
What's the recommended way?
Comment 39•6 years ago
|
||
The recommended way is the name of the patch author unless they want to remain anonymous. I landed it with
Alessandro Castellani <alessandro@thunderbird.net> see
https://hg.mozilla.org/comm-central/rev/e77ea756f198
If you don't like it, I won't do it again. Take a look at
https://hg.mozilla.org/mozilla-central/pushloghtml and
https://hg.mozilla.org/comm-central/pushloghtml
Oh, one "aleca" already slipped through.
Assignee | ||
Comment 40•6 years ago
|
||
I have nothing against it, and thanks for updating it.
I will update my .hgrc
to follow the recommended approach.
Cheers
Updated•5 years ago
|
Description
•