736002 - The editor for twitter should show inputtable character count

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

When I type in the editor for twitter, the editor accepts over 140 characters. Then, when I press Enter key, I see the error message in the timeline.

It's hard to count the characters before submitting the tweet. The UI should show the remaining inputtable character count next to the editor.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Or at a minimum, show how many characters you've used in the "Status is over 140 characters." message it returns.

Comment 3

[Patrick Cloke [:clokep]]

The API exists for this (and is used in Instantbird), seems that no UI was ever hooked up to it for Thunderbird.

Comment 4

[Matthew Sotoudeh [:msot]]

Hello! I'm new here, and would like to try and fix this. Is there anything I need to get set up before I start? I've got a working build from the source.

Thanks!

Comment 5

[Matthew Sotoudeh [:msot]]

Attachment: Partial patch for bug 7326002, more descriptive error message

I went ahead and tried it out, this should improve the error message returned to include the total character count, max character count, and how many characters you need to remove. It doesn't show you the character count as you type, but it does improve the situation a bit.

Comment 6

[aleth [:aleth]]

Thanks for looking into this! You could take a look at the Instantbird code around http://mxr.mozilla.org/comm-central/source/im/content/conversation.xml#868, which uses http://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#48, if you'd like to add a character counter. I guess the main problem is finding a good place to put it in the TB UI, then you can port this code to imconversation.xml (the TB equivalent of conversation.xml).

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8488966 [details] [diff] [review]
Partial patch for bug 7326002, more descriptive error message

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

As aleth mentioned the correct way to fix this is to add a character counter to the UI *somewhere*. I'm not really sure where though, let's ask one of the UI guys if they have an idea!

Comment 8

[Matthew Sotoudeh [:msot]]

Attachment: Added the inputValueChanged code from Instantbird, # of chars left shown in the status bar

Sounds good :)
I know we'll need to wait for the UI guy before actually fixing it, but to get a bit more acquainted with it I've gone ahead and copied the code that aleth linked to into the source (which tries to show it in the statusbar).

It turns out that twitter.js already handles inputValueChanged, but displayStatusText (http://dxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#1015) doesn't actually change the status bar's text because XULBrowserWindow doesn't exist.

Two questions:

Is displayStatusText supposed to work, or have I done something wrong? And if not, is there a better way of setting the status text other than just updating #statusText?

Thanks!

Comment 9

[Patrick Cloke [:clokep]]

I'll take a look at this soon, but displayStatusText being unimplemented is the issue that needs to be solved. Thanks for taking a look at this, by the way! Changing just the UI layer should be necessary, as you've done. I think displaying in the status bar is a good first proposal, we'll see what Josiah thinks!

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8489085 [details] [diff] [review]
Added the inputValueChanged code from Instantbird, # of chars left shown in the status bar

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

So this looks like it's a straight port of the code that aleth pointed to, but it seems TB already has some of this code around http://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#522, it even calls displayStatusText at http://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#553 So we need to investigate why this isn't working and fix that. So you should just need to fix displayStatusText to actually display the result, assuming the other code works.

Comment 11

[Matthew Sotoudeh [:msot]]

You're completely right, I didn't even think to check for anything else that would call that.

Looking through it, what's failing is the check for "XULBrowserWindow" on window at http://dxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#1021. Builds haven't been working for me recently, so I can't try anything out till later tonight, but I'll look into using the XULBrowserWindow defined at http://dxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#75 and http://dxr.mozilla.org/comm-central/source/im/content/tabbrowser.xml#1809, or porting Instantbird's XULBrowserWindow from http://dxr.mozilla.org/comm-central/source/im/content/instantbird.js#124.

Thanks!

Comment 12

[Matthew Sotoudeh [:msot]]

Attachment: Added XULBrowserWindow from Instantbird

OK, I've copied/fixed XULBrowerWindow from instantbird.js into mailWindow.js, and now when typing a tweet it correctly displays the number of chars left in the status bar (meaning displayStatusText works). I've also updated the error message to be a bit more descriptive.

A few more questions:
* Would it be better to add XULBrowserWindow like this, or just to update displayStatusText to directly edit the status text with document.getElementById("statusText").label?
* Is mailWindow.js the right place to put XULBrowserWindow? I glanced over the file names in mail/, and mailWindow was the only one that seemed to be loaded by all pages.
* It seems like Instantbird has both a status text and an "end status text." As far as I can tell, Thunderbird only has one status text, so I've modified it so setStatus and setStatusEnd do basically the same thing. Would it be better to just take out everything but setStatusEnd?


Thanks!

Comment 13

[aleth [:aleth]]

Comment on attachment 8491964 [details] [diff] [review]
Added XULBrowserWindow from Instantbird

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

Thanks for looking at this!

::: chat/locales/en-US/twitter.properties
@@ +7,5 @@
>  twitter.protocolName=Twitter
>  
>  # LOCALIZATION NOTE (error.*):
>  #   These are errors that will be shown to the user in conversation.
> +error.tooLong=Status is %S characters, but the maximum length is 140 (%S too many).

This won't work - have you tested this patch? If you really want to change the string, you'll need to use multiple parameters %1$S, %2$S (see elsewhere in the file) and then change the name of the string so localizers also pick up the change (e.g. error.tooLong -> error.tooLong2).

::: mail/base/content/mailWindow.js
@@ +623,5 @@
>    messenger.setDocumentCharset(msgWindow.mailCharacterSet);
>  }
> +
> +// Copied from mozilla/browser/base/content/browser.js (and simplified)
> +var XULBrowserWindow = {

I don't think porting this is the right thing to do as this file already contains http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#158. Doesn't nsMsgStatusFeedback already contain the methods you need for this? If not, that's probably the right place to put them. Note you can access it via the XULBrowserWindow property set here http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#75

Comment 14

[aleth [:aleth]]

Paenglab, do you have any thoughts about where the character counter should go in the TB UI? If the status bar is used, since the status bar is wider than the input box in TB it might be a good idea to add an explanatory text at least ("Characters remaining: N" or something like that)

Comment 15

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #13)
> >  # LOCALIZATION NOTE (error.*):
> >  #   These are errors that will be shown to the user in conversation.
> > +error.tooLong=Status is %S characters, but the maximum length is 140 (%S too many).
> 
> This won't work - have you tested this patch? If you really want to change
> the string, you'll need to use multiple parameters %1$S, %2$S (see elsewhere
> in the file)

