[de-xbl] convert the conversation (imconversation) binding to a custom element <chat-conversation>
Categories
(Thunderbird :: Instant Messaging, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 9 obsolete files)
125.49 KB,
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
Can you take this? It's causing bug 1556203.
Assignee | ||
Comment 2•6 years ago
|
||
Sure, I'll take care of it
Assignee | ||
Comment 3•6 years ago
|
||
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.
Reporter | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
I found the problem with the textarea.
I was generating it with document.createXULElement("html:textare")
instead of using document.createElementNS
.
Silly me.
Reporter | ||
Comment 6•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Fixing some indentations, sorry for the multiple notifications.
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
Sounds good to me, I'm waiting for Magnus's review anyway that will probably happen later next week.
Comment 12•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 14•6 years ago
|
||
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•6 years ago
|
||
Don't worry, bug 1556203 will be done soon. Then you'll need to rebase your patch.
Comment 16•6 years ago
|
||
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•6 years ago
•
|
||
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.
Assignee | ||
Comment 19•6 years ago
|
||
Thanks Jorg for rebasing this.
I'll check the changes and finish this up if needed.
Comment 20•6 years ago
•
|
||
It will need more rebasing after bug 1559789, very minor though.
EDIT: Nope, no rebasing required.
Reporter | ||
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
Done, thanks for the feedback.
Reporter | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
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.
Assignee | ||
Comment 25•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 27•6 years ago
|
||
Yes, the spellcheck works as expected.
Assignee | ||
Updated•6 years ago
|
Comment 28•6 years ago
|
||
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
Updated•6 years ago
|
Description
•