Closed Bug 1554633 Opened 1 year ago Closed 1 year ago

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

Categories

(Thunderbird :: Instant Messaging, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 9 obsolete files)

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

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

Assignee: nobody → alessandro
Blocks: 1556203

Sure, I'll take care of it

Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

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.

Attachment #9070031 - Flags: feedback?(mkmelin+mozilla)
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)
Attachment #9070031 - Flags: feedback?(mkmelin+mozilla)
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

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

Attachment #9070031 - Attachment is obsolete: true
Attachment #9070743 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9070743 - Flags: review?(mkmelin+mozilla) → feedback+
Status: NEW → ASSIGNED
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review
Attachment #9070743 - Attachment is obsolete: true
Attachment #9071025 - Flags: review?(mkmelin+mozilla)
Attachment #9071025 - Flags: feedback+
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review
Attachment #9071025 - Attachment is obsolete: true
Attachment #9071025 - Flags: review?(mkmelin+mozilla)
Attachment #9071027 - Flags: review?(mkmelin+mozilla)
Attachment #9071027 - Flags: feedback+
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

Fixing some indentations, sorry for the multiple notifications.

Attachment #9071027 - Attachment is obsolete: true
Attachment #9071027 - Flags: review?(mkmelin+mozilla)
Attachment #9071028 - Flags: review?(mkmelin+mozilla)
Attachment #9071028 - Flags: feedback+

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.

No longer blocks: 1556203
Depends on: 1556203

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

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.

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

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.

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)

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?

Flags: needinfo?(mkmelin+mozilla)
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

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.

Attachment #9071028 - Attachment is obsolete: true
Attachment #9072135 - Attachment is obsolete: true
Attachment #9071028 - Flags: review?(mkmelin+mozilla)
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

Oops, this one.

Attachment #9072474 - Attachment is obsolete: true

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

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

Flags: needinfo?(mkmelin+mozilla)
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
Attachment #9072475 - Flags: feedback+
Attached patch dexbl-imconversation.patch (obsolete) — Splinter Review

Done, thanks for the feedback.

Attachment #9072475 - Attachment is obsolete: true
Attachment #9072847 - Flags: review?(mkmelin+mozilla)
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
Attachment #9072847 - Flags: review?(mkmelin+mozilla) → review+

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.

Attachment #9072847 - Attachment is obsolete: true
Attachment #9073050 - Flags: review+

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?

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)

Yes, the spellcheck works as expected.

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
Regressions: 1564443
No longer regressions: 1564443
Regressions: 1570679
Regressions: 1591699
You need to log in before you can comment on or make changes to this bug.