551468 - Stop word-wrapping comments on the server

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

There are various issues caused by the fact that we wrap comments on the server instead of in the browser (see the bugs marked as depending on this bug).

  Now that modern browsers all seem to support client-side wrapping of preformatted text, we should just be able to wrap text on the client side and skip server-side wrapping. The only thing that I have to confirm is that client-side wrapping really does work properly in all modern browsers that are in common use.

  The comment box itself still stay at exactly 80 columns, though, because comments will still be wrapped at that width in emails.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go! Pretty straightforward. I also centralized all of our pre-wrap CSS, while I was at it, so that we don't have to duplicate all of that stuff.

Comment 2

[Frédéric Buclin]

Comment on attachment 431642 [details] [diff] [review]
v1

You forgot to fix bug/comments.html.tmpl:

 comment.body_full({ wrap => 1 })

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Ah, right, thanks. :-)

Comment 4

[Frédéric Buclin]

Comment on attachment 431645 [details] [diff] [review]
v2

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>-  [%- comment.body_full({ wrap => 1 }) FILTER quoteUrls(bug, comment) -%]
>+  [%- comment.body_full FILTER quoteUrls(bug, comment) -%]

Hum, I just realized that quoted replies in comments are now on a single line, no matter how long the quoted text is (also affects bugmail).

Comment 5

[Max Kanat-Alexander]

 Woow, that is going to be annoying to fix. The only complete solution that I can think of off the top of my head, at the moment, is to word-wrap the comment in JS, which means that we'd need some sort of word-wrapping library. Maybe there's something in YUI.

Comment 6

[Max Kanat-Alexander]

  Also, any word-wrapping we did in JS would ideally understand how to wrap CJK text, or people would be getting quoted replies with the wrong line breaks (which is important in CJK languages).

Comment 7

[Max Kanat-Alexander]

Attachment: v3

Wow, that was pretty tough. I had to write a word-wrapping algorithm in JavaScript! Thankfully, I had some help from a guy in the #yui channel, who actually wrote a whole first draft of such a function for me. (He's credited in this patch in comments.js.)

The word-wrapping algorithm that I eventually decided on is somewhat naive, but I think it's just right for wrapping replies.

Comment 8

[:glob ✱]

Comment on attachment 451496 [details] [diff] [review]
v3

r+

this looks good, except you should move punctRe creation out of the loop/function.

Comment 9

[Max Kanat-Alexander]

  Have I told you lately that you're awesome, glob? :-D Thank you so much for the review. :-)

Comment 10

[Max Kanat-Alexander]

Added to the release notes in bug 604256.

Comment 11

[Frédéric Buclin]

Something is wrong with this bug. The patch v3 has never been committed. If this bug has been fixed by another checkin, then it shouldn't have a+, and a comment should clarify this.

Comment 12

[Frédéric Buclin]

Also, I wouldn't want to see this bug committed for 4.0rc1, because this would give us no time to track regressions before we release.

Comment 13

[Gervase Markham [:gerv]]

Can someone explain exactly what the user-visible impact of this patch is going to be, for people with and without JS enabled?

Gerv

Comment 14

[Max Kanat-Alexander]

Wow, this never got committed?? How did that happen, I wonder. In any case, I do agree with LpSolit, rc1 is too late to check it in. We'll have to remove it from the 4.0 release notes and put it into 4.2.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #13)
> Can someone explain exactly what the user-visible impact of this patch is going
> to be, for people with and without JS enabled?

  Comments will be wrapped with CSS instead of on the server side. There will be no difference with JS enabled or disabled. The JS part of the patch has to do with the "reply" link, which is JS to start with.

Comment 16

[Max Kanat-Alexander]

There was some minor bitrot that I fixed on checkin.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Constants.pm
modified js/comments.js
modified skins/standard/IE-fixes.css
modified skins/standard/attachment.css
modified skins/standard/global.css
modified template/en/default/attachment/midair.html.tmpl
modified template/en/default/bug/comments.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/process/midair.html.tmpl
modified template/en/default/global/header.html.tmpl
modified template/en/default/pages/linked.html.tmpl
Committed revision 7574.

Comment 17

[Max Kanat-Alexander]

Attachment: Checked-In Version

Comment 18

[Max Kanat-Alexander]

Attachment: Checked-In Version

Previous "checked in version" attachment was "cdiff" output instead of a correct diff.