Closed Bug 1521610 Opened 6 years ago Closed 6 years ago

De-XBL conv-info-large

Categories

(Thunderbird :: Instant Messaging, task)

Product:
Thunderbird
Component:
Instant Messaging
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

Details

Attachments

(2 files, 7 obsolete files)

[checked in] De_XBL_conv-info-large.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
changes_chat-conversation-info.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1484976 +++

conv-info-large looks like something we could de-xbl now.
It's used in the thunderbird chat tab - it's the box above "previous converations" on the top right.

https://searchfox.org/comm-central/search?q=conv-top-info&path=

It appears to me this doesn't need to be a toolbar, but can probably just be an hbox instead.

At least on trunk it looks broken (black background and such) for me on linux. Maybe due to some other core de-xbl... would have to check 60 to see what it looks like there.

Custom Elements can't QI to an observer, so instead of "this" being the observer, you need to use a helper object. See e.g. conversation-browser.js

(In reply to Magnus Melin [:mkmelin] from comment #0)

At least on trunk it looks broken (black background and such) for me on linux.

This would be a simple fix. I could do it until this bug is fixed.

Yes please, could make it easier to see the actual work in this bug is correct.

Was just talking to Khushil on IRC about this. It seems that the current box is broken anyway -- in a situation where you have the proper permissions to modify the topic you can click on it, but never actually type new information into the box. Not sure if this is what Magnus meant in comment 0 or not though?

This is broken in at least beta (65b3). Not sure if it is worth filing a separate bug or not though?

(The observer part of comment 0 was off, observer isn't used in this binding.)

If is seems like an easy fix we could include the modification fix while doing this conversion. Otherwise may want to do it in a separate bug.

Attached patch De-XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review

I haven't made any changes regarding the error we are talking about. I don't know what exactly needs to be done to solve that error.

Attachment #9039635 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9039635 [details] [diff] [review] De-XBL_conv-info-large.patch Review of attachment 9039635 [details] [diff] [review]: ----------------------------------------------------------------- Didn't try this yet, but can you first fix some style things: - for mail/ we use 2 space indention - break lines at 80 chars - please always put braces for ifs, even if it's just one line Please file the bug for the malfunctioning editing ::: mail/components/im/content/conv-top-info.js @@ +42,5 @@ > + } > + > + _updateAttributes() { > + let userIconHolder = document.querySelector(".userIconHolder"); > + this.hasAttribute("userIcon") ? userIconHolder.setAttribute("userIcon", this.getAttribute("userIcon")) : userIconHolder.removeAttribute("userIcon"); please use else-if for all of these but, you could just use the this.inheritAttribute method @@ +50,5 @@ > + > + let statusTypeIcon = document.querySelector(".statusTypeIcon"); > + this.hasAttribute("status") ? statusTypeIcon.setAttribute("status", this.getAttribute("status")) : statusTypeIcon.removeAttribute("status"); > + this.hasAttribute("typing") ? statusTypeIcon.setAttribute("typing", this.getAttribute("typing")) : statusTypeIcon.removeAttribute("typing"); > + this.hasAttribute("typed") ? statusTypeIcon.setAttribute("typed", this.getAttribute("typed")) : statusTypeIcon.removeAttribute("typed"); this should not really be two attributes I think, they are mutually exclusive but that could go into another bug @@ +76,5 @@ > + if (!this.hasAttribute("editing")) > + return; > + > + let convBinding = > + document.getElementById("conversationsDeck").selectedPanel; maybe call this "panel", not convBinding @@ +133,5 @@ > + } > +} > + > +customElements.define("conv-top-info", MozConvInfoLarge); > + remove both empty lines
Attachment #9039635 - Flags: review?(mkmelin+mozilla)

this should not really be two attributes I think, they are mutually exclusive
but that could go into another bug

Should I file a separate bug for this?

remove both empty lines

Lint is showing following error when removing the last line: "Newline required at end of file but not found"

(In reply to Khushil Mistry [:khushil324] from comment #7)

Should I file a separate bug for this?

Yes please.

Depends on: 1523419, 1523606
Attached patch De-XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review

Resolve Bug 1523606 and Bug 1523419 before landing this patch.

Attachment #9039635 - Attachment is obsolete: true
Attachment #9040417 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9040417 [details] [diff] [review] De-XBL_conv-info-large.patch Review of attachment 9040417 [details] [diff] [review]: ----------------------------------------------------------------- Needs some updates (may want to wait until bug 1525695 lands) Also do hg cp --after to preserve some blame ::: mail/base/content/messenger.xul @@ +146,4 @@ > <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/> > <script type="application/javascript" src="chrome://messenger/content/quickFilterBar.js"/> > <script type="application/javascript" src="chrome://messenger/content/newmailaccount/uriListener.js"/> > +<script type="application/javascript" src="chrome://messenger/content/chat/conv-top-info.js"/> you should put this into the mail customElements.js For chat elements like this we should probably also define them lazily using setElementCreationCallback like https://searchfox.org/comm-central/source/mozilla/toolkit/content/customElements.js#397 ::: mail/components/im/content/conv-top-info.js @@ +5,5 @@ > +"use strict"; > + > +/* global MozXULElement */ > + > +class MozConvInfoLarge extends MozXULElement { please add JsDoc documentation about the typical usage @@ +8,5 @@ > + > +class MozConvInfoLarge extends MozXULElement { > + > + static get observedAttributes() { > + return ["userIcon", "status", "typing", "typed", "statusTypeTooltiptext", typing and typed were folded @@ +29,5 @@ > + <image class="prplIcon" anonid="targetPrplIcon"></image> > + </hbox> > + <description anonid="statusMessage" class="statusMessage" mousethrough="never" crop="end" flex="100000"> > + </description> > + </stack> I think the anonids should go @@ +133,5 @@ > + elt.value = ""; > + elt.select(); > + } > +} > +customElements.define("conv-top-info", MozConvInfoLarge); Maybe chat-conversation-info?
Attachment #9040417 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch De_XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review
Attachment #9040417 - Attachment is obsolete: true
Attachment #9043367 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9043367 [details] [diff] [review] De_XBL_conv-info-large.patch Review of attachment 9043367 [details] [diff] [review]: ----------------------------------------------------------------- Seems to be working, but still needs some work. Please add a proper commit message ::: mail/base/content/customElements.js @@ +17,4 @@ > "chrome://messenger/content/generalBindings.js", > "chrome://messenger/content/statuspanel.js", > "chrome://messenger/content/foldersummary.js", > + "chrome://messenger/content/chat/chat-conversation-info.js" like I wrote earlier, please use setElementCreationCallback for this. Many windows may not need it. Also, missing tailing comma, which eslint would complain about ::: mail/components/im/content/chat-conversation-info.js @@ +6,5 @@ > +/* global MozXULElement */ > + > +/** > + * Chat convesation information that is displayed on top > + * of the chat window. Typo. Perhaps like this: /** * The MozChatConversationInfo widget displays information about a chat: * e.g. the channel name and topic of an IRC channel, or nick, user image and * status of a conversation partner. * It is typically shown at the top right of the chat UI. */ @@ +9,5 @@ > + * Chat convesation information that is displayed on top > + * of the chat window. > + * @extends MozXULElement > + */ > +class MozConvInfoLarge extends MozXULElement { rename to MozChatConversationInfo @@ +20,5 @@ > + connectedCallback() { > + this.appendChild(MozXULElement.parseXULToFragment(` > + <stack anonid="statusImageStack" class="statusImageStack"> > + <box class="userIconHolder"> > + <image anonid="userIcon" class="userIcon" mousethrough="always"></image> please remove the anonids @@ +36,5 @@ > + </stack> > + `)); > + this.topic.addEventListener("click", this.startEditTopic.bind(this)); > + // Cancel any ongoing edit if the binding changes. > + this.removeAttribute("editing"); huh? editing is broken, but having this in the connectedCallback whouldn't at least help @@ +77,5 @@ > + return document.querySelector(".statusMessage"); > + } > + > + finishEditTopic(aSave) { > + if (!this.hasAttribute("editing")) while we're here, as a style nit, add angle brackets even for one-line ifs @@ +100,5 @@ > + // that can't receive keyboard events, so move it to somewhere else. > + panel.editor.focus(); > + } > + > + topicKeyPress(aEvent) { and remove the a-convention. so just "event" @@ +129,5 @@ > + this._topicKeyPress = this.topicKeyPress.bind(this); > + elt.addEventListener("keypress", this._topicKeyPress); > + this._topicBlur = this.topicBlur.bind(this); > + elt.addEventListener("blur", this._topicBlur); > + // force binding attachmant by forcing layout this is related to xbl. You shouldn't need it anymore ::: mail/components/im/content/chat-messenger.inc.xul @@ +160,4 @@ > </deck> > <splitter id="contextSplitter" hidden="true" collapse="after"/> > <vbox id="contextPane" hidden="true" width="250" persist="width"> > + <chat-conversation-info class="chat-conversation-info" id="chat-conversation-info"/> I don't think you need the class for anything, and perhaps keep the original id of conv-top-info (although the name of the element changed). It's slightly confusing that they have the same name
Attachment #9043367 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch De_XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review

Loading element lazily didn't work. Need to look into it.

Attachment #9043367 - Attachment is obsolete: true
Attachment #9044160 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9044160 [details] [diff] [review] De_XBL_conv-info-large.patch Review of attachment 9044160 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin with the comment below fixed ::: mail/components/im/content/chat-conversation-info.js @@ +31,5 @@ > + <stack class="displayNameAndstatusMessageStack" mousethrough="always" flex="1"> > + <hbox align="center" flex="1"> > + <description anonid="displayName" class="displayName" flex="1" crop="end"> > + </description> > + <image class="prplIcon" anonid="targetPrplIcon"></image> please remove the anonids
Attachment #9044160 - Flags: review?(mkmelin+mozilla) → review+
Attached patch De_XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review

Do I need to add check-in required tag?

Attachment #9044160 - Attachment is obsolete: true
Attachment #9044299 - Flags: review+
Attached patch De_XBL_conv-info-large.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044299 - Attachment is obsolete: true
Attachment #9044300 - Flags: review+
Comment on attachment 9044300 [details] [diff] [review] De_XBL_conv-info-large.patch Review of attachment 9044300 [details] [diff] [review]: ----------------------------------------------------------------- There's the checkin-needed keyword ::: mail/components/im/content/chat-conversation-info.js @@ +23,5 @@ > + connectedCallback() { > + this.appendChild(MozXULElement.parseXULToFragment(` > + <stack anonid="statusImageStack" class="statusImageStack"> > + <box class="userIconHolder"> > + <image anonid="userIcon" class="userIcon" mousethrough="always"></image> still 2 anonids
Attachment #9044300 - Attachment is obsolete: true
Attachment #9044308 - Flags: review+
Keywords: checkin-needed

https://hg.mozilla.org/comm-central/rev/365cb705c6427af3bf219b0ced88492fed977d4f
De-XBL: convert conv-info-large to custom element. r=mkmelin

Keywords: checkin-needed
Target Milestone: --- → Thunderbird 67.0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 9044308 [details] [diff] [review] [checked in] De_XBL_conv-info-large.patch Review of attachment 9044308 [details] [diff] [review]: ----------------------------------------------------------------- I wish I looked at this earlier :-(. Sorry for the late comments. Unless I'm missing something, in Thunderbird this binding is used only once (this wasn't the case in Instantbird, and I guess this was a binding in Thunderbird too to make it easy to port patches done in one application to the other) so the right fix would be to include the XUL markup directly within chat-messenger.inc.xul and to put the JS code dealing with topic edits directly in chat-messenger.js. ::: mail/components/im/content/chat-conversation-info.js @@ +51,5 @@ > + _updateAttributes() { > + let userIconHolder = document.querySelector(".userIconHolder"); > + this.inheritAttribute(userIconHolder, "userIcon"); > + > + let userIcon = document.querySelector(".userIcon"); The very generic querySelector calls here for .userIcon .statusTypeIcon .displayName .prplIcon are scary. This code used to be contrained to a specific small XBL binding. Now you are searching across the whole document, and it's likely to match things with the same CSS classes in other parts of the UI (eg. in the list of contacts & conversations on the left side of the UI) either now or after de-XBL'ing other parts of the UI later.

Ah yes, document.querySelector should be this.querySelector here

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch changes_chat-conversation-info.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044308 - Attachment is obsolete: true
Attachment #9044509 - Flags: review?(mkmelin+mozilla)
Attachment #9044308 - Attachment description: De_XBL_conv-info-large.patch → [checked in] De_XBL_conv-info-large.patch
Attachment #9044308 - Attachment is obsolete: false
Attachment #9044509 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9044509 [details] [diff] [review] changes_chat-conversation-info.patch Review of attachment 9044509 [details] [diff] [review]: ----------------------------------------------------------------- Can you change the commit message to something someone without context could understand. Like Bug 1521610 - followup to change document.querySelector to this.querySelector

(In reply to Florian Quèze [:florian] from comment #20)

so the right fix would be to include the XUL markup directly
within chat-messenger.inc.xul and to put the JS code dealing with topic
edits directly in chat-messenger.js.

While it's certainly possible to do it like that too, the element does have a good amount of behaviour attached to it. I like CEs in the way that you get a natural place to put element specific behaviour. We have too much global functions touching specific items.

But we can certainly do a follow-up to change it if you prefer.

Attachment #9044509 - Attachment is obsolete: true
Attachment #9044515 - Flags: review+
Keywords: checkin-needed

Since we're already criticising commit messages: Please provide your e-mail address in the HG headers in the future. "Followup" is not a word, it's "follow-up". I'll fix both.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eba05990178d
Follow-up: change document.querySelector to this.querySelector. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

You do that by setting this in ~/.hgrc

[ui]
username = First Last <email@example.com>

(In reply to Magnus Melin [:mkmelin] from comment #24)

But we can certainly do a follow-up to change it if you prefer.

I don't care strongly. With document.querySelector changed to this.querySelector this is no longer a regression. Thanks for fixing that.

Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: