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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: mail-newsgroup.patch

Comment 2

[Arshad Khan [:arshad]]

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?

Comment 3

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield.patch

Comment 4

[Magnus Melin [:mkmelin]]

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

[Magnus Melin [:mkmelin]]

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

Comment 6

[Arshad Khan [:arshad]]

This bug has similar namespace issue as mail-emailaddress bug 1491698..

Comment 7

[Arshad Khan [:arshad]]

Attachment: exp_newsgroup.patch

Comment 8

[Arshad Khan [:arshad]]

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..

Comment 9

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups.patch

Comment 10

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups.patch

Comment 11

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups.patch

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..

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[Magnus Melin [:mkmelin]]

eh, if (!this.isConnected) of course

Comment 15

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield.patch

Comment 16

[Magnus Melin [:mkmelin]]

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.

Comment 17

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield.patch

Comment 18

[Arshad Khan [:arshad]]

(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?

Comment 19

[Brian Grinstead [:bgrins]]

(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.

Comment 20

[Arshad Khan [:arshad]]

(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..

Comment 21

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield.patch

Comment 22

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3552b794492f776c407b4e2e80e5b56f427c4ae&selectedJob=203325974

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: mail-newsgroups-headerfield.patch (simplified, missing some css still)

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 24

[Arshad Khan [:arshad]]

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 25

[Arshad Khan [:arshad]]

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 26

[Arshad Khan [:arshad]]

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 27

[Magnus Melin [:mkmelin]]

Yes, of course ;) All seemed to work fine and no console noise either.

Comment 28

[Magnus Melin [:mkmelin]]

Attachment: mail-newsgroups-headerfield.patch (unbitrotted, simplified, missing some css still)

unbitrotted

Comment 29

[Arshad Khan [:arshad]]

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

Comment 30

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield(with_correct_css).patch

Comment 31

[Arshad Khan [:arshad]]

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..

Comment 32

[Arshad Khan [:arshad]]

mail-newsgroups-headerfield(with_correct_css).patch - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=de841b15c9efc2e9aecce09488f8df624633df05

Comment 33

[Magnus Melin [:mkmelin]]

Attachment: 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.

Comment 34

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield_1.patch

Hopefully, this patch will be better.. Have you tested the ui on windows? I can confirm on mac, it is very similar.

Comment 35

[Magnus Melin [:mkmelin]]

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.

Comment 36

[Arshad Khan [:arshad]]

Attachment: mail-newsgroups-headerfield_2.patch

rebased.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0158dffba3a8cd0949374f01333d54606dd33f8c

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/849aa9b1b90c
Convert mail-newsgroup and mail-newsgroups-headerfield bindings to custom element. r=mkmelin

Comment 38

[Jorg K (CEST = GMT+2)]

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.

Comment 39

[Jorg K (CEST = GMT+2)]

This needs to come out too since it's based on bug 1491698. Please rebase when the other bug is ready.

Comment 40

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/73a9936d64609bd700e5218d0b86dcb8d201d138

Comment 41

[Arshad Khan [:arshad]]

ohk.

Comment 42

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b271a3b39dabaafbb14e3bda9079760466287c79

Comment 43

[Arshad Khan [:arshad]]

jorg, can we push this before mail-emailaddress? mail-emailaddress patch is blocked by another bug.

Comment 44

[Jorg K (CEST = GMT+2)]

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.

Comment 45

[Arshad Khan [:arshad]]

dont push it now, try run is still running

Comment 46

[Jorg K (CEST = GMT+2)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6cbc6a0b807b
Convert mail-newsgroup and mail-newsgroups-headerfield bindings to custom element. r=mkmelin