displayStatusText() is not implemented in Thunderbird

RESOLVED FIXED in Thunderbird 36.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: msot, Assigned: msot)

Tracking

unspecified
Thunderbird 36.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird36 fixed)

Details

Attachments

(3 attachments, 12 obsolete attachments)

33.35 KB, image/png
Details
18.47 KB, image/jpeg
Details
8.44 KB, patch
aleth
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
A lot of code in imconversation.xml (https://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml) attempts to call displayStatusText(), which does not exist in Thunderbird. Because of this, the output of a lot of that code is never shown to the user.

Either displayStatusText should be implemented (possibly by porting the code from instantbird at https://mxr.mozilla.org/comm-central/source/im/content/conversation.xml#1620) or that information should be displayed to the user in another way (like the character counter from https://bugzilla.mozilla.org/show_bug.cgi?id=736002).

Comment 1

5 years ago
(In reply to Matthew Sotoudeh [:msot] from comment #0)
> Either displayStatusText should be implemented (possibly by porting the code
> from instantbird at
> https://mxr.mozilla.org/comm-central/source/im/content/conversation.
> xml#1620) or that information should be displayed to the user in another way
> (like the character counter from
> https://bugzilla.mozilla.org/show_bug.cgi?id=736002).

The second option, while more work, would be preferable in the long run. We're planning to get rid of the status bar in Instantbird eventually.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: x86_64 → All
Assignee

Comment 2

5 years ago
Seems like the main usages of displayStatusText() left are:

(line numbers from https://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml)

- Line 555
  - This doesn't seem to be necessary any more after bug 736002, we should be able to just get rid of it.
- Line 579
  - It looks like this just resets the status, so if we're getting rid of the whole status text we should be able to get rid of this too.
- Line 104
  - This one just re-displays any status text that was there before the input lost focus, should be able to take this out as well.
- Line 1094
  - As far as I can tell this resets the status when the buddy is not connected (so it can be safely taken out)
- Line 1196
  - And this just seems to look for changes in the status text and update them. Can be removed as well.

That's all a quick ctrl-F found, it looks like we can just take out everything left that references displayStatusText?
Matthew, that looks reasonable. It looks to me like _statusTextEnd and _statusTextEndIsError might be able to be removed as well too?

The one question I have is where typing notifications are shown. I think this is unrelated, but my Thunderbird is currently busted...
Assignee

Comment 4

5 years ago
If I understand you correctly, typing notifications should be handled around line 533 (https://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#533).

As far as I can find it doesn't show any notification when someone else is typing (does it? I don't ever use it with any service that uses typing notifications)

Comment 5

5 years ago
That's the outgoing typing notifications. The incoming ones are handled here http://dxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#1033

XMPP supports typing notifications, if you are wondering what to test with ;)
Assignee

Comment 6

5 years ago
Posted patch Removed displayStatusText() (obsolete) — Splinter Review
Here's a quick removal of everything referencing statusText, I'll go through it & mxr tonight to see if there's anything major I missed (do you guys see anything?)

It looks like the typing notifications are handled separately (in the buddy image), so that shouldn't be a problem.

Thanks,
Matthew

Comment 7

5 years ago
Comment on attachment 8507464 [details] [diff] [review]
Removed displayStatusText()

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

::: mail/components/im/content/imconversation.xml
@@ +1068,2 @@
>                cti.removeAttribute("userIcon");
>              }

Nit: We generally do without the brackets around a single line of code.

@@ -1190,5 @@
>             if (this.loaded)
>               this.addMsg(aSubject);
>             break;
>  
> -         case "status-text-changed":

It's a good idea when removing the handler for a notification to check what is sending it. In this case, you find http://dxr.mozilla.org/comm-central/source/chat/protocols/twitter/twitter.js#156. This is used by twitter to remind the user when she is replying to a particular tweet.
Attachment #8507464 - Flags: review-
Assignee

Comment 8

5 years ago
Posted patch Removed displayStatusText() (obsolete) — Splinter Review
I've fixed the brackets, but not sure how we should deal with the replying to part. Any ideas? We could just put it in the main status bar, but would it be better to do that by fixing displayStatusText(), or just setting the status text directly with document.getElementById("statusText")?

Maybe we could put the replying-to text right above the textbox, or some other place. I've added richard.marti, since I *think* he's in charge of the UI for chat(?)

Thanks,
Matthew
Attachment #8507464 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Flags: needinfo?(aleth)

Comment 9

5 years ago
(In reply to Matthew Sotoudeh [:msot] from comment #8)
> I've fixed the brackets, but not sure how we should deal with the replying
> to part. Any ideas? We could just put it in the main status bar, but would
> it be better to do that by fixing displayStatusText(), or just setting the
> status text directly with document.getElementById("statusText")?

I dislike putting new things in the status bar (see comment 1). I would suggest doing essentially the same as you did for the character counter, but on the bottom left side of the input box (maybe using lighter gray colours so it is less obtrusive).

> Maybe we could put the replying-to text right above the textbox, or some
> other place. I've added richard.marti, since I *think* he's in charge of the
> UI for chat(?)

Paenglab is (along with the other TB UI peers) in charge of the UI for Thunderbird.
Flags: needinfo?(aleth)
I second aleth's view to not use the status bar because it can be hidden. For TB I've never seen this message but if it appears in the input box like the counter, like aleth suggested, it would be great.
Flags: needinfo?(richard.marti)
Just to show how the UI currently looks in Instantbird (so they kind of information we're trying to present to the user). (Note that we're also trying to move away from using the statusbar in Instantbird.)
Assignee

Comment 12

5 years ago
What about something like this, a status bar of sorts right above the input box? That way people would still see it when they disable the main status bar, and it would be clearer that it's related to what they're typing right then.

I can't think of many other ways to tell the user who they're replying to.
Assignee

Comment 13

5 years ago
Or we might be able to highlight somehow the tweet you're replying to in the message view and sort of fade out the other messages to emphasize it?

Comment 14

5 years ago
(In reply to Matthew Sotoudeh [:msot] from comment #12)
> What about something like this, a status bar of sorts right above the input
> box? That way people would still see it when they disable the main status
> bar, and it would be clearer that it's related to what they're typing right
> then.

That's a good suggestion!  When you implement it,
- add a nice CSS transition for showing/hiding
- make sure it doesn't cause layout issues when the window is very small or when it is resized. You might need to add a min-height in various places, and I think there are already media tags in the CSS.

> Or we might be able to highlight somehow the tweet you're replying to in the message view and sort of
> fade out the other messages to emphasize it?

This is almost impossible to do well across different message styles.

Updated

5 years ago
Flags: needinfo?(richard.marti)
(In reply to Matthew Sotoudeh [:msot] from comment #12)
> What about something like this, a status bar of sorts right above the input
> box? That way people would still see it when they disable the main status
> bar, and it would be clearer that it's related to what they're typing right
> then.

That looks good and makes sense with the reply directly below the referring text. And yes, a transition would also help to indicate this text and makes the display smoother.
Flags: needinfo?(richard.marti)
Assignee

Comment 16

5 years ago
Here's a quick stab at it, but I'm having trouble getting the new status bar/description element to slide in. It seems to have some non-overidable min-height: 15px; (or something like that) that won't let the height: 0; work. I've tried removing all margins, but .conv-status still won't start off with 0 height :\

Any ideas? I can't find any reference to this in the Mozilla docs.

Thanks,
Matthew
Attachment #8509201 - Attachment is obsolete: true

Comment 17

5 years ago
Comment on attachment 8511600 [details] [diff] [review]
Above-input status bar (doesn't slide in)

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

I would start by looking at the CSS of the findbar (mozilla-central/toolkit/content/widgets/findbar.xml, http://dxr.mozilla.org/mozilla-central/search?q=findbar+ext%3Acss+path%3Atoolkit&case=false), maybe it can be reused.

::: mail/components/im/content/imconversation.xml
@@ +33,5 @@
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
>          <xul:stack anonid="conv-bottom" class="conv-bottom">
> +          <xul:vbox>
> +            <xul:description anonid="convStatus" class="conv-status" value="" />
> +            <xul:splitter class="splitter" />

Why are you adding a splitter?
Assignee

Comment 18

5 years ago
Here it is with the find-bar animation copied in, I think it looks alright but it feels pretty jerky. It seems like margin-top isn't actually being animated. In fact, I can't seem to get .conv-status to animate its margins at all :\

I'm gonna play around with it some more hopefully over the weekend, but I've been pretty busy lately.
Attachment #8511600 - Attachment is obsolete: true
Attachment #8514749 - Flags: review?(aleth)

Comment 19

5 years ago
Do you see how to fix the CSS transition issue?
Flags: needinfo?(richard.marti)
It seems the description doesn't like the animations. All I tried ends in a immediate appearance/disappearance. When I build a hbox around the description and animate this hbox then it works. It needs only a trigger to animate this hbox and not the description.
Flags: needinfo?(richard.marti)

Comment 21

5 years ago
Comment on attachment 8514749 [details] [diff] [review]
Above-input status bar, fades in

Apart from following Paenglab's solution, please confirm this works for very small windows (What happens when the description appears and there is not enough vertical space? What happens when the description is present and the window is made very small? What happens when the label text is longer than the available space?)
Attachment #8514749 - Flags: review?(aleth) → review-
Assignee

Comment 22

5 years ago
Here it is with Paenglab's animation fix, still double checking on small windows but it seems to work alright (unless the text is long, which I'll test hopefully on Thursday night).

Thanks,
Matthew
Attachment #8514749 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
And here it is without brackets on the one-line if/thens
Attachment #8516464 - Attachment is obsolete: true
Assignee

Comment 24

5 years ago
aaand...here it is with resetInput resetting the status text again.
Attachment #8516465 - Attachment is obsolete: true

Comment 25

5 years ago
Comment on attachment 8516466 [details] [diff] [review]
Above-input status bar, fades & slides in

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

Looks good - I'm pretty sure the next version will be r+, unless you discover problems in your further testing.

::: mail/components/im/content/imconversation.xml
@@ +135,5 @@
> +         convStatus.value = this._statusText;
> +         if(this._statusText.length > 0)
> +           convStatusContainer.removeAttribute("hidden");
> +         else
> +           convStatusContainer.setAttribute("hidden", "");

Nit: We generally use setAttribute("hidden", "true")

::: mail/components/im/themes/chat.css
@@ +313,5 @@
> +  transition-property: margin-top, opacity, visibility;
> +  transition-duration: 150ms, 150ms, 0s;
> +  transition-timing-function: ease-in-out, ease-in-out, linear;
> +}
> +  .conv-status-container[hidden] {

I see why you did this, but please don't indent CSS blocks.

Comment 26

5 years ago
Comment on attachment 8516466 [details] [diff] [review]
Above-input status bar, fades & slides in

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

::: mail/components/im/content/imconversation.xml
@@ +32,5 @@
>          </xul:hbox>
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
>          <xul:stack anonid="conv-bottom" class="conv-bottom">
> +          <xul:vbox>
> +            <hbox anonid="convStatusContainer" class="conv-status-container" hidden="true">

Would it look better to place this hbox between the splitter-bottom and the stack, so the input box doesn't shrink when a statusText appears?

@@ +33,5 @@
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
>          <xul:stack anonid="conv-bottom" class="conv-bottom">
> +          <xul:vbox>
> +            <hbox anonid="convStatusContainer" class="conv-status-container" hidden="true">
> +              <xul:description anonid="convStatus" class="plain conv-status" value="" />

Almost certainly needs a crop="end".
Attachment #8516466 - Flags: review-
Assignee

Comment 27

5 years ago
This should have all those fixes, putting the <hbox> before the input box's stack does seem to look better.

Thanks,
Matthew
Attachment #8516466 - Attachment is obsolete: true
Attachment #8517198 - Flags: review?(aleth)
Assignee

Comment 28

5 years ago
Removed the now-useless vbox around the input
Attachment #8517198 - Attachment is obsolete: true
Attachment #8517198 - Flags: review?(aleth)
Attachment #8517199 - Flags: review?(aleth)

Comment 29

5 years ago
Comment on attachment 8517199 [details] [diff] [review]
Above-input status bar, fades & slides in

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

r+ with nits fixed.

::: mail/components/im/content/imconversation.xml
@@ +31,5 @@
>            </xul:notificationbox>
>          </xul:hbox>
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
> +        <hbox anonid="convStatusContainer" class="conv-status-container" hidden="true">
> +          <xul:description anonid="convStatus" class="plain conv-status" value="" crop="end" />

Do you actually need to set value="" here or is that already done dynamically by the JS anyway?

@@ +130,5 @@
> +       <![CDATA[
> +         let convStatusContainer = this.getElt("convStatusContainer");
> +         let convStatus = this.getElt("convStatus");
> +         convStatus.value = this._statusText;
> +         if(this._statusText.length > 0)

Nit: space between if and (
You can also shorten this to if (this._statusText.length)
Attachment #8517199 - Flags: review?(aleth) → ui-review?(richard.marti)
Assignee

Comment 30

5 years ago
> Do you actually need to set value="" here or is that already done dynamically by the JS anyway?
Pretty sure I didn't, but now that it's hiding based on the hbox there's no reason to have it anyways. Should be taken out now.

> Nit: space between if and (
You can also shorten this to if (this._statusText.length)
Fixed

Thanks,
Matthew
Attachment #8517199 - Attachment is obsolete: true
Attachment #8517199 - Flags: ui-review?(richard.marti)
Attachment #8517480 - Flags: review?(aleth)

Comment 31

5 years ago
Comment on attachment 8517480 [details] [diff] [review]
Above-input status bar, fades & slides in

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

Thanks!

::: mail/components/im/themes/chat.css
@@ +318,5 @@
> +.conv-status-container[hidden] {
> +  /* Override display:none to make the transition work. */
> +  display: -moz-box;
> +  visibility: collapse;
> +  margin-top: -1em;

What is this margin-top for, by the way? Does it in fact do anything when visibility = collapse?
Attachment #8517480 - Flags: ui-review?(richard.marti)
Attachment #8517480 - Flags: review?(aleth)
Attachment #8517480 - Flags: review+
Assignee

Comment 32

5 years ago
(In reply to aleth [:aleth] from comment #31)
> What is this margin-top for, by the way? Does it in fact do anything when
> visibility = collapse?

That was there in the find bar's animation (as a margin-bottom), but what I *think* it does is move the bar up when hidden so that it has the slide down animation when it's unhidden & slide up animation when hidden.

Trying it out without the margin makes it feel more "blocky" to me, it just fades out then goes away all at once (once visibility: collapse gets put into effect).

Comment 33

5 years ago
(In reply to Matthew Sotoudeh [:msot] from comment #32)
> (In reply to aleth [:aleth] from comment #31)
> > What is this margin-top for, by the way? Does it in fact do anything when
> > visibility = collapse?
> 
> That was there in the find bar's animation (as a margin-bottom), but what I
> *think* it does is move the bar up when hidden so that it has the slide down
> animation when it's unhidden & slide up animation when hidden.
> 
> Trying it out without the margin makes it feel more "blocky" to me, it just
> fades out then goes away all at once (once visibility: collapse gets put
> into effect).

Clearly the transition on the margin property matters, but what I mean is I would like to understand better why the findbar (also displayed at the bottom) uses margin-bottom and why this change doesn't seem to make a difference.

I'd also like to know why you put a border on the bottom of the hbox which is 1px high but has two colours assigned to it (I assume one is not used). If you lifted this from the findbar top border (which I don't think you did as why would you move it to the bottom), note that the styling there is OS-dependent. Or do the colours match something in the TB UI? In that case a comment might be useful.
Comment on attachment 8517480 [details] [diff] [review]
Above-input status bar, fades & slides in

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

Looks good.

As aleth already wrote the border color can be only one color, the ThreeDShadow. But on Aero you need to add this rule:

@media (-moz-windows-default-theme) {
  .conv-status-container {
    border-bottom-color: #A9B7C9;
  }
}

and on OS X the border color should be #8b8b8b.

With this ui-r+

aleth, I tried margin-bottom: -1em and it makes no difference to the margin-top rule.
Attachment #8517480 - Flags: ui-review?(richard.marti) → ui-review+

Comment 35

5 years ago
(In reply to Richard Marti (:Paenglab) from comment #34)
> Comment on attachment 8517480 [details] [diff] [review]
> As aleth already wrote the border color can be only one color, the
> ThreeDShadow. But on Aero you need to add this rule:

I was also surprised that it was a border-*bottom*.

> aleth, I tried margin-bottom: -1em and it makes no difference to the
> margin-top rule.

Right, I just don't understand exactly how that margin transition works and hoped you would ;)
(In reply to aleth [:aleth] from comment #35)
> 
> Right, I just don't understand exactly how that margin transition works and
> hoped you would ;)

With a previous patch where the description came into the textbox you could see, when disabling the visibility and opacity rules, how the box comes down from the splitter and when -1em the container stays above the splitter (only partially because 1em is not the full height). With the last patch it's not so visible because the splitter is moving and the container stays still, but it is still the same as before.

Updated

5 years ago
Attachment #8517480 - Flags: review+
Assignee

Comment 37

5 years ago
Here it is with the OS styles, anything else?

Thanks,
Matthew
Attachment #8517480 - Attachment is obsolete: true
Attachment #8519355 - Flags: ui-review?(richard.marti)
Attachment #8519355 - Flags: review?(aleth)
Comment on attachment 8519355 [details] [diff] [review]
Above-input status bar, fades & slides in

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

ui-r- because the special border colors aren't shown. I'm sure the next version will be ready for check-in.

Thank you for your patience.

::: mail/components/im/themes/chat.css
@@ +307,5 @@
> +  height: 20px;
> +  padding: 3px;
> +
> +  border-bottom: 1px solid;
> +  -moz-border-bottom-colors: ThreeDShadow;

You are still using -moz-border-bottom-colors which has always precedence to border-bottom-color and the special OS X and Aero colors aren't shown. Use 'border-bottom: 1px solid ThreeDShadow;' instead.
Attachment #8519355 - Flags: ui-review?(richard.marti) → ui-review-

Comment 39

5 years ago
I ported this for Instantbird in https://hg.mozilla.org/comm-central/rev/d351cea9d237 and discovered that I had an easier time with the CSS when the status bar was above the splitter. I then didn't need a border-bottom on the status bar. For Instantbird, we are doing Windows and Linux tweaking in a followup, so I'm not sure how this looks on those OS yet.

Comment 40

5 years ago
(In reply to aleth [:aleth] from comment #39)
Just to avoid confusion, of course the surroundings in Instantbird are a bit different, so the colours in particular probably won't be the same.

Updated

5 years ago
Blocks: 954845
Assignee

Comment 41

5 years ago
This has the border-bottom changes.

I'm not sure what problems putting it above the splitter would fix - didn't you basically just replace the border-bottom with a border-top to differentiate it from the main tweet view?

Thanks,
Matthew
Attachment #8519355 - Attachment is obsolete: true
Attachment #8519355 - Flags: review?(aleth)
Attachment #8520153 - Flags: ui-review?(richard.marti)
Attachment #8520153 - Flags: review?(aleth)

Comment 42

5 years ago
Comment on attachment 8520153 [details] [diff] [review]
Above-input status bar, fades & slides in

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

>Not sure what problems putting it above the splitter would fix - didn't you basically just replace the border-bottom with a border-top to >differentiate it from the main tweet view?

I just thought I'd mention it as for IB the splitter could be a bit thick to use as a border-top replacement. Doesn't seem to apply here.

By the way, thanks again for your work on this! Your patches were very helpful in getting rid of the status bar on Instantbird.

::: mail/components/im/content/imconversation.xml
@@ +586,5 @@
>        <body>
>        <![CDATA[
>          var inputBox = this.editor;
>          inputBox.value = "";
> +        this._statusText = "";

This has likely bitrotted (trivially). Please hg pull -u and rebase.

::: mail/themes/windows/mail/chat-aero.css
@@ +74,5 @@
>  @media (-moz-windows-default-theme) {
> +  .conv-status-container {
> +    border-bottom-color: #A9B7C9;
> +  }
> +  

Nit: please don't add trailing whitespace. There's probably a setting in your editor that allows you to automatically get rid of this kind of thing.
Attachment #8520153 - Flags: review?(aleth) → review-
Comment on attachment 8520153 [details] [diff] [review]
Above-input status bar, fades & slides in

As aleth already wrote the patch doesn't apply. But the changes in CSS are looking good.
Attachment #8520153 - Flags: ui-review?(richard.marti)
Assignee

Comment 44

5 years ago
This *should* apply, I was avoiding a checkout because it was messing with my build a bit last time I pulled :\

Thanks,
Matthew
Attachment #8520153 - Attachment is obsolete: true
Attachment #8520401 - Flags: ui-review?(richard.marti)
Attachment #8520401 - Flags: review?(aleth)

Comment 45

5 years ago
Comment on attachment 8520401 [details] [diff] [review]
Above-input status bar, fades & slides in

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

Looks good, thanks!
Attachment #8520401 - Flags: review?(aleth) → review+

Comment 46

5 years ago
(In reply to Matthew Sotoudeh [:msot] from comment #44)
> This *should* apply, I was avoiding a checkout because it was messing with
> my build a bit last time I pulled :\

Btw, you can often avoid unwelcome surprises by checking whether the latest build succeeded on treeherder before pulling.
Comment on attachment 8520401 [details] [diff] [review]
Above-input status bar, fades & slides in

Looks good, thanks too!
Attachment #8520401 - Flags: ui-review?(richard.marti) → ui-review+

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 48

5 years ago
Great, and thanks for the tip with treeherder :D
Thanks!

https://hg.mozilla.org/comm-central/rev/ed470ac5123a
Assignee: nobody → matthewsot
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
You need to log in before you can comment on or make changes to this bug.