84950 - Emoticons (smileys) should be themable (skinnable)

Status: RESOLVED FIXED

(Core Graveyard :: Skinability, defect)

Description

[Eric Murphy]

I can't stand the emoticons in Mozilla, and now they remind me of the Wal-Mart Face.

My suggestion is that they be replaced with some that look more realistic to a
human face.

Comment 1

[R.K.Aa.]

That it reminds someone of a Wal-Mart Face is hardly a bug.
Most of the planet wouldn't know what it is, and if you don't like them,
emoticons can be turned off in preferences.

Changing severity to enhancement (tempted to set invalid)

Comment 2

[jay garcia]

Reporter: this is not a bug. If you wish to 'discuss' this issue please do so in 
news://news.mozilla.org/netscape.public.mozilla.general

Comment 3

[Eric Murphy]

Changing summary and reopening.

I want to be able to make my own skin without the Wal-mart icons then. They
scare me. We should be able to use emoticons without being brainwashed.

P.S. This is supposed to be funny, and also a valid bug.

Comment 4

[Keyser Sose]

Marking NEW.

Comment 5

[Matthew T (active 1999-2002)]

Agree. --> Skinability

Comment 6

[lohphat]

See comments in bug 108928

Comment 7

[Karl Ove Hufthammer]

Smileys in ChatZilla are now themable (see e.g. the Sky Pilot theme). However, I
believe smileys in mailnews are still not (at least they're not changed in the
Sky Pilot theme).

Comment 8

[Jean-Marc Desperrier]

I think what would really be useful to customize the emoticons not only by
changing the gif, but also by extending the list of emoticons.
This could be not only a skin level option, but also a preference level option.
(or modifiable in user.js)

For mail/news, the code of :
http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
would need to changed and enhanced so that it will compare the text to a binary
tree of possible text representation of smileys so that the list can be changed
easily without performance problems.

I think the initial bug can be solved just by changing the hard-coded
"chrome://editor/content/images/" in that file to a skinable location.

Comment 9

[Alfred Kayser]

Attachment: Better looking smileys (from Nautipolis)

It would be even better if an 'composite image', such 
as the attachment could be used. Saves a lot of space.

Comment 10

[Jon Henry]

*** Bug 210828 has been marked as a duplicate of this bug. ***

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Mass reassign of my non-Firefox bugs to [e-mail]

Comment 12

[Scott MacGregor]

Here's how I think we should fix this:

1) Modify mozTXTToHTMLConv to not use hard coded images when converting smiley
text. Currently it does something like:

<img src="chrome://editor/content/images/frown.gif"> (I made up that image name)

Instead, create an image with no src, but set a style rule on the image:

<img class="smiley-frown"/>

2) In a skin style sheet, bind the image class names to list style image urls
that point to theme based smileys.

3) Modify the css file loaded for the mail body to include this new skin style
sheet.

Ben, what do you think of this proposal?

Comment 13

[Ben Bucksch (:BenB)]

> <img class="smiley-frown"/>

(class should probably be "moz-smily-frown")

> Ben, what do you think of this proposal?

Great, that's what I intended all the time. My problem was this:

> 3) Modify the css file loaded for the mail body to include this new skin
> style sheet.

This is not sufficient, because the converter is used by other components as
well. It is used in AIM, for example (not sure, if that product still exists),
and it should be used in the browser in the future. So, I didn't know in which
stylesheet to put the rules.

Comment 14

[Ben Bucksch (:BenB)]

One solution might be to create a new CSS file for the converter, putting it
next to the smily images, and requiring all users of the converter to include
the stylesheet. Not exactly nice IMHO, but I don't see another way, apart from
adding these rules to html.css, which is probably not a good idea.

Comment 15

[Scott MacGregor]

Well we don't have to worry about the AIM useage anymore. If the current
consumer is ONLY mail, then I don't think it's unreasonable to have a style
sheet in the theme\directory that contains the smiley mappings and the consumer
of the converter has the burden of proof for including the style sheet that will
bind the images.

