De-XBL conv-info-large
Categories
(Thunderbird :: Instant Messaging, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
(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
Comment 1•6 years ago
|
||
(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.
Reporter | ||
Comment 2•6 years ago
|
||
Yes please, could make it easier to see the actual work in this bug is correct.
Comment 3•6 years ago
|
||
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?
Reporter | ||
Comment 4•6 years ago
|
||
(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.
Assignee | ||
Comment 5•6 years ago
|
||
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.
Reporter | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
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"
Reporter | ||
Comment 8•6 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #7)
Should I file a separate bug for this?
Yes please.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 9•6 years ago
|
||
Resolve Bug 1523606 and Bug 1523419 before landing this patch.
Reporter | ||
Comment 10•6 years ago
|
||
Assignee | ||
Comment 11•6 years ago
|
||
Reporter | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
Loading element lazily didn't work. Need to look into it.
Reporter | ||
Comment 14•6 years ago
|
||
Assignee | ||
Comment 15•6 years ago
|
||
Do I need to add check-in required tag?
Assignee | ||
Comment 16•6 years ago
|
||
Reporter | ||
Comment 17•6 years ago
|
||
Assignee | ||
Comment 18•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 19•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/365cb705c6427af3bf219b0ced88492fed977d4f
De-XBL: convert conv-info-large to custom element. r=mkmelin
Updated•6 years ago
|
Comment 20•6 years ago
|
||
Reporter | ||
Comment 21•6 years ago
|
||
Ah yes, document.querySelector should be this.querySelector here
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 22•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 23•6 years ago
|
||
Reporter | ||
Comment 24•6 years ago
|
||
(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.
Assignee | ||
Comment 25•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Comment 26•6 years ago
|
||
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.
Comment 27•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eba05990178d
Follow-up: change document.querySelector to this.querySelector. r=mkmelin
Reporter | ||
Comment 28•6 years ago
|
||
You do that by setting this in ~/.hgrc
[ui]
username = First Last <email@example.com>
Comment 29•6 years ago
|
||
(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.
Updated•5 years ago
|
Description
•