66213 - editor change for smiley feature

Status: VERIFIED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Anatoliy Averbukh]

new menu to represent and insert smileys

Comment 1

[Anatoliy Averbukh]

Attachment: editor fix for smiley menu

Comment 2

[Kathleen :Brade]

This looks fine to me except I'm not sure I agree with the spaces inserted (in 
editorOverlay.xul).  Seems to me that we should either insert spaces before and 
after or we shouldn't insert any spaces.

AIM 3.5.966 on Macintosh doesn't insert any spaces.

Comment 3

[Syd Logan]

AIM windows inserts a trailing space

Comment 4

[Anatoliy Averbukh]

Attachment: overide old attachment

Comment 5

[Simon Fraser [no longer active]]

Comments on the patch:
1. The smiley commands are not using the editor command handling mechanism. They 
won't be enabled and disabled correctly (e.g. during document loading).
2. Need better names for the smileys. :( is "Smile6" for example. It should be
  "Short frown" or something.
3. Why do the smileys insert a space before them? They should be smart about 
this, depending on whether there is already a space before the insertion point.

A more worrysome aspect of this patch is that it introduces a dependency between 
the editor and mailnews XUL files, since the EditorToolbars.css file contains 
paths into the messenger jar. I think we should try to avoid this.

Comment 6

[Akkana Peck]

The patch doesn't apply for me: I get "**** malformed patch at line 96: Index:
mozilla/themes/classic/editor/EditorToolbars.css".

Comment 7

[Syd Logan]

akk, if you are going from a windows patch to Linux, it almost never works. is
that the issue? 

sfraser: could you explain how to correct the editor command handling mechanism
issue? anatoliy is new, and could use the help.

sfraser: we were told by UE to get this into the editor toolbar. maybe smilies
aren't used by all apps, but seems to me that with irc, chat, messenger, and im
already applications that use smilies and use composer, it doesn't seem
unreasonable that it be made available. I think even Word has support for
smilies in one way or another, surely if 3 our of 4 of the NS 6 apps have a need
for it, you guys should take a serious look at providing it as opposed to having
each of us roll it ourselves...

Comment 8

[timeless]

if that's the case, let's move the smilies into toolkit or comm. or you could 
create a jar just for them...

Comment 9

[Simon Fraser [no longer active]]

Re: editor command handling mechanism. You should write your smiley insert as a 
command, and register that command with the composer command handler. See 
ComposerCommands.js for examples, and read <http://www.mozilla.org/editor/
editor_commands.html> to understand how this works. Your comamnd's
isCommandEnabled() method should figure out when the command should be enabled 
(probably 'return (window.editorShell && window.editorShell.documentEditable);').
Its doCommand() method calls editorShell.insertText() with the appropriate smiley 
string. To get the appropriate string, you can either use something like the 
stateful command mechanism that stashes the smiley string on the command node, 
and has the command retrieve it from there, or can implement a separate command 
for each type of smiley. In future, commands will be able to take parameters, so 
this will become easier.

As for where to put this stuff:
We certainly don't want to add a dependency between editor and messenger jar 
files. Neither do we want this feature exposed in the composer UI. I suggest that 
you add the GIFs to the communicator jar. It's also odd that you've added CSS to 
EditorToolbars.css, but the buttons that are styled are not in editor XUL. It 
would be perferable to find a better home for that CSS.

Comment 10

[Anatoliy Averbukh]

Attachment: update in according to Simon's notes

Comment 11

[Anatoliy Averbukh]

update includes:
- editor command handling mechanism,
- new location for CSS and images,
- "smart processing" for insertion

Comment 12

[Syd Logan]

Simon, could you please review Anatoliya's patch? Thanks.

Comment 13

[Simon Fraser [no longer active]]

Patch looks much better. My only qualms are with the call to
editorShell.GetContentsAs("text/plain", 8);
which is done to find out if there is a space at the end of the text. 
editorShell.GetContentsAs() is a very expensive call for long document, and 
calling it could produce noticable delay when the user chooses this command.
Also, you're trying to find if there is a space before hte insertion point, but 
your code only looks for a space at the end of the entire document. What if the 
insertion point isn't at the end?

ccing: akkana and jfrancis for ideas here. Talking with Joe, it seems hard to get 
the char before the insertion point (what if you're just after a table?). There 
is some code that could be refactored to ask "is previous char a space".

Comment 14

[Anatoliy Averbukh]

Attachment: editor smiley insertion

Comment 15

[Anatoliy Averbukh]

I added several features providing "smart" behavour for smiley insertion for any 
document position (not just for end of document). 
As a "side effect", editorShell.GetContentsAs call was eliminated,
also, there is no need to look for space, before the insertion point now.

Comment 16

[Simon Fraser [no longer active]]

Phew, big patch  ;)
OK, we're getting closer. I do have some comments on the selection code tho.

In a number of cases you bail with an alert. I think showing an alert here is way 
to "in your face" for the user; just fail silently. Or, if you really want to do 
things right, test for these situations in the 'isCommandEnabled', and return 
false if you're in a state where smiley insertion is not permitted. This code 
should not be too expensive to call from isCommandEnabled.

Why disallow inserting smileys when the selection is not collapsed? It seems 
reasonable to allow the user to select text, and replace it with a smiley.

You also check !selection.isCollapsed before checking if you have a selection.

The stuff to get the next/previous char is cool, but the last part that crawls up 
the content tree seems a little excessive, and it's not clear what it's trying to 
do. Does it just ensure that it will add a right space if you'are at the very end 
of the doc? I'd be inclined to just remove that code.

Minor nit: there is some inconsistent spacing in the file.

Comment 17

[Anatoliy Averbukh]

Attachment: editor smiley

Comment 18

[Anatoliy Averbukh]

New update includes:
- alerts are deleted;
- error handling was moved to isCommandEnabled
- remove code for last char finding 9I agree it is expensive)
- adjusted spacing

