Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Arshad Khan [:arshad] 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?

	Magnus Melin [:mkmelin]
	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)

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] Assignee
	Comment 6 • 6 years ago

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

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] 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..

	Magnus Melin [:mkmelin]
	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

	Magnus Melin [:mkmelin]
	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

	Magnus Melin [:mkmelin]
	Comment 14 • 6 years ago

eh, if (!this.isConnected) of course

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] 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?

	Brian Grinstead [:bgrins]
	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.

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 22 • 6 years ago

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

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] 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...

	Arshad Khan [:arshad] 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?

	Arshad Khan [:arshad] 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..?

	Magnus Melin [:mkmelin]
	Comment 27 • 6 years ago

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

	Magnus Melin [:mkmelin]
	Comment 28 • 6 years ago

unbitrotted

	Arshad Khan [:arshad] 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

	Arshad Khan [:arshad] 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..

	Arshad Khan [:arshad] Assignee
	Comment 32 • 6 years ago

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

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] 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.

	Magnus Melin [:mkmelin]
	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.

	Arshad Khan [:arshad] Assignee
	Comment 36 • 6 years ago

rebased.

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

	Pulsebot
	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

	Jorg K (CEST = GMT+2)
	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.

	Jorg K (CEST = GMT+2)
	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.

	Jorg K (CEST = GMT+2)
	Comment 40 • 6 years ago

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

	Arshad Khan [:arshad] Assignee
	Comment 41 • 6 years ago

ohk.

	Arshad Khan [:arshad] Assignee
	Comment 42 • 6 years ago

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

	Arshad Khan [:arshad] Assignee
	Comment 43 • 6 years ago

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

	Jorg K (CEST = GMT+2)
	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.

	Arshad Khan [:arshad] Assignee
	Comment 45 • 6 years ago

dont push it now, try run is still running

	Jorg K (CEST = GMT+2)
	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 ;-)

	Pulsebot
	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