953653 - URLs should not contain Smileys

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[Instantbot]

*** Original post on bio 207 by DetroitLibertyPenguin <fulner AT gmail.com> at 2009-08-04 21:08:00 UTC ***

a statement starting with http:// is automatically identified as a URL and allows you to click on the hyper link

however this should be known to be a URL and as such not contain smileys within it 

example http://lxde.org/wiki/Planet_LXDE currently shows XD as a smiley within the URL

Comment 1

[Romain BEZUT]

*** Original post on bio 207 at 2009-08-05 14:55:03 UTC ***

I had encountered this problem several times too :).
Thanks for your report.

Comment 2

[Instantbot]

*** Original post on bio 207 by DetroitLibertyPenguin <fulner AT gmail.com> at 2009-08-19 16:35:09 UTC ***

I do spend quite a bit of time in #lxde so this is going to bother me more than it is most folks, as such would it be possible to just de-activate that one smiley XD?

Comment 3

[Florian Quèze [:florian]]

Attachment: Somewhat related work-in-progress

*** Original post on bio 207 as attmnt 255 at 2009-12-11 15:51:00 UTC ***

I'm attaching a patch that I have that prevents matching an emoticon if there's an alpha-numeric character just after it. This would fix the specific example given in this bug.
I'm not sure we want to take this though because it would also prevent matching the first emoticon is someone writes XDXD. I don't know if that case matters.

And clearly, this patch doesn't fix fully the case of emoticons in URLs, an URL like http://foo.com/XD/bar would still have an emoticon in it.
I think we still need to disable emoticon replacement in URLs, but the attached patch may be useful for cases like "first:Place [blahblahblah]" that would currently match a :P emoticon.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: Untested (in Instantbird) patch for fix

*** Original post on bio 207 as attmnt 316 at 2010-07-20 15:28:00 UTC ***

A "working" patch that hasn't been tried in Instantbird. All testing was done in HTML/JS files with the following test cases (all of which pass):
surrounding http://lxde.org/wiki/Planet_LXDE text
surrounding http://foo.com/XD/bar test
surrounding XD:) text
surrounding :) text
surrounding XD text XDXD
aaa teXDst text
http://foo.com/fr/bar :)
XD test
o_o

Testing was done using the string version of the function, however, not the text node version.

Please post more test cases if they need to be tried (I'm sure I missed some).

Note that this follows the current behavior of replacing a smiley in a string such as "teXDst".

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Small syntax error fix

*** Original post on bio 207 as attmnt 317 at 2010-07-21 02:07:00 UTC ***

Tested, fixed syntax error.  Works as expected.

There was some discussion about whether this is the correct way to go about this or not. But it should work in complex situations.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8352057 [details] [diff] [review]
Untested (in Instantbird) patch for fix

*** Original change on bio 207 attmnt 316 at 2010-07-21 02:07:11 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Benedikt Pfeifer]

*** Original post on bio 207 at 2010-07-31 10:45:48 UTC ***

