335344 - Remove leading and trailing space character from emoticon text when using graphical smilies

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Cédric "chewey" Menge]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060404 SeaMonkey/1.0.1 Mnenhy/0.7.3.10006
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060404 SeaMonkey/1.0.1 Mnenhy/0.7.3.10006

The emoticon-building characters are placed inside a hidden <span> element when using graphical smilies. Inside this element, the smily characters become enclosed by additional whitespace characters not present in the messages' original text. 

This is an unnecessary alteration, and can even lead to unwanted behaviour if the original text smilies are re-activated via userContenc.css while leaving general "glyph replacement" activated (that's how I discovered this).

Reproducible: Always



Expected Results:  
Do not alter the smilies' text in any way.

Comment 1

[Cédric "chewey" Menge]

Attachment: removes enclosing whitespaces and adds a "title" attribute to the enclosing span, trying to resolve bug 234638

Comment 2

[Karsten Düsterloh]

Actually, your patch here won't solve bug 234638, but it's needed to fix that one (I have a patch for that)... :-)

Comment 3

[Cédric "chewey" Menge]

(In reply to comment #2)
> Actually, your patch here won't solve bug 234638,

I know - that's why I said "trying :-)

> but it's needed to fix that one (I have a patch for that)... :-)

Great.
 

Comment 4

[Cédric "chewey" Menge]

Attachment: same patch, improved file format

Comment 5

[Ben Bucksch (:BenB)]

Comment on attachment 219829 [details] [diff] [review]
same patch, improved file format

Patch is still messed up.

You're only removing 2 spaces and adding title="tagTXT", right? (Or trying to...)

Please check for side effects, the space may have been intentional as workaround for Gecko wrapping strangeness.

Comment 6

[Karsten Düsterloh]

> Patch is still messed up.

First "+" line missing, it seems.

> You're only removing 2 spaces and adding title="tagTXT", right?

Adding the tagTXT as a title.

> Please check for side effects, the space may have been intentional as
> workaround for Gecko wrapping strangeness.
 
I don't see any side effects. Usually, we recognise only smilies surrounded by spaces, and those are kept - what good should spaces do here that are not needed in real HTML? These spaces are even messing up textual copies of smilies!
I really think they're just (misplaced) there for "beauty", because they were believed not to be seen at all. Or do you have any special edge cases in mind?

Comment 7

[Cédric "chewey" Menge]

Attachment: fixed patch (been stupid)

Yikes! Yes, my bad - the missing line was the result of a c&p mishap while editing the source file. *Now* it only removes two whitespaces around the smily and adds the title attribute in preparation for bug 234638. :-)

I tested this patch thoroughly with both plain text and HTML messages, using manual line wrap and letting mailnews doing the wrapping, and couldn't find any peculiarity in wrapping behaviour - or even any difference at all.

Comment 8

[Ben Bucksch (:BenB)]

Comment on attachment 219946 [details] [diff] [review]
fixed patch (been stupid)

I wonder why the additional <span> is added there, too, so I think we're missing something.

Nevertheless, trusting your claim to have tested it carefully. r=BenB

Comment 9

[Ben Bucksch (:BenB)]

(P.S. please keep the comment
// e.g. smiley-frown
instead of "smiley image" - it's important to make sense of the code and the related CSS.)

Comment 10

[Cédric "chewey" Menge]

(In reply to comment #9)
> (P.S. please keep the comment
> // e.g. smiley-frown
> instead of "smiley image" - it's important to make sense of the code and the
> related CSS.)

Oh, sorry, I wasn't aware of that - this being my very first patch submission on Bugzilla, I'm not sure how to do this with a patch already bearing an r+ flag.

Resubmit a new patch anyway?

Comment 11

[Karsten Düsterloh]

> Resubmit a new patch anyway?

Not necessarily, a small change like that can be done on check-in. Ask for sr, and only if the sr wants other changes as well provide a new patch.

Comment 12

[Cédric "chewey" Menge]

Attachment: same patch again - comment fixed

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp 	1.82

Comment 14

[Karsten Düsterloh]

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