955303 - Leading whitespace is lost from messages

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[aleth [:aleth]]

*** Original post on bio 1870 at 2013-02-02 16:51:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1870 as attmnt 2219 at 2013-02-02 16:51:00 UTC ***

Any leading whitespace in messages is lost during the DOM parser call in http://lxr.instantbird.org/instantbird/source/chat/modules/imContentSink.jsm#335.

To avoid this, we can wrap the text to be parsed in a <span> (thanks clokep!)

As an alternative to the use of .firstChild in the patch, we could look for the first span node, but this seems clearer.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 1870 at 2013-02-02 16:59:35 UTC ***

Nice idea (and btw, the   idea wasn't usable because replacing spaces with unbreakable spaces in code makes it unparsable (in JS at least) in a way that is difficult for users to understand).

r- because:
- This definitely needs a comment explaining why we are adding that span node
- Also, I think it would be nicer to import https://bugzilla.mozilla.org/show_bug.cgi?id=830456 first (I think these patches bitrot each other).

Thanks for looking at this annoying issue anyway! :)

Comment 3

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1870 as attmnt 2236 at 2013-02-18 12:18:00 UTC ***

The patch blocking this has now landed, so here is a new patch with an added comment.

Comment 4

[aleth [:aleth]]

Comment on attachment 8353982 [details] [diff] [review]
Patch

*** Original change on bio 1870 attmnt 2219 at 2013-02-18 12:18:40 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 1870 at 2013-02-18 13:07:23 UTC ***

Drive-by review: would it make more sense to use a div instead of a span? (My reasoning is simply that div is a block level element, span is an inline element.)

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8353999 [details] [diff] [review]
Patch

*** Original change on bio 1870 attmnt 2236 at 2013-02-18 23:11:50 UTC ***

Thanks!!! :-)

Comment 7

[Benedikt Pfeifer]

*** Original post on bio 1870 at 2013-02-18 23:26:46 UTC ***

Comment on attachment 8353999 [details] [diff] [review] (bio-attmnt 2236)
Patch

>+  let div = doc.querySelector("span");

I think the variable name should be changed while you're at it.

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 1870 at 2013-02-19 00:01:35 UTC ***

(In reply to comment #5)
> Comment on attachment 8353999 [details] [diff] [review] (bio-attmnt 2236) [details]
> Patch
> 
> >+  let div = doc.querySelector("span");
> 
> I think the variable name should be changed while you're at it.

Good suggestion!
I renamed the variable from div to span before pushing:
http://hg.instantbird.org/instantbird/rev/dcd23e84ee7a

Comment 9

[aleth [:aleth]]

*** Original post on bio 1870 at 2013-02-19 10:27:12 UTC ***

(In reply to comment #5)
> >+  let div = doc.querySelector("span");
> 
> I think the variable name should be changed while you're at it.

:) (Either that or we could have gone with comment #3 and used a div ;) )