(In reply to comment #6)
> Created an attachment (id=317) [details]
> Small syntax error fix
> 
> Tested, fixed syntax error.  Works as expected.
> 
> There was some discussion about whether this is the correct way to go about
> this or not. But it should work in complex situations.

If you think this is a valid solution and a working fix then you should request review for your patch from Florian.

Comment 9

[Patrick Cloke [:clokep]]

*** Original post on bio 207 at 2010-07-31 15:09:42 UTC ***

I actually have a slightly updated patch, which just uses some nicer syntax, and the patch here isn't really a valid patch, the paths are wrong. I'll put it up this week and request review on it.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Working fix

*** Original post on bio 207 as attmnt 324 at 2010-08-02 22:54:00 UTC ***

Working fix with a proper patch path

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8352058 [details] [diff] [review]
Small syntax error fix

*** Original change on bio 207 attmnt 317 at 2010-08-02 22:54:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

*** Original post on bio 207 at 2010-10-05 17:12:04 UTC ***

The mozITXTToHTMLConv routines can handle doing URLs and "Glyphs" together, (and I assume this won't cause a Glpyh to occur in a URL -- I've never seen on in Thunderbird). But it cannot handle custom Glyphs.

I'm wondering if it'd make more sense to expand mozITXTToHTMLConv to handle custom glyphs, this is covered in https://bugzilla.mozilla.org/show_bug.cgi?id=23327 (Support custom glyph substitutions in mozTXTToHTMLConv) (https://bugzilla.mozilla.org/show_bug.cgi?id=116842 may also be of interest: "mozTXTToHTMLConv: Tracking and testcases").

We could then rewrap these in the textModifiers system if we wanted to (or just choose which parameters (http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/public/mozITXTToHTMLConv.idl#62) to use based on preferences (i.e. on http://lxr.instantbird.org/instantbird/source/instantbird/content/convbrowser.xml#294 -- which is kind of confusing just having a "2" as that parameter right now)).

Sorry if this doesn't fully make sense, its kind of a vague idea in my mind right now.

Comment 13

[Patrick Cloke [:clokep]]

*** Original post on bio 207 at 2010-10-05 17:15:34 UTC ***

Forgot to include a link to mozITXTToHTMLConv documentation: http://www.bucksch.org/1/projects/mozilla/16507/

Comment 14

[Benedikt Pfeifer]

*** Original post on bio 207 at 2010-10-06 12:34:36 UTC ***

(In reply to comment #10)
> I'm wondering if it'd make more sense to expand mozITXTToHTMLConv to handle
> custom glyphs, this is covered in
> https://bugzilla.mozilla.org/show_bug.cgi?id=23327 (Support custom glyph
> substitutions in mozTXTToHTMLConv)
> (https://bugzilla.mozilla.org/show_bug.cgi?id=116842 may also be of interest:
> "mozTXTToHTMLConv: Tracking and testcases").

From my current point of view I'd say we have a nice extensible system while they have .. something. In this sense it'd rather replace their part than Instantbird's if I had to choose.

> We could then rewrap these in the textModifiers system if we wanted to

Wrapping the call to their code in a textModifier sounds like a good idea. It would remove this special case of changing content of a text node with something that isn't a text modifier.

>(or just choose which parameters
> (http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/public/mozITXTToHTMLConv.idl#62)
> to use based on preferences (i.e. on
> http://lxr.instantbird.org/instantbird/source/instantbird/content/convbrowser.xml#294
> -- which is kind of confusing just having a "2" as that parameter right now)).

Using the name of the constant could help here.

Comment 15

[Florian Quèze [:florian]]

*** Original post on bio 207 at 2010-10-08 16:13:59 UTC ***

(In reply to comment #10)

I agree that fixing the Mozilla code to replace emoticons and using it looks like the right way to go but... I think you really don't want to touch this code. It's in C++, it's hard to understand (even the XPCOM interface around it is confusing), ...

As Mic wrote, our current system is extensible, the mozITXTToHTMLConv thing is not.

I don't think the linkification should be inside a text modifier.
Let's suppose you have this input: test<a href="about:blank">http://foo.bar</a>test
http://foo.bar is a text node, it will be processed by text modifiers and we will create a link inside a link...

Also, if we ever rewrite the linkifying code in JS to make it more suitable for our use case, I'll be very happy to get rid fo the .replace(/&/g, "&amp;") / .replace(/&amp;/g, "&") hack we currently need to use scanHTML. :)

Comment 16

[Patrick Cloke [:clokep]]

Attachment: Fix checking if a node is

*** Original post on bio 207 as attmnt 364 at 2010-10-08 20:06:00 UTC ***

Fixed via Flo's suggestion to check if the parent node is a link and then compare the inner content to the href. Works in all my tests.

Comment 17

[Patrick Cloke [:clokep]]

Comment on attachment 8352065 [details] [diff] [review]
Working fix

*** Original change on bio 207 attmnt 324 at 2010-10-08 20:06:44 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8352106 [details] [diff] [review]
Fix checking if a node is

*** Original change on bio 207 attmnt 364 at 2010-10-08 20:37:23 UTC ***

>diff --git a/instantbird/modules/imSmileys.jsm b/instantbird/modules/imSmileys.jsm

>+  if (parentNode.tagName == "A"
I only have a limited confidence in this test.
Testing parentNode instanceof Components.interfaces.nsIDOMHTMLAnchorElement seems safer.

>+      && parentNode.getAttribute("href") == aNode.data)
If we are sure we have an nsIDOMHTMLAnchorElement instance, we can use .href instead of .getAttribute

Depending of how you want to handle the case of multiple text nodes inside the anchor element, you may want to use parentNode.textContent instead of aNode.data.

Otherwise, I really like how simple this solution is. :)

Comment 19

[Patrick Cloke [:clokep]]

Attachment: Version 0.2

*** Original post on bio 207 as attmnt 367 at 2010-10-10 15:42:00 UTC ***

(In reply to comment #15)
> (From update of attachment 8352106 [details] [diff] [review] (bio-attmnt 364) [details])
> >+      && parentNode.getAttribute("href") == aNode.data)
> If we are sure we have an nsIDOMHTMLAnchorElement instance, we can use .href
> instead of .getAttribute
As mentioned on IRC using .href doesn't work, it seems to do some auto-formatting/encoding for that which won't exactly match the text.
> 
> Depending of how you want to handle the case of multiple text nodes inside the
> anchor element, you may want to use parentNode.textContent instead of
> aNode.data.
This also doesn't seem to work and I'm not too worried about multiple text nodes in an anchor element since it seems like libpurple has trouble even sending those. I think that'd be a follow up bug, but this patch covers the most common situations.

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 8352106 [details] [diff] [review]
Fix checking if a node is

*** Original change on bio 207 attmnt 364 at 2010-10-10 15:42:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 21

[Patrick Cloke [:clokep]]

Attachment: Version 0.3

*** Original post on bio 207 as attmnt 368 at 2010-10-10 17:14:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 22

[Patrick Cloke [:clokep]]

Comment on attachment 8352109 [details] [diff] [review]
Version 0.2

*** Original change on bio 207 attmnt 367 at 2010-10-10 17:14:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 23

[Patrick Cloke [:clokep]]

*** Original post on bio 207 at 2010-10-13 14:53:26 UTC ***

Mic asked for some test cases, these are ones I remember using:
http://google.com
http://google.com/
www.google.com
http://goo^^le.com
http://google.com/XD
/raw <a href="http://google.com/XD"><b>http://google.com/XD</b></a>
/raw <a href="http://google.com/XD">http://google.com/XD <b>BOLD</b></a>

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8352110 [details] [diff] [review]
Version 0.3

*** Original change on bio 207 attmnt 368 at 2010-10-13 15:24:32 UTC ***

r=me. This looks great, and I've tested it without finding any issue. I'm glad we finally have a simple fix for this problem :). Thanks for working on this.

Comment 25

[Florian Quèze [:florian]]

*** Original post on bio 207 at 2010-10-13 15:47:51 UTC ***

Fixed! (Pushed as https://hg.instantbird.org/instantbird/rev/a078133a0492)