Sorry, it does work. The reason we use numbered parameters is so localisers can change the order if that works better for their language.

Comment 16

[Richard Marti (:Paenglab)]

The status bar would be the default. But what is when the status bar is hidden and does the user automatically make the connection between the text he writes and the status bar text?

What about a overlay in the text field on bottom right in Grey with "N characters remaining"? This field is normally tall enough to not overlap the written text with the char count text with the max of 140 characters. Also would be the connection of this feedback more direct for the user.

Comment 17

[Josiah Bruner [:jsbruner]]

Alternatively, as I agree with Richard for the most part, we could avoid the overlay initially by color coding the background of the input area. If you are about to go over the count we could make the background orange (and show the overlay with the count), then red once you go over.

Comment 18

[aleth [:aleth]]

(In reply to Josiah Bruner [:JosiahOne] from comment #17)
> Alternatively, as I agree with Richard for the most part, we could avoid the
> overlay initially by color coding the background of the input area. 

Instantbird doesn't show the counter at all unless you get close to the limit.

Comment 19

[Josiah Bruner [:jsbruner]]

Attachment: Example of color-coding.

Here's an example without the overlay. I think we should show an overlay when we're in the orange or red zone.

Comment 20

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #16)
> What about a overlay in the text field on bottom right in Grey with "N
> characters remaining"? This field is normally tall enough to not overlap the
> written text with the char count text with the max of 140 characters. Also
> would be the connection of this feedback more direct for the user.

Experience with IB shows the number is enough (without a "characters remaining" string), as it's obvious what it is as it changes as you type. The number becomes red once it goes negative (over the limit). Changing the background colour sounds like it's worth experimenting with too though!

We discussed this briefly on #instantbird and agreed that an overlay would be a nice improvement for IB as well :)

Comment 21

[Josiah Bruner [:jsbruner]]

I suppose the next question is: Do we want to always show the overlay? Or only when the status bar is hidden?

Comment 22

[Richard Marti (:Paenglab)]

Twitter is showing the counter always. When it's be showing from beginning the user may know better for what it is than when it appear shortly before it's on 0 and can change his writing during his progress to not come over the max.

Comment 23

[aleth [:aleth]]

(In reply to Josiah Bruner [:JosiahOne] from comment #21)
> I suppose the next question is: Do we want to always show the overlay? Or
> only when the status bar is hidden?

We should avoid using the status bar altogether if we can. A well-designed overlay in the bottom right corner of the textbox should be much nicer.

Comment 24

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #22)
> Twitter is showing the counter always. When it's be showing from beginning
> the user may know better for what it is than when it appear shortly before
> it's on 0 and can change his writing during his progress to not come over
> the max.

It's important to remember Twitter is a bit of a special case. IB has the behaviour you suggest for Twitter (http://mxr.mozilla.org/comm-central/source/im/content/conversation.xml#881), but it is rather distracting for other protocols where the maximum length is much higher and usually not reached.

Comment 25

[Richard Marti (:Paenglab)]

Ah it's also shown on other protocols. How many chars before the max is the counter shown in IB?

Comment 26

[Patrick Cloke [:clokep]]

From aleth's link: "// 200 is a 'magic' constant to avoid showing big numbers."

Comment 27

[Matthew Sotoudeh [:msot]]

Attachment: Color-coded input box, char countdown overlay

Is this along the right lines?

There's a counter overlay in the bottom-right and the background changes color depending on the # of characters left (>50 is white, 0-50 is yellow, and <0 is red).

Thanks,
Matthew

Comment 28

[Josiah Bruner [:jsbruner]]

Comment on attachment 8492786 [details] [diff] [review]
Color-coded input box, char countdown overlay

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

I haven't looked at this yet in-product, just the code, but I had several style nits.

::: mail/components/im/content/imconversation.xml
@@ +35,2 @@
>            <xul:textbox anonid="inputBox" class="conv-textbox" multiline="true" flex="1"/>
> +          <xul:description anonid="charCount" value="" width="35" height="20" right="0" bottom="0" style="background-color:#333333;color:#fff;opacity:0.8;font-size:15px;line-height:20px;text-align:center;display:none;"/>

You're doing a lot of legacy styling here. Can you move most of this to a style sheet please? Everything in style="" definitely can move, and probably the width, height, right, and bottom stuff as well.

@@ +549,3 @@
>                this._statusTextEnd = "";
>                this._statusTextEndIsError = false;
> +              inputBox.style.backgroundColor = "#FFFFFF";

Don't directly modify style properties. Add a single property and change its value depending on its "zone". Like inputBox.limitWarning or something similar. Then you can assign values like normal, warning, and over. That way we can use CSS to style things and can change the styling later without having to dig through XML files. Do the same with charCount actually.

@@ +552,3 @@
>              }
>              else {
> +              charCount.style.display = "block";

See above.

@@ +557,5 @@
>                this._statusTextEnd = left < 200 ? left.toString() : "";
>                this._statusTextEndIsError = left < 0;
> +
> +              if (left >= 50)
> +                inputBox.style.backgroundColor = "#FFFFFF";

Use a specific property, not the style one. (As mentioned above)

@@ +559,5 @@
> +
> +              if (left >= 50)
> +                inputBox.style.backgroundColor = "#FFFFFF";
> +              else if (left < 50 && left >= 0)
> +                inputBox.style.backgroundColor = "#F2C500";

Ditto.

@@ +561,5 @@
> +                inputBox.style.backgroundColor = "#FFFFFF";
> +              else if (left < 50 && left >= 0)
> +                inputBox.style.backgroundColor = "#F2C500";
> +              else if (left < 0)
> +                inputBox.style.backgroundColor = "#E94B35";

Ditto and you're going to want to change the foreground color to white I'm guessing.

Comment 29

[Florian Quèze [:florian]]

Coloring the whole background of the inputbox in red seems a bit extreme. I wonder if we could imitate the styling of HTML elements with invalid content. See https://developer.mozilla.org/en-US/docs/Web/CSS/:invalid for some related explanations and http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css#708 for the exact CSS used.

Comment 30

[Matthew Sotoudeh [:msot]]

Attachment: Color-coded input box, char countdown overlay (cleaned up)

Sure! I've moved & simplified most of the styles to mail\components\im\themes\chat.css, except for bottom and right, which the stack element seems to need to see as attributes (moving those to CSS made it fill up the entire box).

I also added a limitWarning attribute (with values "none", "warning", and "error"), show/hide the counter with CSS, and gave the error limitWarning white text.

Florian - yeah, it does look pretty unsubtle to change the whole background, so I tried using Firefox's invalid styling like you suggested, and it ended up like https://i.imgur.com/5Yhuola.jpg. IMO it looks weird to have the textbox look like it's floating over everything else, but maybe a border/different dropshadow would look better?


Thanks,
Matthew

Comment 31

[Josiah Bruner [:jsbruner]]

Yes, the CSS would need to be changed slightly, but I do like that more than coloring the whole box.

Comment 32

[aleth [:aleth]]

Do we really need a "warning" colour change? It seems overly complicated and distracting. The counter showing up at all is enough of a warning I think. (It makes sense to change the colour when over the limit, as then the message can't be sent.)

For the styling of the counter, instead of the floating square with dark background, how about trying something more along the lines of what you get when you hover a URL in Firefox (only in the bottom right)?

Comment 33

[aleth [:aleth]]

Comment on attachment 8492812 [details] [diff] [review]
Color-coded input box, char countdown overlay (cleaned up)

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

::: mail/components/im/themes/chat.css
@@ +306,5 @@
>    padding: 2px;
>    box-sizing: content-box;
>  }
> +.conv-textbox[limitWarning="none"] {
> +  background-color:#FFFFFF;

nit: We add a space between the colon and the value

Comment 34

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #32)
> Do we really need a "warning" colour change? It seems overly complicated and
> distracting. The counter showing up at all is enough of a warning I think.
> (It makes sense to change the colour when over the limit, as then the
> message can't be sent.)

It could also only change the counter color for warning.

Comment 35

[Patrick Cloke [:clokep]]

Comment on attachment 8492812 [details] [diff] [review]
Color-coded input box, char countdown overlay (cleaned up)

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

Looks like you've gotten some great feedback so far, sorry this took so long to review for you! I think it looks pretty good overall and hopefully just needs a few more iterations. I've left you a few comments that may or may not still be applicable depending on the UI improvements that have been suggested by Josiah and Florian. Thanks for the work so far!

::: mail/components/im/content/imconversation.xml
@@ +549,2 @@
>                this._statusTextEnd = "";
>                this._statusTextEndIsError = false;

We can now remove things that modify the statusText, right? (We might be able to fully remove these properties, in fact!)

@@ +556,5 @@
>                this._statusTextEnd = left < 200 ? left.toString() : "";
>                this._statusTextEndIsError = left < 0;
> +
> +              if (left >= 50)
> +                inputBox.setAttribute("limitWarning", "none");

I'd suggest removing the attribute here instead of setting it to a string of "none".

@@ +560,5 @@
> +                inputBox.setAttribute("limitWarning", "none");
> +              else if (left < 50 && left >= 0)
> +                inputBox.setAttribute("limitWarning", "warning");
> +              else if (left < 0)
> +                inputBox.setAttribute("limitWarning", "error");

I find it odd that you're setting an attribute called "limitWarning" to a value of "error". Maybe rename this to "inputValid" or something more declarative. This would work well if you get rid of the warning state.

::: mail/components/im/themes/chat.css
@@ +331,5 @@
> +  padding:3px;
> +}
> +.conv-counter[value=""] {
> +  display:none;
> +}

You'll probably want to add a third clause here which is .conv-counter when the number of characters is too many (e.g. a negative number). This should turn the counter red or something.

Comment 36

[Matthew Sotoudeh [:msot]]

Attachment: Color-coded input box, char countdown overlay (re-cleaned up)

Hello,

This now uses a border instead of the box-shadow/background-color, which I personally think looks a lot nicer: https://i.imgur.com/S3HZDev.jpg. Would that be all right, or is it better just to remove all effects on the textbox and stick to the counter?

Sorry for all the questions, but:

Should the counter show up right when the user starts typing, or wait until they get to, say, 50 characters left? The box definitely needs to be redesigned, the whole black background was just the first thing that popped into my head.

I've also removed the attributes (instead of setting them to "none"), and renamed them. I ended up going with an "invalid" attribute, but I'm not sure if that was the best idea. The only reason I didn't go with "inputValid" like you suggested is that it's really only being used when the input is invalid, and otherwise it'd just be removed. Is there any reason to use inputValid over invalid or inputInvalid?

Took out all the warnings and changed the counter color to a rather dark red when the user goes over the limit, though that makes it somewhat hard to read. Maybe changing the background color (or using a lighter red) would help? Also removed the statusText stuff as you suggested.

There's some other code in imconversation.xml that tries to use displayStatusText to change the status - is there any point to fixing displayStatusText, or should all that just be taken out?


Thanks!

Comment 37

[Richard Marti (:Paenglab)]

Comment on attachment 8495700 [details] [diff] [review]
Color-coded input box, char countdown overlay (re-cleaned up)

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

(In reply to Matthew Sotoudeh [:msot] from comment #36)
> 
> Should the counter show up right when the user starts typing, or wait until
> they get to, say, 50 characters left? The box definitely needs to be
> redesigned, the whole black background was just the first thing that popped
> into my head.

I think the previous magic number of 200 was a good compromise.

> I ended up going with an "invalid" attribute, but I'm not sure if that
> was the best idea. The only reason I didn't go with "inputValid" like you
> suggested is that it's really only being used when the input is invalid,
> and otherwise it'd just be removed. Is there any reason to use inputValid
> over invalid or inputInvalid?

What about limitReached?

::: mail/components/im/themes/chat.css
@@ +306,5 @@
>    padding: 2px;
>    box-sizing: content-box;
>  }
> +.conv-textbox[invalid="true"] {
> +  border: 1px  solid red;

Only one space between 1px and solid.

On your screenshot on the left and right where is no red border visible but they should.

@@ +316,5 @@
>  }
>  %endif
>  
> +.conv-counter {
> +  background-color: #333333;

Try to use transparency like: background-color: rgba(0,0,0,0.1) then also the opacity isn't needed and the red text is better visible.

Comment 38

[aleth [:aleth]]

(In reply to Matthew Sotoudeh [:msot] from comment #36)
> This now uses a border instead of the box-shadow/background-color, which I
> personally think looks a lot nicer: https://i.imgur.com/S3HZDev.jpg. Would
> that be all right, or is it better just to remove all effects on the textbox
> and stick to the counter?

I like the idea of changing the border colour, but the border should be on all sides of the input box, not just the top and bottom.

> Should the counter show up right when the user starts typing, or wait until
> they get to, say, 50 characters left? The box definitely needs to be
> redesigned, the whole black background was just the first thing that popped
> into my head.

Did you see my suggestion in comment #32? The counter should be unobtrusive and in the corner so it is least likely to overlap text the user has entered (this may be unlikely to happen for Twitter, but for protocols with a much larger limit it is more likely).

> I've also removed the attributes (instead of setting them to "none"), and
> renamed them. I ended up going with an "invalid" attribute, but I'm not sure
> if that was the best idea. The only reason I didn't go with "inputValid"
> like you suggested is that it's really only being used when the input is
> invalid, and otherwise it'd just be removed. Is there any reason to use
> inputValid over invalid or inputInvalid?

Personally I rather like the invalid attribute you used. After all, we may wish to set it in other circumstances where a message can't be sent, not just when the message length is over the limit.

> Took out all the warnings and changed the counter color to a rather dark red
> when the user goes over the limit, though that makes it somewhat hard to
> read. Maybe changing the background color (or using a lighter red) would
> help? Also removed the statusText stuff as you suggested.

If you do use a background colour, make it extremely light.

> There's some other code in imconversation.xml that tries to use
> displayStatusText to change the status - is there any point to fixing
> displayStatusText, or should all that just be taken out?

I believe it's also currently used to display typing notifications, so it's worth fixing (or, possibly, removing and then displaying the typing notifications in a different way too). But that should be done in a separate bug.

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8495700 [details] [diff] [review]
Color-coded input box, char countdown overlay (re-cleaned up)

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

The code looks good overall, just a couple of minor nits. Note that I didn't look closely at the CSS, just the JavaScript. Please use a generic term (like invalidInput) and not limitReached so this functionality can be reused.

(In reply to aleth [:aleth] from comment #38)
> (In reply to Matthew Sotoudeh [:msot] from comment #36)
> > I've also removed the attributes (instead of setting them to "none"), and
> > renamed them. I ended up going with an "invalid" attribute, but I'm not sure
> > if that was the best idea. The only reason I didn't go with "inputValid"
> > like you suggested is that it's really only being used when the input is
> > invalid, and otherwise it'd just be removed. Is there any reason to use
> > inputValid over invalid or inputInvalid?
> 
> Personally I rather like the invalid attribute you used. After all, we may
> wish to set it in other circumstances where a message can't be sent, not
> just when the message length is over the limit.

There is no reason in this case. Generally it's good to use the positive form because it makes programming logic easier to understand (i.e. !invalid vs !valid vs invalid vs valid). Less levels of indirection. To be clear: using invalid here is better.

> > There's some other code in imconversation.xml that tries to use
> > displayStatusText to change the status - is there any point to fixing
> > displayStatusText, or should all that just be taken out?
> 
> I believe it's also currently used to display typing notifications, so it's
> worth fixing (or, possibly, removing and then displaying the typing
> notifications in a different way too). But that should be done in a separate
> bug.

It'd be awesome if this was fixed also! We've floated around some ideas of what to replace that with. :)

As an aside, would you mind attaching your screenshots to bugzilla? I find it much easier to follow when everything is in one place (and a lot of firewalls like to block imgur...).

::: mail/components/im/content/imconversation.xml
@@ +545,5 @@
>              // When the input box is cleared or there is no character limit,
>              // don't show the character limit in the statusbar.
>              if (left == Ci.prplIConversation.NO_TYPING_LIMIT || !text.length) {
> +              charCounter.setAttribute("value", "");
> +              inputBox.removeAttribute("invalid");

Please be explicit and name this "invalidInput".

@@ +550,3 @@
>              }
>              else {
> +              charCounter.setAttribute("value", left.toString());

Is the toString() necessary here? Seems like it would auto-cast.

@@ +555,5 @@
> +                inputBox.removeAttribute("invalid");
> +              }
> +              else if (left < 0) {
> +                inputBox.setAttribute("invalid", "true");
> +              }

Nit: Match the style of the code this is in (no { } around single line statements).

::: mail/components/im/themes/chat.css
@@ +325,5 @@
> +}
> +.conv-counter[value=""] {
> +  display: none;
> +}
> +.conv-counter[value^="-"] {

Maybe I'm just not well versed enough in CSS, but I'd like a comment above this saying it's for negative numbers. :)

Comment 40

[Matthew Sotoudeh [:msot]]

Attachment: Color-coded input borders, char counter

Hello!
I've tried to fix most of the things with this patch, except for redesigning the counter (which I'll play with this weekend).

(replying to Richard Marti)
> I think the previous magic number of 200 was a good compromise.
Even for Twitter? Twitter limits to 140 characters, so it would essentially make the counter show up always.

> Only one space between 1px and solid.
Fixed this, not sure how that got in there :)

> On your screenshot on the left and right where is no red border visible but they should.
I've fixed this by adding 1px margins to the right & left of the textbox, not completely sure why but apparently there's a bit of an overlap between the textbox and the surrounding panels.

> Try to use transparency like: background-color: rgba(0,0,0,0.1) then also the opacity isn't needed and the red text is better visible.
Thanks, ended up going with 0.35 alpha, and the red is definitely more visible.


(replying to aleth)
> I like the idea of changing the border colour, but the border should be on all sides of the input box, not just the top and bottom.
Fixed

> Did you see my suggestion in comment #32? The counter should be unobtrusive and in the corner so it is least likely to overlap text the user has entered (this may be unlikely to happen for Twitter, but for protocols with a much larger limit it is more likely).
I saw the suggestion, but am not completely sure how to go about it :\ The only way I could imagine that happening would be if the box wasn't transparent, which would overlap more with the text than it does now. I'll try playing around with it over this weekend and seeing how it works out tho.

> If you do use a background colour, make it extremely light.
I think we should go ahead without a background color, seeing as the border & counter color are pretty apparent.


(replying to Patrick Cloke)
> Please use a generic term (like invalidInput) and not limitReached so this functionality can be reused.
Went ahead and renamed it to invalidInput.

> It'd be awesome if this was fixed also! We've floated around some ideas of what to replace that with. :)
So should a new bug be started for displayStatusText, or should it still be fixed as a part of this bug?

> As an aside, would you mind attaching your screenshots to bugzilla? I find it much easier to follow when everything is in one place (and a lot of firewalls like to block imgur...).
Sure, still getting used to bugzilla, but is there any way to attach two things to one comment? As in, attach an image to the same comment that I make when attaching a patch, or should those be put in two different comments?

> Is the toString() necessary here? Seems like it would auto-cast.
It's not, but comming from C# leaving it out just felt dirty :P I've taken it out in this patch.

> Nit: Match the style of the code this is in (no { } around single line statements).
Fixed

> Maybe I'm just not well versed enough in CSS, but I'd like a comment above this saying it's for negative numbers. :)
I've added comments to explain that & the margins.


Thanks,
Matthew

Comment 41

[Patrick Cloke [:clokep]]

Comment on attachment 8496396 [details] [diff] [review]
Color-coded input borders, char counter

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

(In reply to Matthew Sotoudeh [:msot] from comment #40)
> I've tried to fix most of the things with this patch, except for redesigning
> the counter (which I'll play with this weekend).
Great! Once you work that out a bit, you can request review from me and ui-review from Paenglab!

> (replying to Richard Marti)
> > I think the previous magic number of 200 was a good compromise.
> Even for Twitter? Twitter limits to 140 characters, so it would essentially
> make the counter show up always.
Yep, that was one of the goals of that number. :) It looks like you removed the code which checks for this.

> > It'd be awesome if this was fixed also! We've floated around some ideas of what to replace that with. :)
> So should a new bug be started for displayStatusText, or should it still be
> fixed as a part of this bug?
A separate bug, please!

> > As an aside, would you mind attaching your screenshots to bugzilla? I find it much easier to follow when everything is in one place (and a lot of firewalls like to block imgur...).
> Sure, still getting used to bugzilla, but is there any way to attach two
> things to one comment? As in, attach an image to the same comment that I
> make when attaching a patch, or should those be put in two different
> comments?

You don't need to add a comment when adding an attachment, just leave the comment blank (and add a good description like "Screenshot of Patch v1.7").

Comment 42

[Matthew Sotoudeh [:msot]]

Attachment: Counter box only shows up under 200 characters

This adds back the code to only show the counter when there's less than 200 characters.

Due to some build problems I've been having I can't test it right now, but as far as I can tell it should work.

Comment 43

[Matthew Sotoudeh [:msot]]

Attachment: Mockup of a URL-hover-style counter box

> For the styling of the counter, instead of the floating square with
> dark background, how about trying something more along the lines of
> what you get when you hover a URL in Firefox (only in the bottom right)?

Is this what you were thinking? I put my rudimentary photoshop skills to work splicing the URL hover popup onto the chat box and this is what came out :)

Comment 44

[Richard Marti (:Paenglab)]

Comment on attachment 8499293 [details]
Mockup of a URL-hover-style counter box

Looks good. Please add also the ui-r? on the patch you want the reviews. Then I can check it in live.

Comment 45

[Matthew Sotoudeh [:msot]]

Attachment: URL-style counter, color-coded textbox

Here's my best shot at mimicking the URL-hover style mockup, not sure how well it worked. Ended up making the gradients slightly transparent so you can see text that's written under them.

Thanks,
Matthew

Comment 46

[Matthew Sotoudeh [:msot]]

Attachment: Screenshot of URL-style counter box (not overlapping with text)

Here's a screenshot of the new counter when it's not overlapping with any text

Comment 47

[Matthew Sotoudeh [:msot]]

Attachment: URL-style counter (overlapping with text)

And here's one where it does overlap with the text

Comment 48

[Richard Marti (:Paenglab)]

Comment on attachment 8500202 [details] [diff] [review]
URL-style counter, color-coded textbox

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

Looks already good but I have to ui-r- because of the following issues: When the red border is shown the whole text is jumping. This is because it changes from no border to 1px border. If you add to .conv-textbox border: 1px solid transparent and to .conv-textbox[invalidInput="true"] border-color: red it does no more jump. For the .conv-textbox the 1px margin on both sides is only needed on Windows. On OS X there is no margin needed and on Linux it would be -moz-margin-start: 1px. Would, because Linux is special as no red border is shown at all because the native textbox widget is used. Linux needs a -moz-appearance: none to show the border (but again, use it also when not showing the red border). When you are changing this, please style it platform dependent where not all platforms use the same code.

When you are on it please also use only one background color instead of the gradient for the .conv-counter.

Comment 49

[Matthew Sotoudeh [:msot]]

(In reply to Richard Marti (:Paenglab) from comment #48)
> For the .conv-textbox the 1px margin on both sides is only needed on Windows.
> On OS X there is no margin needed and on Linux it would be -moz-margin-start:
> 1px. Would, because Linux is special as no red border is shown at all because
> the native textbox widget is used. Linux needs a -moz-appearance: none to show
> the border (but again, use it also when not showing the red border). When you
> are changing this, please style it platform dependent where not all platforms
> use the same code.

Would the best way to do this be to put platform-specific code in "%ifndef XP_MACOSX"-type statements in mail\components\im\themes\chat.css, or should I put the platform-specific code in mail\themes\(windows/osx/linux)\mail\chat.css? What's the difference between the two?

Thanks,
Matthew

Comment 50

[Richard Marti (:Paenglab)]

Both is okay ;). All in one file with %ifdef comes from IB where it's AFAIC the standard. On TB we are trying to use a common file and platform specific files.

So when you are on it I propose to use mail\components\im\themes\chat.css for the styles which are the same on all platforms and mail\themes\(windows/osx/linux)\mail\chat.css for the theme specific styles. Windows has already a rule for .conv-textbox in mail\themes\windows\mail\chat.css you can use (and need to edit because it has a border: none rule).

Comment 51

[Matthew Sotoudeh [:msot]]

Attachment: Per-OS Styled Counter Box & Warning Border

Is this better? It seems to work on Windows, but I haven't yet tested on OSX/Linux.

Also - why is the extra 1px margin necessary for the border to show up on Windows, but not on OSX/Linux? Is it just a random quirk, or is there a real reason for it?

Thanks,
Matthew

Comment 52

[Patrick Cloke [:clokep]]

Comment on attachment 8502280 [details] [diff] [review]
Per-OS Styled Counter Box & Warning Border

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

A few minor nits. I'm not 100% up to date on our CSS coding styles, so hopefully Richard can add additional comments as he sees fit. Thanks for the work so far! :)

::: mail/components/im/content/imconversation.xml
@@ +30,5 @@
>              </xul:vbox>
>            </xul:notificationbox>
>          </xul:hbox>
>          <xul:splitter class="splitter" anonid="splitter-bottom"/>
> +        <xul:stack anonid="conv-bottom" class="conv-bottom">

Nit: Follow the naming scheme of other anonids: "convBottom".

@@ +550,3 @@
>              }
>              else {
> +              charCounter.setAttribute("value", (left < 200 ? left : ""));

Please keep the comment from before that 200 is arbitrarily chosen: "200 is a 'magic' constant to avoid showing big numbers."

::: mail/components/im/themes/chat.css
@@ +316,5 @@
>  }
>  %endif
>  
> +.conv-counter {
> +  color:#000;

Nit: Space after :.

@@ +328,5 @@
> +}
> +/* Negative counter values (user went over the limit) */
> +.conv-counter[value^="-"] {
> +  color: red;
> +  margin:0 1px 1px 0; /* accounts for the red warning borders */

Nit: Space after the :.

::: mail/themes/windows/mail/chat.css
@@ +78,5 @@
> +  margin: 0 1px 0 1px; /* right/left margins so the borders show up on all sides */
> +}
> +
> +.conv-counter[value^="-"] {
> +  margin:0 2px 1px 0; /* accounts for the red warning borders + extra 1px margin (see .conv-textbox above) */

Nit: Space after the :.

Comment 53

[Richard Marti (:Paenglab)]

Comment on attachment 8502280 [details] [diff] [review]
Per-OS Styled Counter Box & Warning Border

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

Very close but the counter is slightly jumping when it changes to negative values. This is because you add a margin on negative values to not hide the border. When you change the padding together with the margin, the text stays at the same position. Also are your definitions not compatible to rtl locales. In the comments I show how it would work.

::: mail/components/im/themes/chat.css
@@ +319,5 @@
> +.conv-counter {
> +  color:#000;
> +  background-color: rgba(246, 246, 246, 0.7);
> +  font-size: 130%;
> +  padding: 0 5px 0 5px;

Instead of the combined padding, use:

  padding-top: 0;
  padding-bottom: 1px;
  -moz-padding-start: 5px;
  -moz-padding-end: 6px;

@@ +328,5 @@
> +}
> +/* Negative counter values (user went over the limit) */
> +.conv-counter[value^="-"] {
> +  color: red;
> +  margin:0 1px 1px 0; /* accounts for the red warning borders */

Instead of the margin definition, use:

  margin-bottom: 1px;
  -moz-margin-end: 1px;
  padding-top: 1px;
  padding-bottom: 0;
  -moz-padding-end: 5px

::: mail/themes/windows/mail/chat.css
@@ +78,5 @@
> +  margin: 0 1px 0 1px; /* right/left margins so the borders show up on all sides */
> +}
> +
> +.conv-counter[value^="-"] {
> +  margin:0 2px 1px 0; /* accounts for the red warning borders + extra 1px margin (see .conv-textbox above) */

Instead of the margin definition, use:

  -moz-margin-end: 2px;
  -moz-padding-end: 4px;

Comment 54

[Matthew Sotoudeh [:msot]]

Attachment: Counter box & warning border

Is this right? It works on Windows (the counter shouldn't jump anymore), & I *think* I've got spaces after all my colons ;)

Thanks,
Matthew

Comment 55

[Matthew Sotoudeh [:msot]]

Attachment: Counter box & warning border

And here's that patch with the comment added back :)

Comment 56

[Patrick Cloke [:clokep]]

Comment on attachment 8502921 [details] [diff] [review]
Counter box & warning border

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

Just some nits left about comments (and needing UI review). Thanks for hanging with us. Sorry for all the styling comments!

::: mail/components/im/content/imconversation.xml
@@ +550,3 @@
>              }
>              else {
> +              charCounter.setAttribute("value", (left < 200 ? left : "")); //200 is a 'magic' constant to avoid showing big numbers.

Nit: Please keep this on it's own line as it was before. (Generally we like comments on their own line, especially when it would make the line as long as this one is.)

::: mail/components/im/themes/chat.css
@@ +318,5 @@
>  
> +.conv-counter {
> +  color: #000;
> +  background-color: rgba(246, 246, 246, 0.7);
> +  font-size: 130%;  /* padding that gets flipped to margins in .conv-counter[value^="0"] to avoid the red border */

Nit: Please put this on it's own line and make it a real sentence (capital letter to start, ending with a .).

@@ +328,5 @@
> +}
> +.conv-counter[value=""] {
> +  display: none;
> +}
> +/* Negative counter values (user went over the limit) */

Can you change the end of this to "went over the character limit" and add a period at the end. Thanks! (If you see the pattern at this point...please take care of this for all your comments, thanks!)

Comment 57

[Richard Marti (:Paenglab)]

Comment on attachment 8502921 [details] [diff] [review]
Counter box & warning border

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

Okay, now it looks good.

For your question on irc: the -moz-padding-end: 6px is for your normal padding 5px + 1px for the transparent border. When the red border is shown then the padding is 5px and the margin 1px. This lets stay the text at the same position as without the red border.

Comment 58

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #57)
> For your question on irc: the -moz-padding-end: 6px is for your normal
> padding 5px + 1px for the transparent border. When the red border is shown
> then the padding is 5px and the margin 1px. This lets stay the text at the
> same position as without the red border.

To avoid future confusion, maybe a shortened version of this should be a comment in the CSS.

I haven't seen a recent screenshot, but: would it look even better if the counter had a (very light) border-left and border-top as well as a background colour?

Comment 59

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #58)
> I haven't seen a recent screenshot, but: would it look even better if the
> counter had a (very light) border-left and border-top as well as a
> background colour?

Yes, this would circumvent a bit the counter with its light background from textbox. But then use -moz-border-start instead of border-left to be rtl conform.

Comment 60

[Matthew Sotoudeh [:msot]]

Attachment: Counter box & warning border

> Nit: Please keep this on it's own line as it was before. (Generally we
> like comments on their own line, especially when it would make the line
> as long as this one is.)
Pretty sure I've fixed all the comments, as well as adding one attempting to explain what happens to the 6px padding-end.

> would it look even better if the counter had a (very light) border-left
> and border-top as well as a background colour?
I've added a slightly-darker-than-the-background border in this patch, and I think it definitely helps. Truth be told, though, that I can't really distinguish any contrast on my monitor any more without f.lux, so I'm *probably* not the best person to judge that :)

Thanks,
Matthew

Comment 61

[Matthew Sotoudeh [:msot]]

Attachment: Screenshot as of 10/10/14 - counter with border, non-overlapping

Here are a few screenshots of the current counter.

Comment 62

[Matthew Sotoudeh [:msot]]

Attachment: Screenshot as of 10/10/14 - counter with border, overlapping

Comment 63

[Matthew Sotoudeh [:msot]]

Attachment: Screenshot as of 10/10/14 - counter with border, over character limit

Comment 64

[Florian Quèze [:florian]]

It would be nice to have a rounded top left corner on the border, like Firefox has for the URL of the link a user is about to click.

Comment 65

[Matthew Sotoudeh [:msot]]

Is there a border-top-start-radius property that should be used for rtl? I can't find anything like it mentioned on the Mozilla docs, or should I just use border-top-left-radius?

Thanks!

Comment 66

[Richard Marti (:Paenglab)]

(In reply to Florian Quèze [:florian] [:flo] from comment #64)
> It would be nice to have a rounded top left corner on the border, like
> Firefox has for the URL of the link a user is about to click.

On Windows the radius is disabled for triggering grayscale AA (bug 659213). But I think we can use 3px for all platforms.

(In reply to Matthew Sotoudeh [:msot] from comment #65)
> Is there a border-top-start-radius property that should be used for rtl? I
> can't find anything like it mentioned on the Mozilla docs, or should I just
> use border-top-left-radius?

There is no such property. You have to use :-moz-locale-dir(ltr) and :-moz-locale-dir(rtl).

Comment 67

[Richard Marti (:Paenglab)]

Comment on attachment 8503536 [details] [diff] [review]
Counter box & warning border

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

ui-r+ with adding:

.conv-counter:-moz-locale-dir(ltr) {
  border-top-left-radius: 3px;
}

.conv-counter:-moz-locale-dir(rtl) {
  border-top-right-radius: 3px;
}

Comment 68

[Patrick Cloke [:clokep]]

Comment on attachment 8503536 [details] [diff] [review]
Counter box & warning border

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

Just the minor nit below!

::: mail/components/im/content/imconversation.xml
@@ +550,3 @@
>              }
>              else {
> +              //200 is a 'magic' constant to avoid showing big numbers.

Nit: Put a space after the //.

Comment 69

[Matthew Sotoudeh [:msot]]

Attachment: Rounded counter box & warning border

This has the 3px border-radius and a space at the beginning of the comment, anything else?

Thanks,
Matthew

Comment 70

[Patrick Cloke [:clokep]]

Comment on attachment 8504056 [details] [diff] [review]
Rounded counter box & warning border

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

Looks good to me! Thanks. :)

Comment 71

[Richard Marti (:Paenglab)]

Comment on attachment 8504056 [details] [diff] [review]
Rounded counter box & warning border

Yeah, looks good to me too: ui-r+ :)

Comment 72

[Patrick Cloke [:clokep]]

Thanks for working through this with us! This has now been checked into comm-central and should be available in the next set of nightly builds.

http://hg.mozilla.org/comm-central/rev/4cc2cead9507

Matthew, would you mind filing a follow-up bug to fix those other locations that try to use displayStatusText? Thanks!

Comment 73

[Matthew Sotoudeh [:msot]]

Sure, how's https://bugzilla.mozilla.org/show_bug.cgi?id=1084109 ?

Comment 74

[aleth [:aleth]]

Thanks for bringing this feature to TB!

Comment 75

[Matthew Sotoudeh [:msot]]

(In reply to aleth [:aleth] from comment #74)
> Thanks for bringing this feature to TB!

Glad I could help! Thanks everyone for being so supportive while I figured stuff out :)

Comment 76

[Matthew Sotoudeh [:msot]]

Hey,

Going over imconversation now looking for status text references, one thing I noticed is the comment on line 541 (https://mxr.mozilla.org/comm-central/source/mail/components/im/content/imconversation.xml#541) still references "character limit in the statusbar."

Should that be updated? And if so, should that be in a new bug or somehow tagging on to this/1084109?

Thanks,
Matthew

Comment 77

[Patrick Cloke [:clokep]]

You can just attach another patch here. I think that comment is still true if you just remove the "in the statusbar" part, do you agree?

Comment 78

[Matthew Sotoudeh [:msot]]

Attachment: Fixed the character limit comment

Agreed, this should do it :)

Comment 79

[Patrick Cloke [:clokep]]

Comment on attachment 8509189 [details] [diff] [review]
Fixed the character limit comment

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

Thanks! I'll check this in next time I do checkins...

Comment 80

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/e95d563804fa

Comment 81

[aleth [:aleth]]

I've just tried this out and it looks great :) However, at least on OSX, the red border colour never appears (the border is always blue). Can someone confirm it works on Windows and Linux?

Comment 82

[Matthew Sotoudeh [:msot]]

It was working for me on Windows the last time I checked, but I'm rebuilding now & will test it later tonight.

Just a guess, but might it have something to do with Yosemite?

Comment 83

[aleth [:aleth]]

(In reply to Matthew Sotoudeh [:msot] from comment #82)
> Just a guess, but might it have something to do with Yosemite?

Good guess, but I'm still on 10.9.

Comment 84

[Matthew Sotoudeh [:msot]]

Attachment: Working border on Windows

> Good guess, but I'm still on 10.9.
It was worth a try :)

I can confirm that it's working on Windows (at least on the latest public preview of Windows 10). I might be able to test it on OSX soonish (probably this weekend) if nobody else can. I can test on Linux by about then as well.

Comment 85

[aleth [:aleth]]

(In reply to Matthew Sotoudeh [:msot] from comment #84)

I've figured out how to fix the problem on OSX and will attach a patch in a separate bug.

Comment 86

[Wayne Mery (:wsmwk)]

In Moztrap https://moztrap.mozilla.org/manage/case/16267/ thanks to msot for the steps