If we then add support to the browser to show these icons too, then no big deal,
the piece of browser chrome that gets added to use the converter then just has
to include a reference to the style sheet too.

Sound good? If so, I can take a stab at putting something together.

Maybe something like moz-smileys.css with a chrome path of communicator/skin

Comment 16

[Ben Bucksch (:BenB)]

> If we then add support to the browser to show these icons too, then no big
> deal, the piece of browser chrome that gets added to use the converter then
> just has to include a reference to the style sheet too.

I am thinking mainly of rendering plaintext (.txt) docs loaded in the browser
content using the converter. But I guess we could include the stylesheet in the
HTML header we generate?

> Maybe something like moz-smileys.css with a chrome path of communicator/skin

Whereever it is, I'd put it right next to the smiley images.

> Sound good?

Yup. Thanks.

Comment 17

[Scott MacGregor]

Ben, what do you think about this:

Editor / mail compose uses spans to insert smileys into out going HTML messages.
Mozilla knows how to show these smiley icons when displaying an HTML message
that has spans with these moz class names.

mozTXTToHTMLConv has a separate way of showing smileys in HTML mail for display,
it uses an img src tag with alt text.

This means the CSS I'm putting together has to have two sets of rules that all
point to the same set of images (rules for img src and rules for moz-span).

What do you think about changing mozTXTToHTMLConv to insert span HTML just like
editor does for ::SmileyHit instead of an img src element? Then everyone can use
the same set of CSS rules for showing a smiley.....

Comment 18

[Scott MacGregor]

Attachment: work in progress

This patch shows what I have done so far for classic only. I'll do modern once
we are sure about the location of some of this stuff and the names being used.

This patch shows:
1)one CSS file for all of the smiley bindings, smileys.css which lives in
themes\communicator. Remove all duplicate style bindings in messageBody.css and
in editor. 

2) Rebind the smileys to images in skin and not content

3) Modify SmilyHit to insert a span with a class on it instead of an img src
url. This allows everyone to use one set of span.class based rules for binding
a smily image instead of having a set for the img element code path and a set
for HTML mail sent from mozilla / composer.

4) I modified the class names composer generates. Instead of using
"moz-smiley-s1", moz-smiley-s2, etc. I changed it to "moz-smiley-smile",
moz-smiley-frown. This makes it much more clear to theme authors what icon
should get binded to this class. Of course this also means that viewing a HTML
message sent using the old span class names won't show up right. In the short
term, I could add style rules to smiley.css to 'double' bind each image. i.e.
add temporary bindings using the old -s1 class name to the correct image.

This patch does not show:
1) the icons getting removed from editor and added to
mozilla\themes\classic\communicator\icons

Thoughts before I plow further ahead with this?

Comment 19

[Alfred Kayser]

Attachment: A better set of smileys for classic/modern

The other 'smiley' set has dropshadow which probably not wanted for classic and
modern.
Regarding the 'work in progress': good stuff.
P.s. It would be better to use a composed image (like the attached one),
instead of several image files: much less overhead.

Comment 20

[Scott MacGregor]

hmm the more I think about this, as much as I would like to, I probably
shouldn't change the class names from -s1, -s2 to -smile, -frown...

I'd love to do it 'cause it makes things much less obscure but it will cause
backward compatibility issues when you send an HTML message to a Mozilla 1.6
user which expects the span class to have the old name.

So I think I'll take that out.

Comment 21

[Scott MacGregor]

Attachment: updated patch for modern and classic themes

Here is a finished patch ready for review. I reverted the class names to
moz-smiley-s* to avoid compatibility issues with earlier versions of mozilla
that would not have this change.

I still have one bug to work out. When viewing a plain text message, the
mozTxtToHTMLConv now inserts:

<span class='moz-smiley-s1'><span> :-) </span></span>
instead of an img src url

