Closed
Bug 1491699
Opened 6 years ago
Closed 6 years ago
[de-xbl] Convert mail-newsgroup and mail-newsgroups-headerfield into custom element
Categories
(Thunderbird :: Mail Window Front End, task)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 64.0
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(2 files, 13 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
arshad
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Assignee | ||
Updated•6 years ago
|
Blocks: tb-war-on-xbl
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 9009476 [details] [diff] [review] mail-newsgroup.patch Hey, could please take a look at this and tell me whether it is working or not?
Attachment #9009476 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Summary: [de-xbl] Convert mail-newsgroup into custom element → [de-xbl] Convert mail-newsgroup and mail-newsgroups-headerfield into custom element
Assignee | ||
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Comment on attachment 9009537 [details] [diff] [review] mail-newsgroups-headerfield.patch Review of attachment 9009537 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +4,4 @@ > > /* global MozXULElement */ > > +const updateMailNewsAttributes = (node) => { I don't think the belongs here. It's just an internal helper. static _updateMailNewsAttributes(node) perhaps? @@ +29,5 @@ > + get newsgroups() { > + return this.querySelector(".headerValue"); > + } > + > + addNewsgroupView(aNewsgroup) { I don't think we're using the "a" convention for new code. Anyway, seems unused? And what type are these? Please document @@ +36,5 @@ > + > + buildViews() { > + for (let i = 0; i < this.mNewsgroups.length; i++) { > + let newNode = document.createElement("mail-newsgroup"); > + if (i) { i > 0 @@ +64,5 @@ > + } > + > + clearHeaderValues() { > + this.mNewsgroups = []; > + while (this.newsgroups.hasChildNodes()) this.newsgroups.lastChild.remove(); for loops, always put braces, and not on the same line @@ +101,5 @@ > > customElements.define("mail-newsgroup", MozMailNewsgroup); > +customElements.define( > + "mail-newsgroups-headerfield", > + MozMailNewsgroupsHeaderfield please put these on the same line Also, mail use 2-space indenting. calendar uses 4, but we should convince them to switch ;) (here and elsewhere in the patches)
Comment 5•6 years ago
|
||
Comment on attachment 9009476 [details] [diff] [review] mail-newsgroup.patch Review of attachment 9009476 [details] [diff] [review]: ----------------------------------------------------------------- Why don't you test it yourself? But no, it's not working. Set up a news account, use for testing on newsgroups.mozilla.org you can subscribe to netscape.public.mozilla.test JavaScript error: chrome://messenger/content/mailWidgets.xml, line 1020: NotSupportedError: Operation is not supported JavaScript error: chrome://messenger/content/mailWidgets.xml, line 1047: TypeError: contentElement is null; can't access its "setAttribute" property
Attachment #9009476 -
Flags: feedback?(mkmelin+mozilla) → feedback-
Assignee | ||
Comment 6•6 years ago
|
||
This bug has similar namespace issue as mail-emailaddress bug 1491698..
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9009476 -
Attachment is obsolete: true
Attachment #9009537 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 9012109 [details] [diff] [review] exp_newsgroup.patch Review of attachment 9012109 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.xml @@ +953,2 @@ > { > let newNode = document.createElement("mail-newsgroup"); > let newNode = document.createElement("mail-newsgroup"); This line throws NotSupportedError. I tried createXULElement and createElementNS(htmlns.. but all of them appears to be not working.. This issue/error can be seen in a lot of bugs where I try to create customElement by js.. In somecase the children of customelement are removed..
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #9012109 -
Attachment is obsolete: true
Attachment #9012338 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #9012338 -
Attachment is obsolete: true
Attachment #9012338 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9012339 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 11•6 years ago
|
||
magnus, could please provide look at the comment i added in updateAttributes and lemme know a better one if that's not ok.. and check out the patch as well..
Attachment #9012339 -
Attachment is obsolete: true
Attachment #9012339 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9012341 -
Flags: feedback?(mkmelin+mozilla)
Comment 12•6 years ago
|
||
Comment on attachment 9012341 [details] [diff] [review] mail-newsgroups.patch Review of attachment 9012341 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working find for me. I don't see any errors
Attachment #9012341 -
Flags: feedback?(mkmelin+mozilla) → feedback+
Assignee | ||
Updated•6 years ago
|
Attachment #9012341 -
Flags: review?(mkmelin+mozilla)
Comment 13•6 years ago
|
||
Comment on attachment 9012341 [details] [diff] [review] mail-newsgroups.patch Review of attachment 9012341 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +3,4 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +/* global MozXULElement, global openUILink */ you don't need the second "global" here @@ +50,5 @@ > + > + updateAttributes() { > + const label = this.querySelector("label.newsgrouplabel"); > + // guarding the below code to get executed from the very first call from > + // attributeChangedCallback because children are not appended at that time use this instead: if (this.isConnected) { return; } @@ +54,5 @@ > + // attributeChangedCallback because children are not appended at that time > + if (label) { > + if (this.hasAttribute("crop")) { > + label.setAttribute("crop", this.getAttribute("crop")); > + } should account for the attribute removal case too @@ +79,5 @@ > + <hbox class="headerValueBox" flex="1"> > + <description class="headerValue" containsEmail="true" flex="1"></description> > + </hbox> > + `) > + ); let'sjust get rid of MozXULElement.parseXULToFragment for this simple case, like in bug 1489748
Attachment #9012341 -
Flags: review?(mkmelin+mozilla) → review-
Assignee | ||
Updated•6 years ago
|
Attachment #9013302 -
Flags: review?(mkmelin+mozilla)
Comment 16•6 years ago
|
||
Comment on attachment 9013302 [details] [diff] [review] mail-newsgroups-headerfield.patch Review of attachment 9013302 [details] [diff] [review]: ----------------------------------------------------------------- > let'sjust get rid of MozXULElement.parseXULToFragment for this simple case, like in bug 1489748 You didn't address this yet.
Attachment #9013302 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 18•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #16) > Comment on attachment 9013302 [details] [diff] [review] > mail-newsgroups-headerfield.patch > > Review of attachment 9013302 [details] [diff] [review]: > ----------------------------------------------------------------- > > > let'sjust get rid of MozXULElement.parseXULToFragment for this simple case, like in bug 1489748 > > You didn't address this yet. mail-newsgroup is not the simple case, it has a popup which appears to not work just when I remove the hbox and label.. i can investigate more into this but definitely this is not the simple case like mail-urlfield/mail-tagfield/mail-headerfield.. Should I work on removing the parseXulTofragament or leave it for this one?
Flags: needinfo?(mkmelin+mozilla)
Comment 19•6 years ago
|
||
(In reply to Arshad Khan [:arshad] from comment #18) > (In reply to Magnus Melin [:mkmelin] from comment #16) > > Comment on attachment 9013302 [details] [diff] [review] > > mail-newsgroups-headerfield.patch > > > > Review of attachment 9013302 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > > let'sjust get rid of MozXULElement.parseXULToFragment for this simple case, like in bug 1489748 > > > > You didn't address this yet. > > mail-newsgroup is not the simple case, it has a popup which appears to not > work just when I remove the hbox and label.. i can investigate more into > this but definitely this is not the simple case like > mail-urlfield/mail-tagfield/mail-headerfield.. Should I work on removing the > parseXulTofragament or leave it for this one? I haven't looked at the markup but just FYI: parseXulTofragament was added to work around issues where XBL bindings don't get immediately attached to DOM nodes that were created with createElement even after they are appended to the document: https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/toolkit/content/customElements.js#25-35. So in general I've preferred to use it if the content contains elements that have XBL bindings attached. For a case where the content doesn't use XBL (i.e. an hbox and plain label without [control]), it shouldn't be necessary.
Assignee | ||
Comment 20•6 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #19) > (In reply to Arshad Khan [:arshad] from comment #18) > > (In reply to Magnus Melin [:mkmelin] from comment #16) > > > Comment on attachment 9013302 [details] [diff] [review] > > > mail-newsgroups-headerfield.patch > > > > > > Review of attachment 9013302 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > > let'sjust get rid of MozXULElement.parseXULToFragment for this simple case, like in bug 1489748 > > > > > > You didn't address this yet. > > > > mail-newsgroup is not the simple case, it has a popup which appears to not > > work just when I remove the hbox and label.. i can investigate more into > > this but definitely this is not the simple case like > > mail-urlfield/mail-tagfield/mail-headerfield.. Should I work on removing the > > parseXulTofragament or leave it for this one? > > I haven't looked at the markup but just FYI: parseXulTofragament was added > to work around issues where XBL bindings don't get immediately attached to > DOM nodes that were created with createElement even after they are appended > to the document: > https://searchfox.org/mozilla-central/rev/ > a11c128b90ea85d7bb68f00c5de52c0794e0b215/toolkit/content/customElements. > js#25-35. So in general I've preferred to use it if the content contains > elements that have XBL bindings attached. For a case where the content > doesn't use XBL (i.e. an hbox and plain label without [control]), it > shouldn't be necessary. Ok, makes sense. I was a little lost. I can drop parseXULToFragemnt here easily. I can just create xul elements via js and append them..
Assignee | ||
Updated•6 years ago
|
Attachment #9014285 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 22•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3552b794492f776c407b4e2e80e5b56f427c4ae&selectedJob=203325974
Comment 23•6 years ago
|
||
I think we can simplify it down to this. There's some margin/padding which needs to be adjusted yet, but other than that it seems to be working fine. I'll let you find out which css rule needs to be adjusted.
Assignee | ||
Comment 24•6 years ago
|
||
Comment on attachment 9014348 [details] [diff] [review] mail-newsgroups-headerfield.patch (simplified, missing some css still) Review of attachment 9014348 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ -252,4 @@ > <menuseparator id="subscribeToNewsgroupSeparator"/> > <menuitem id="subscribeToNewsgroupItem" label="&SubscribeToNewsgroup.label;" > accesskey="&SubscribeToNewsgroup.accesskey;" > - oncommand="SubscribeToNewsgroup(findEmailNodeFromPopupNode(document.popupNode, 'newsgroupPopup'))"/> aah, this was what I was missing when I tried simplyfing it...
Assignee | ||
Comment 25•6 years ago
|
||
Comment on attachment 9014348 [details] [diff] [review] mail-newsgroups-headerfield.patch (simplified, missing some css still) Review of attachment 9014348 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ +235,5 @@ > <tooltip id="attachmentListTooltip"/> > > <menupopup id="newsgroupPopup" position="after_start" class="newsgroupPopup" > + onpopupshowing="setupNewsgroupPopup(document.popupNode); goUpdateCommand('cmd_createFilterFromPopup')" > + onpopuphiding="hideEmailNewsPopup(document.popupNode);"> I am doubtful about the places where document.popupNode is used.. have you checked console, it might throw error?
Assignee | ||
Comment 26•6 years ago
|
||
Comment on attachment 9014348 [details] [diff] [review] mail-newsgroups-headerfield.patch (simplified, missing some css still) Review of attachment 9014348 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/messenger.xul @@ +241,5 @@ > disabled="true"/> > <menuseparator/> > <menuitem id="sendMessageToNewsgroupItem" label="&SendMessageTo.label;" > accesskey="&SendMessageTo.accesskey;" > + oncommand="SendMailToNode(document.popupNode, event)"/> Have you tested all these menuitem by clicking on them..?
Comment 28•6 years ago
|
||
unbitrotted
Attachment #9014285 -
Attachment is obsolete: true
Attachment #9014348 -
Attachment is obsolete: true
Attachment #9014285 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 29•6 years ago
|
||
Comment on attachment 9014739 [details] [diff] [review] mail-newsgroups-headerfield.patch (unbitrotted, simplified, missing some css still) Review of attachment 9014739 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +119,5 @@ > customElements.define("mail-urlfield", MozMailUrlfield); > customElements.define("mail-tagfield", MozMailHeaderfieldTags); > +customElements.define("mail-newsgroup", MozMailNewsgroup); > +customElements.define("mail-newsgroups-headerfield", MozMailNewsgroupsHeaderfield); > + extra line
Assignee | ||
Comment 30•6 years ago
|
||
Attachment #9014739 -
Attachment is obsolete: true
Attachment #9015083 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 31•6 years ago
|
||
Comment on attachment 9015083 [details] [diff] [review] mail-newsgroups-headerfield(with_correct_css).patch Review of attachment 9015083 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +74,5 @@ > } > > +class MozMailNewsgroup extends MozXULElement { > + connectedCallback() { > + this.classList.add("emailDisplayButton"); probably beccuase of the newsgrouplabel class, you mightbe seeing the less space horizontally between the mail-newsgroups. ::: mail/themes/shared/mail/messageHeader.css @@ +106,5 @@ > } > > +.headerValueBox { > + overflow: visible; > + padding: .1em 0; for the bottom margin/padding..
Assignee | ||
Comment 32•6 years ago
|
||
mail-newsgroups-headerfield(with_correct_css).patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=de841b15c9efc2e9aecce09488f8df624633df05
Comment 33•6 years ago
|
||
Almost there, but still some small discrepancy. Look at this comparison. Left is current situation, Right is with the patch. Notice the headers are not "at same distance" from each other. Only a few px off though.
Updated•6 years ago
|
Attachment #9015083 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 34•6 years ago
|
||
Hopefully, this patch will be better.. Have you tested the ui on windows? I can confirm on mac, it is very similar.
Attachment #9015083 -
Attachment is obsolete: true
Attachment #9015212 -
Flags: review?(mkmelin+mozilla)
Comment 35•6 years ago
|
||
Comment on attachment 9015212 [details] [diff] [review] mail-newsgroups-headerfield_1.patch Review of attachment 9015212 [details] [diff] [review]: ----------------------------------------------------------------- LGTM r=mkmelin ::: mail/themes/shared/mail/messageHeader.css @@ +110,5 @@ > +} > + > +mail-newsgroups-headerfield.headerValueBox { > + padding: .1em 0; > + margin-bottom: 4px; I'm not completely convinced this is the correct fix, but visually it seems ok. We can revisit later after all the fields are converted and see if there's cleanup needed.
Attachment #9015212 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 36•6 years ago
|
||
rebased. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0158dffba3a8cd0949374f01333d54606dd33f8c
Attachment #9015212 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9015352 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 37•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/849aa9b1b90c Convert mail-newsgroup and mail-newsgroups-headerfield bindings to custom element. r=mkmelin
Comment 38•6 years ago
|
||
The patch didn't apply since you were removing the last empty line in customElements.js, a change already made in bug 1491698: https://hg.mozilla.org/comm-central/rev/f6d8ba2898f9#l1.12 Also, can you please set up your mercurial.ini with [diff] git = 1 showfunc = 1 unified = 8 If I refresh any of your patches I get hundreds of differences to the original since I will add 8 lines of context when it only had 3 before.
Target Milestone: --- → Thunderbird 64.0
Comment 39•6 years ago
|
||
This needs to come out too since it's based on bug 1491698. Please rebase when the other bug is ready.
Status: RESOLVED → REOPENED
Flags: needinfo?(arshdkhn1)
Resolution: FIXED → ---
Target Milestone: Thunderbird 64.0 → ---
Comment 40•6 years ago
|
||
Backout: https://hg.mozilla.org/comm-central/rev/73a9936d64609bd700e5218d0b86dcb8d201d138
Assignee | ||
Comment 42•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b271a3b39dabaafbb14e3bda9079760466287c79
Assignee | ||
Comment 43•6 years ago
|
||
jorg, can we push this before mail-emailaddress? mail-emailaddress patch is blocked by another bug.
Flags: needinfo?(jorgk)
Comment 44•6 years ago
|
||
Yes, we can, seems to apply without bug 1491698. Last night bug 1491698 couldn't be backed out without backing this one out, too. But the patch applies and I'll land it again.
Flags: needinfo?(jorgk)
Keywords: checkin-needed
Comment 46•6 years ago
|
||
I know. But the previous try from comment #36 was successful. I land after M-C merges, mostly at the following times (UTC+2): Mornings, 10-11, afternoons, 18-20, midnight, Geoff lands at 6. So now it's not landing time ;-)
Comment 47•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/6cbc6a0b807b Convert mail-newsgroup and mail-newsgroups-headerfield bindings 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 64.0
Updated•5 years ago
|
Type: defect → task
You need to log in
before you can comment on or make changes to this bug.
Description
•