I left "collapsed condition" 
(without that the procedure can become too complex)

Comment 19

[Simon Fraser [no longer active]]

That looks good. Just one more small thing! Your command's enabled state depends 
on whether the selection is collapsed on not, yet your command won't be udpated 
every time the selection changes. Only commands under a commandset whose events 
include "style" get updated in this way. But I don't think you want to add your 
command there, because we want to try to avoid the overhead of updating the 
command every time the selection moves, and, since you have no button for this, 
it doesn't matter. You just need to ensure that the command gets updated before 
the user sees the menu.

So your <menupopup> needs an 'oncreate' handler that updates the state of
the command. That oncreate handler should call goUpdateCommand("cmd_smiley");
With this in place, you shoudl check that the menu items are appropriately 
disabled when the selection is non-collapsed.

I still question why you are only allowing smiley insertion when the selection is 
collapsed? Can you respond to that?

Comment 20

[Anatoliy Averbukh]

Attachment: - insertion of smiley images

Comment 21

[Anatoliy Averbukh]

update:

- insertion of smiley images (not codes) to composer area
(works for any selection);
- smiley menu disable, when smiley images are disabled from prefs

Comment 22

[Simon Fraser [no longer active]]

OK, um, so if you insert images now, where (if anywhere) do they get converted to 
strings when the message is sent? If they don't, can you be sure that the image 
URLs are valid on the receiving end? And what if I insert a smiley into a mail 
destined for a 4.x user?

Comment 23

[Anatoliy Averbukh]

Sure, I am doing conversion images to text, but AFTER user pushes Send button.
(the user cannot edit message after that, of course). No images are sent to 
remotely.
The same can be done for mail.

Comment 24

[german]

cc'ing to inform scott from mail team

Comment 25

[Simon Fraser [no longer active]]

But who says that the composer content will be sent by mail? The code is in 
composer, so you can't guarantee that it won't be invoked by a user writing a web 
page or generating content that is output by some other system that you don't 
know about. I believe that I brought up this problem during the original 
discussion of this (IMHO somewhat ill conceived) feature.

Comment 26

[Simon Fraser [no longer active]]

So, thinking a little more, I think the solution to this is to use CSS to somehow 
style a span such that the content contains the smiley text, but the user sees 
the image. I'm not quite sure how to best do this: ccing cmanske for input.

Comment 27

[Anatoliy Averbukh]

Simon,
1. My decision is working fine for my applications.
2. As to mail:
   a) Currently there is no Smiley menu at mail composer menu and on composer, 
      my code can be invoked JUST from Smiley menu;
   b) if email will be sent, the same approach (text convertsion) can be used;
   c) if it is NOT for remote user (like email), let's say for web page 
      composer, currently there is a feature "insert image". Everybody can 
      insert ANY image. Smiley feature works exactly like that, except, just 
      smiley images can be inserted. That is no reason to insert text instead of 
      image here (with span, for instance)

Comment 28

[Anatoliy Averbukh]

One more thing: 
I need to convert <span> element the same way like image element, before I send 
it remotely, so I get no advantage from that

Comment 29

[Simon Fraser [no longer active]]

OK, so let's go with attachment 24243 [details] [diff] [review] (02/02/01) for now. sr=sfraser on that 
patch.

Comment 30

[Ben Bucksch (:BenB)]

IMO, this is a bad idea for Mozilla Mailnews. More later (hopefully).

Comment 31

[Anatoliy Averbukh]

fixed

Comment 32

[sujay]

Anatoliya, please verify this bug and mark VERIFIED-FIXED...thanks!

Comment 33

[Anatoliy Averbukh]

Verified.