But for some reason this causes the top and bottom row of pixels for the image
to get cut off. Yet identical HTML contained in an HTML message sent from
mozilla renders perfectly. I'm still trying to figure that out.

To Do:
1) get a review from BenB
2) get module owner approval from Daniel
3) Show Daniel the change he needs to make to the stand alone editor app
4) Land changes for thunderbird

Comment 22

[Scott MacGregor]

Comment on attachment 142793 [details] [diff] [review]
updated patch for modern and classic themes

I won't check this in until I've figured out why the plain text case is gettng
the top row and bottom row of pixels cut off. But the patch itself is ready for
review.

Comment 23

[Scott MacGregor]

cc'ing Daniel. He needs to be in the loop on this proposed change. 

Daniel, this will allow theme authors to skin the smiley icons by moving them
into the skin and out of content.

I have a supplemental patch not shown here which makes this also work for
thunderbird. I also need to make a small patch for
mozilla\composer\...\editorContent.css to import smileys.css and to remove all
of the smiley image binding code there....

Comment 24

[Scott MacGregor]

Alfred, in comment #19, you asked about making us use a single image, leveraging
-moz-image-region to extract each image out of a single image containing all of
the smileys. I'd like to do this, but I don't know if we can. I may need someone
to show me how.

I just tried setting a background-image on a span:

span.moz-smiley-s16 
{
  min-height: 18px; 
  margin-left: 2px; 
  margin-top: 2px;
  padding-left: 20px; 
  background-image: url("chrome://communicator/skin/icons/smileys.png");
  background-repeat: no-repeat; 
  background-position: center center;
  -moz-user-select: all;
}

and then setting the moz region for each specific smiley:

/* sealed */
span.moz-smiley-s16 {
  -moz-image-region: rect(270px 18px 288px 0px);
}

Unfortunately, moz-image-region does not seem to work with background-image.
Does someone know more about if this is possible?

Comment 25

[Ben Bucksch (:BenB)]

Sorry for the delay.

> What do you think about changing mozTXTToHTMLConv to insert span HTML
> just like editor does for ::SmileyHit instead of an img src element?

