1554633 - [de-xbl] convert the conversation (imconversation) binding to a custom element <chat-conversation>

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/mail/components/im/content/imconversation.xml#18

If I'm not mistaken, the only place this is created is https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/mail/components/im/content/chat-messenger.js#679

Comment 1

[Magnus Melin [:mkmelin]]

Can you take this? It's causing bug 1556203.

Comment 2

[Alessandro Castellani (:aleca)]

Sure, I'll take care of it

Comment 3

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

I need some help with this one.

The main issue is that the textarea to write the message is not visible and I don't understand why. it's there, not disabled, and not hidden, but it's unusable and it shows as a grey box.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9070031 [details] [diff] [review]
dexbl-imconversation.patch

Review of attachment 9070031 [details] [diff] [review]:
-----------------------------------------------------------------

In the native console I get 
JavaScript error: chrome://messenger/content/chat/chat-messenger.js, line 693: TypeError: item.convView.updateConvStatus is not a function
JavaScript error: chrome://messenger/content/chat/chat-messenger.js, line 400: TypeError: panel.onConvResize is not a function

I suspect that's why nothing works.

::: mail/components/im/content/imConversation.js
@@ +6,5 @@
> +
> +/* global MozElements MozXULElement */
> +/* globals chatHandler, Status */
> +
> +var { Services } = ChromeUtils.import("resource:///modules/imServices.jsm");

we've mostly used these without spaces in mail, like {Services} =

