68480 - toolbar menubuttons in msgcompose need cleanup

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Joe Hewitt (gone)]

The menubuttons on the right side of the formatting toolbar in mail compose
(align, insert object, and smileys) need some tender loving care.  There are a
number of anoying problems with them that are fairly easy to remedy. I could
probably file a dozen bugs on this subject alone, but I'll just file one and
list the problems.  I've done the work of fixing this already, so my patch will
follow shortly.

The specific problems are:

* in the xul, these are currently menus with buttons inside of them.  They
should just be menubuttons.
* in the xul, the menuitems have buttons inside of them.  The buttons are not
necessary, these should just be menuitems.
* in classic, hover and active feedback is out of sync with the theme with
regard to icons and borders
* none of these menubuttons become disabled when the other buttons on the
toolbar become disabled
* there are no theme-matching smiley icons for modern and classic. 

[e-mail] seems to be the one who did the work on the smiley stuff,
but he's not a registered bugzilla user yet so I'll pass this bug on to him
directly.

Comment 1

[Joe Hewitt (gone)]

Attachment: patch to fix

Comment 2

[Joe Hewitt (gone)]

Attachment: newimages.zip (unzip this to your themes directory)

Comment 3

[Joe Hewitt (gone)]

Some comments regarding the patch:

* in ComposerCommands.js I removed some code that was causing the smiley button
to never become enabled.  Previously, this command was never even observed so
the problem was hidden.  This code checked to see if the selection is collapsed
and the current editing node is text.  Why this is criteria for inserting a
smiley, I have no idea.  All I know is it works fine with that gone.

* in editorOverlay.xul I removed the oncommand handler for the commands
cmd_align and cmd_smiley.  This is because these are stateful commands and
observers of this command should not call goDoCommand.  The menuitems already
make a call to doStatefulCommand, and having oncommand on the broadcaster which
the menubutton observes only causes the command to be fired twice.

Reviews, anyone?

Comment 4

[Blake Ross]

He's [e-mail].  cc'd

Comment 5

[Anatoliy Averbukh]

Hi Joe,

Thank you for your comments and proposed patch.
However it is in according with specifications.
Your patch cannot implement suitable functionality for ALL applications for 
this smiley menu (it wasn't primary for mailnews).
Way like you insert images isNOT suitable for my applications (smiley codes 
will be inserted incorrectly with your patch, that is a reason for collapsed 
and other exceptions).

Also Smiley images areNOT a part of skin (they are the same like anchor image, 
for instance, for internal needs).

there are and other incompatabilities.

Currently I am doing my patch 67981 and so I propose mark this patch is an a  
dependent of my patch.

Thank you.

Comment 6

[Håkan Waara]

Anatoliy, if something is critical for your application and will break if the
other moz developers change something then you should implement it on your own
tree only. As an addon.

Comment 7

[Joe Hewitt (gone)]

Based on Anatoliy's comments I will remove from my patch the stuff affecting
updating of the smiley command.  However, the rest of the stuff is still valid. 

Comment 8

[Joe Hewitt (gone)]

The revised patch has the following changes:

* removed command updating for smiley button pending bug 67981
* moved css for smiley images out of skin and into content

Comment 9

[Joe Hewitt (gone)]

Attachment: patch, rev2

Comment 10

[Jean-Francois Ducarroz]

Looks good for me for the message compose part, R=ducarroz. You still need a review from editor team (cmanske).

Comment 11

[Charles Manske]

r=cmanske

Comment 12

[Joe Hewitt (gone)]

Ick.  I just noticed some problems this patch is causing outside of msg compose.
 I'll have to revise it a bit... 

Comment 13

[Joe Hewitt (gone)]

I've updated the patch to fix a problem I discovered in the Classic theme
(toolbar bookmarks got an extra border) and to completely remove all changes
related to anatoliy's smiley changes.  This now only affects the align and
insert menubuttons.

The messiness comes into play with regard to my changes to the Modern theme. 
I'm about to checkin a major re-organization of Modern, and the fix for this bug
needs to go in simultaneously.  I'd rather do that than try to separate them and
check them in separately.  

The following patch incorporates the xul changes and the classic theme changes.
 Anyone who wants to see the modern theme changes, see bug 65745

Comment 14

[Joe Hewitt (gone)]

Attachment: patch, rev3

Comment 15

[Joe Hewitt (gone)]

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

Comment 16

[[e-mail]]

Attachment: Suggested smileys

Comment 17

[Joe Hewitt (gone)]

fixed

Comment 18

[Ninoschka Baca]

Build 2001-03-29-04: NT4, Mac 9.04
Build 2001-03-29-08: Linux RH 6.2
Reopening due to Classic problems with the Smiley drop down.

1. Modern: OK
2 Classic: 
  a. Icons not appearing for Link, Anchor, Image etc... (bug# 63624)
  b. Smiley drop down looks strange
     - Mac: blank lines at top and bottom
     - NT4 and Linux: up arrow at top, box appearing around each smiley item and        
       down arrow at bottom.

Comment 19

[Joe Hewitt (gone)]

1. the issue with icons in the insert/align buttons in classic is a separate bug
(bug# 63624)

2. anatoliya recently fixed the smiley thing (bugscape 4013)

Marking fixed again

Comment 20

[rubydoo123]

Is there a bug on the border that's around the word descriptions
for the smileys in the dropdown menu? Doesn't look very good.

Comment 21

[Anatoliy Averbukh]

I fixed, but didn't check-in yet. Will do that today.

Comment 22

[Joe Hewitt (gone)]

Beth, Anatoliya's fix for 4013 should resolve that problem. Don't know if he's
checked it in yet.

Comment 23

[Ninoschka Baca]

scalkins is going to log a bug on the Smiley dropdown menu not looking correct. 
In a build from a couple of days ago I saw the borders that beppe describes. In 
the 4/5 build on the Mac I don't see the borders but an empty area appears above 
and below the smileys. She will copy beppe, hewitt and anatoliy in the new bug. 

I'm verifying this bug because the majority of the issues are fixed. Separate 
bugs will be logged for additional problems (i.e. smiley drop down)

Comment 24

[Charles Manske]

Actually, that was cmanske, not beppe! I'm on her machine today and forgot to
log off bugzilla first!