This definitely makes sense, I actually assumed you'd do it like that. We can't
use an <img> with alt and class and without src, because src is mandatory :-(.

[class name changes]
> backward compatibility issues when you send an HTML message to a Mozilla 1.6

Having a receiving user agent capable of displaying the images is the exception
anyways.

> Yet identical HTML contained in an HTML message sent from
> mozilla renders perfectly.

Related to the wrapping <pre>?

> Unfortunately, moz-image-region does not seem to work with background-image.

-list-style-image? (No idea why editor used background-image, is there a good
reason?) I don't know how much faster region is, but it definitely makes it much
harder to read and alter, which is the whole point of this bug.

Review comments:
- NPL-only licence in new file. Shouldn't this be MPL/LGPL dual license now?

- I'd have maybe other, non-smiley-related rules which the converter could
generate. How would I hook them up? Another stylesheet and including the
smiley.css there? Or should we use a more general name? Or not worry for now?

- I'd use a separate directory for the smiley images, but use whatever you see fit.

- So far, I had a class for all smileys (e.g. moz-txt-smiley, not
moz-smiley-aaa), this allows easier matching in user stylesheets (or is there a
wildcard selector like moz-simley-* in CSS?). Maybe we should keep that in
addition to the specifc classes?

- +    outputHTML += NS_LITERAL_STRING("\"><span> ");     // "> <span> 
  +    outputHTML += NS_LITERAL_STRING(" </span></span>"); // </span></span>
Spaces?

- ":-P",
While you're at it, could you add ";-P" (same meaning), too, please?

Alfred Kayser, please file a new bug about the image changes, but the current
mouths are larger and thus much easier to recognize, esp. on bad/highres
displays. Also, there may be license problems (MPL != GPL).

Comment 26

[Scott MacGregor]

bleh, according to Bug #113577, we don't support moz-background-image-region so
we can't take a single smileys image and use moz-image-region on it.

Comment 27

[Scott MacGregor]

well i figured out why some of the icons look clipped when the smiley is
generated by mozTXTToHTMLConv in response to a text smiley hit. I just don't
know how to fix it. It turns out it all depends on how large a font you are
using for the message pane. If I hit ctrl +, and make the font one size larger
for me, then I no longer get clipped smileys for this case. If I shrink the font
size smaller, I see even more drastic vertical clipping.

I can even reproduce the same problem with reading HTML mail sent from mozilla
where the spans are already part of the source.

So I think what I have uncovered is really a pre-exisiting bug with the span
case that is just more pronounced now because the html converter is also
generating spans. 

Comment 28

[Oneweb]

(In reply to comment #24)
> Alfred, in comment #19, you asked about making us use a single image, leveraging
> -moz-image-region to extract each image out of a single image containing all of
> the smileys. I'd like to do this, but I don't know if we can. I may need someone
> to show me how.

Just my 2c, but what if a user would like animated smilies? afaik, a single
image for all smilies would not allow animation. Ultimate customisability?

Comment 29

[Alfred Kayser]

The theme writer can do what she/he wants, and could do animated smileys...

However, there two other issues:
* use a better / nicer designed set of smileys icons (see attachment for example)
* use an image map in the classic and modern themes: optimisation of these
themes: less footprint, smaller theme jarfile size, better cache usage, etc...

May be we need to create separate bugs for these two issues.

A trick for themers: instead of using a 'background-image' to image the span, it
could be possible to rebind the span.moz-smiley-* as an image or xul thing with
an image inside.

Comment 30

[Alfred Kayser]

Attachment: Encore better, and specially created for Mozilla

(almost) completely re-created set of smileys.
Key thing is that they are 16x16 pixels, which fit better in menu's,
and in the message body in the textlines (except with very small fonts).

This set is smaller (2.3K) than all of the original smileys (16color gifs,
totalling 5K).

At least, this set could be of use for themers, if not used in classic or
modern...

Comment 31

[Scott MacGregor]

Attachment: updated patch addressing Ben's review comments

1) Fixed the wrong license text on the new files
2) Moved the smiley icons into communicator/icons/smileys instead of
communicator/icons
3) Added ";-P" to the list of strings the mozTxtToHTMLConv looks up
4) About the spaces in mozTXTToHTMLConv. I took out the space between the
closing span tags to exactly match the syntax generated by composer when it
adds the spans.
5) About using a shared class in mozTXTToHTMLConv....composer doesn't use that
shared class. Even if both did it, then we'd break backwards compatibility
which I was reluctant to do. Hence I left the class names alone.
6)I fixed my problem with the span not having enough height by adding a font
size attribute to the span contianing the image that matches the height of the
smiley. This works really well.

Let's see if we can get this in before 1.7b closes tonight so themers can do
this for 1.7 final.

Comment 32

[Scott MacGregor]

Comment on attachment 143437 [details] [diff] [review]
updated patch addressing Ben's review comments

asking ben for a review now that I've addressed his comments and suggestions.
Thanks Ben!

Comment 33

[Scott MacGregor]

Comment on attachment 143437 [details] [diff] [review]
updated patch addressing Ben's review comments

I won't check in until I hear a moa from Daniel...

Comment 34

[Ben Bucksch (:BenB)]

Just a few minor comments:

> 1) Fixed the wrong license text on the new files

You used pure MPL now, although it should be tri-license by now per moz.org
policy, but that's probably not important, Gerv will probably builk-change that.

> 2) Moved the smiley icons into communicator/icons/smileys instead of
> communicator/icons

Great, thanks. I see you left smileys.css at the higher dir - that makes sense,
if themes switch to a image file layout, e.g. using the combined image as
proposed by Alfred Kayser.