@@ +136,5 @@
> +    this._pendingValueChangedCall = false;
> +    this._nickEscape = /[[\]{}()*+?.\\^$|]/g;
> +    this._currentTypingName = "";
> +
> +    /* This value represents the difference between the deck's height and the

please use // style comments for inline code comments

@@ +1546,5 @@
> +    return this._bundle;
> +  }
> +}
> +
> +customElements.define("imconversation", MozIMConversation);

Please change it to chat-conversation and change name accordingly
(It should be something with dash in it, it's a hack that it works for non-dashed too and that may go away at some point since it's against the standard)

Comment 5

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

I found the problem with the textarea.
I was generating it with document.createXULElement("html:textare") instead of using document.createElementNS.
Silly me.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9070743 [details] [diff] [review]
dexbl-imconversation.patch

Review of attachment 9070743 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to work now!

Please put the MozChatConversation into the "leaking to global scope" protection.

Can you hg mv --after imconversattion.xul to the new chat-conversation.js

And change the name to chat-conversation.js since that's the CE name.

::: chat/content/conversation-browser.js
@@ +6,5 @@
>  
>  /* global MozXULElement */
> +{
> +  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +  var {

it's true some of these would ideally have this protection from being seen globally, but it's hard to see what changes was made here when you do the indent in once. If it's not significant changes, I'a do it in two steps

::: mail/components/im/content/chatConversation.js
@@ +170,5 @@
> +      "tooltip=contenttooltip,contextmenu=contentcontextmenu,autoscrollpopup");
> +
> +    this.progressBar = document.createElementNS("http://www.w3.org/1999/xhtml",
> +      "progress");
> +    this.progressBar.setAttribute("hidden", "true");

.setAttribute("hidden", "hidden");

@@ +202,5 @@
> +
> +    this.inputBox = document.createElementNS("http://www.w3.org/1999/xhtml",
> +      "textarea");
> +    this.inputBox.setAttribute("flex", "1");
> +    this.inputBox.setAttribute("multiline", "true");

I don't think neither flex nor multiline does anything for a textarea.

Comment 7

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

Comment 8

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

Comment 9

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

Fixing some indentations, sorry for the multiple notifications.

Comment 10

[Jorg K (CEST = GMT+2)]

Can you wait with this until I can fix bug 1556203. If this goes ahead first, I'll have a bad time porting my patch back to beta.

Comment 11

[Alessandro Castellani (:aleca)]

Sounds good to me, I'm waiting for Magnus's review anyway that will probably happen later next week.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: dexbl-imconversation.patch - with rename of XML to JS

Typically we do the de-XBL by renaming the XML to a JS file. I don't know whether it makes sense in this case.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #10)

Can you wait with this until I can fix bug 1556203. If this goes ahead first, I'll have a bad time porting my patch back to beta.

Another thought would be to do this bug first and then steal a lot of code from
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/textbox.js
for the context/spelling menu once the conversation is a CE.

But then I really don't know how to fix the context/spelling menu in TB 68 :-( - Note that TB 68 was returned to XBL code in bug 1555136 comment #23 and above.

Comment 14

[Alessandro Castellani (:aleca)]

We could potentially land this and create a couple of follow up bugs to take care of the context/spelling and the notifications.
I'll let Magnus chime in to define a good action plan.

Comment 15

[Jorg K (CEST = GMT+2)]

Don't worry, bug 1556203 will be done soon. Then you'll need to rebase your patch.

Comment 16

[Jorg K (CEST = GMT+2)]

You need to rebase your patch now. Basically what I added to imconversation.xml now needs to go into chat-conversation.js:
https://hg.mozilla.org/comm-central/rev/f72f5ad61b91#l4.2

Magnus, do you prefer to rename imconversation.xml to chat-conversation.js or delete the former and add the latter?

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: dexbl-imconversation.patch

I've rebased it for you based on the "rename" patch.

EDIT: Here's the interdiff, just the spellcheck bits in imconversation.xml:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9072135&action=interdiff&newid=9072474&headers=1
It seems to work even without the getter for spellchecker, no idea why. I'll leave it to the de-XBL crew to finish it.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: dexbl-imconversation.patch

Oops, this one.

Comment 19

[Alessandro Castellani (:aleca)]

Thanks Jorg for rebasing this.
I'll check the changes and finish this up if needed.

Comment 20

[Jorg K (CEST = GMT+2)]

It will need more rebasing after bug 1559789, very minor though.
EDIT: Nope, no rebasing required.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9072475 [details] [diff] [review]
dexbl-imconversation.patch

Review of attachment 9072475 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to work alright

::: mail/components/im/content/chat-conversation.js
@@ +6,3 @@
>  
> +/* global MozElements MozXULElement */
> +/* globals chatHandler, Status */

I think it could be better to add

var { Status } = ChromeUtils.import("resource:///modules/imStatusUtils.jsm");

@@ +9,2 @@
>  
> +(function() {

you don't need to use an IIFE. Just the braces will do to prevent leaking everything to global scope

@@ +303,4 @@
>  
> +    displayStatusText() {
> +      this.convStatus.value = this._statusText;
> +      if (this._statusText.length) {

if (this._statusText) {

@@ +401,5 @@
> +        // "/say" or "/say " should be ignored, as should "/" and "/ ".
> +        if (aMsg.match(/^\/(?:say)? ?$/)) {
> +          this.resetInput();
> +          return;
> +        } else if (aMsg.match(/^\/(?:say)? .*/)) {

no else after return please

@@ +1058,4 @@
>  
> +    // Replace the current selection in the inputBox by the given string
> +    addString(aString) {
> +      let length = (aString != "") ? aString.length : 0;

höh :p

@@ +1185,5 @@
> +        let middle = start + Math.floor((end - start) / 2);
> +        // .firstChild.nextSibling gets us to the label. We can't use
> +        // .label since the XBL binding might not be attached yet.
> +        if (nick < nicklist.getItemAtIndex(middle).firstChild.nextSibling
> +                          .getAttribute("value").toLowerCase()) {

(this was broken a few times in the last months.)
I'm pretty sure the comment would be now incorrect since we have no xbl here anymore

@@ +1200,5 @@
> +        nicklist.insertBefore(aListItem, nicklist.getItemAtIndex(end));
> +      }
> +    }
> +
> +    // Update a buddy in the visible list of participants

please use /** */ jsdoc comments for documentation

@@ +1333,2 @@
>  
> +      var topic = this._conv.topic;

please change to let

@@ +1395,5 @@
> +      // Start a timer to detect if the tab has been visible to the
> +      // user for long enough to actually be seen (as opposed to the
> +      // tab only being visible "accidentally in passing").
> +      delete this._wasVisible;
> +      this._visibleTimer = setTimeout(function() {

please use fat arrow function so you don't need to bind this. 

setTimeout(() => {

@@ +1466,5 @@
> +    showParticipants() {
> +      if (this._conv.isChat) {
> +        let nicklist = document.getElementById("nicklist");
> +        while (nicklist.hasChildNodes())
> +          nicklist.lastChild.remove();

please always braces for loops

@@ +1514,5 @@
> +    //   return this.spellchecker;
> +    // }
> +
> +    get _isConversationSelected() {
> +      // TB-only: returns true if the conversation binding is the currently

chat-conversation element

Comment 22

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

Done, thanks for the feedback.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9072847 [details] [diff] [review]
dexbl-imconversation.patch

Review of attachment 9072847 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/content/chat-conversation.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +/* global MozElements MozXULElement */
> +/* globals chatHandler */

global or globals. maybe the latter. I think with commas between is the one eslint uses in examples. so

/* globals MozElements, MozXULElement */

@@ +262,5 @@
> +
> +      let newNotificationBox = new MozElements.NotificationBox(element => {
> +        element.setAttribute("flex", "1");
> +        element.setAttribute("notificationside", "top");
> +        document.getElementById("conv-notification").append(element);

seems unfortunate to use an id for the notification that is part of this CE. Elements are supposed to be reusable and you could have more than one in the document...

Maybe just set a unique class for it, and this.querySelector to get it here

@@ +559,5 @@
> +      let text = this.inputBox.value;
> +
> +      const navKeyCodes = [KeyEvent.DOM_VK_PAGE_UP, KeyEvent.DOM_VK_PAGE_DOWN,
> +      KeyEvent.DOM_VK_HOME, KeyEvent.DOM_VK_END,
> +      KeyEvent.DOM_VK_UP, KeyEvent.DOM_VK_DOWN];

please fix this indention

@@ +1112,2 @@
>  
> +    // compute color for a nick

jsdoc style please

@@ +1139,5 @@
> +    _isBuddyActive(aBuddyName) {
> +      return Object.prototype.hasOwnProperty.call(this._activeBuddies, aBuddyName);
> +    }
> +
> +    // Create a buddy item to add in the visible list of participants

jsdoc style please

Comment 24

[Alessandro Castellani (:aleca)]

Attachment: dexbl-imconversation.patch

I updated all the comments above methods to follow the jsdoc format.

For the notification, since I was generating the conv-notification element in the connectedCallback, I declared that hbox as a property of the class. So now the NotificationBox looks like this:

let newNotificationBox = new MozElements.NotificationBox(element => {
        element.setAttribute("flex", "1");
        element.setAttribute("notificationside", "top");
        this.notification.append(element);
      });

Doing a try-push now.

Comment 25

[Alessandro Castellani (:aleca)]

Here's the try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=252667019&revision=06d49e2b032e3ae339c7615a1710b4268e3f64a6

I got a Z1 bug related to the attachment tests, which is odd since those this patch doesn't touch the attachment bindings, and I don't experience that failure locally.

Jorgk, do you think this is good for a checkin-needed?

Comment 26

[Jorg K (CEST = GMT+2)]

That Z1 is intermittent. It's good for check-in if the spellcheck still works in the conversation where you type. It worked after me rebasing it, but you might as well check it as well.

Comment 27

[Alessandro Castellani (:aleca)]

Yes, the spellcheck works as expected.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3252789dc296
[de-xbl] convert the conversation binding to a custom element. r=mkmelin