[de-xbl] Convert mail-newsgroup and mail-newsgroups-headerfield into custom element

RESOLVED FIXED in Thunderbird 64.0

Status

defect
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 13 obsolete attachments)

30.79 KB, image/png
Details
12.52 KB, patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Posted patch mail-newsgroup.patch (obsolete) — Splinter Review
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)
Summary: [de-xbl] Convert mail-newsgroup into custom element → [de-xbl] Convert mail-newsgroup and mail-newsgroups-headerfield into custom element
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 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-
This bug has similar namespace issue as mail-emailaddress bug 1491698..
Posted patch exp_newsgroup.patch (obsolete) — Splinter Review
Attachment #9009476 - Attachment is obsolete: true
Attachment #9009537 - Attachment is obsolete: true
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..
Status: NEW → ASSIGNED
Posted patch mail-newsgroups.patch (obsolete) — Splinter Review
Attachment #9012109 - Attachment is obsolete: true
Attachment #9012338 - Flags: review?(mkmelin+mozilla)
Posted patch mail-newsgroups.patch (obsolete) — Splinter Review
Attachment #9012338 - Attachment is obsolete: true
Attachment #9012338 - Flags: review?(mkmelin+mozilla)
Attachment #9012339 - Flags: feedback?(mkmelin+mozilla)
Posted patch mail-newsgroups.patch (obsolete) — Splinter Review
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 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+
Attachment #9012341 - Flags: review?(mkmelin+mozilla)
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-
eh, if (!this.isConnected) of course
Attachment #9012341 - Attachment is obsolete: true
Attachment #9013302 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9013302 - Attachment is obsolete: true
(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)
(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.
(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..
Attachment #9014163 - Attachment is obsolete: true
Attachment #9014285 - Flags: review?(mkmelin+mozilla)
Flags: needinfo?(mkmelin+mozilla)
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.
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...
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?
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..?
Yes, of course ;) All seemed to work fine and no console noise either.
unbitrotted
Attachment #9014285 - Attachment is obsolete: true
Attachment #9014348 - Attachment is obsolete: true
Attachment #9014285 - Flags: review?(mkmelin+mozilla)
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
Attachment #9014739 - Attachment is obsolete: true
Attachment #9015083 - Flags: review?(mkmelin+mozilla)
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..
Posted image ng-screenshot.png
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.
Attachment #9015083 - Flags: review?(mkmelin+mozilla)
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 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+
Attachment #9015352 - Flags: review+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
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 → ---
ohk.
Flags: needinfo?(arshdkhn1)
jorg, can we push this before mail-emailaddress? mail-emailaddress patch is blocked by another bug.
Flags: needinfo?(jorgk)
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
dont push it now, try run is still running
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 ;-)
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: 9 months ago9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.