> 3) Added ";-P" to the list of strings the mozTxtToHTMLConv looks up

Thanks :-)

> 4) About the spaces in mozTXTToHTMLConv. I took out the space between the
> closing span tags to exactly match the syntax generated by composer when it
> adds the spans.

I think the spaces inside the span shouldn't be there either, but maybe the
parser strips them out anyways.

> 5) About using a shared class in mozTXTToHTMLConv....composer doesn't use that
> shared class. Even if both did it, then we'd break backwards compatibility
> which I was reluctant to do. Hence I left the class names alone.

I was thinking of an *additional* class, i.e. 2 classes, that shouldn't hurt
backwards compat, because the current class is still there, but not important.

> 6)I fixed my problem with the span not having enough height by adding a font
> size attribute to the span contianing the image that matches the height of the
> smiley. This works really well.

Good to know that you solved that.


Nothing of what I said is severe, so r=BenB, so that you can check it in before
1.7b closure.

Thanks, mscott,
Ben

Comment 35

[Ben Bucksch (:BenB)]

> Nothing of what I said is severe

Just to be clear: I am *not* asking you to fix any of that.

Comment 36

[David :Bienvenu]

Comment on attachment 143437 [details] [diff] [review]
updated patch addressing Ben's review comments

sr=bienvenu, modulo moa

Comment 37

[Scott MacGregor]

Attachment: patch Daniel can use for stand alone composer

This patch assumes that stand alone composer still brings in communicator
chrome from one of the two default mozilla themes.

Comment 38

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

moa=[e-mail] for editor changes

Sorry guys, but I got the review request at midnight local time... I went to bed
at 11pm, you missed me. Next time, don't forget I am not living in the US ;-)

scott: thanks for the Standalone Composer patch.

Comment 39

[Robert Accettura [:raccettura]]

Scott,

In Bug 217808 we ran across an issue with a few emoticons not showing up (see
attachment screenshot), patch shall come shortly.

Most imporantly, we have removed some case sensitivity and other tweaks to be
like other clients that do emoticons.  For example:

:-b = :-P
:-/ :-\

etc.

Shouldn't this patch also include such enhancements?

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 143437 [details] [diff] [review]
updated patch addressing Ben's review comments

a=asa (on behalf of drivers) for checkin to 1.7 Beta

Comment 41

[Scott MacGregor]

Ok I checked in the patch for seamonkey. BenB and I can talk about some of the
good ideas he had going forward as well. Enjoy theme authors!

Comment 42

[Alfred Kayser]

Attachment: .css, .xml and .png files for the image-set trick.

This zipfile contains the files for implementing the smileys in a theme 
using an imagemap (instead of several different image files).
Replaced 16 images files with 1 image file and 1 binding file.

The trick is to bind the smiley span to contain an image, so that
normal list-style-image with -moz-image-region can be used.

Comment 43

[Alfred Kayser]

Attachment: Fixed some bugs in my starter kit. It now works!

Changes:
* moved to /communicator/skin, as messengerComposer expects it there
* added -moz-select-all
* moved the 'display: none/inline' to the binding, saves some style rules.

Comment 44

[Markus Multrus]

Attachment: image of cut-off emoticons

Comment 45

[Markus Multrus]

Actually I'm seeing cut-off emoticons all over (see my attachment). Was this
caused by this bugfix?
I'm using 1.7b Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316

Comment 46

[R.K.Aa.]

to comment 45: No - that's bug 150396

Comment 47

[[e-mail]]

(In reply to comment #29)
>A trick for themers: instead of using a 'background-image' to image the span,
>it could be possible to rebind the span.moz-smiley-* as an image or xul thing
>with an image inside.
And this should also solve any remaining cut-offs problems too :-)

Comment 48

[Ben Bucksch (:BenB)]

*** Bug 59094 has been marked as a duplicate of this bug. ***