Bug 37792 - bugs.webkit.org: should not hard-wrap comment lines exceeding 80 characters
Summary: bugs.webkit.org: should not hard-wrap comment lines exceeding 80 characters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-19 01:07 PDT by Chris Jerdonek
Modified: 2010-05-09 00:13 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (2.65 KB, patch)
2010-04-29 21:41 PDT, Chris Jerdonek
ddkilzer: review-
cjerdonek: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 2 (2.82 KB, patch)
2010-05-06 08:55 PDT, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 3 (4.54 KB, patch)
2010-05-06 19:09 PDT, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-04-19 01:07:01 PDT
This report stems from the following webkit-dev e-mail:

https://lists.webkit.org/pipermail/webkit-dev/2010-April/012528.html
Comment 1 Chris Jerdonek 2010-04-19 01:11:32 PDT
This is configured here:

http://trac.webkit.org/browser/trunk/BugsSite/Bugzilla/Constants.pm?rev=45520#L263
Comment 2 Chris Jerdonek 2010-04-19 01:33:59 PDT
Hmm, I guess it would be best if we can also make the box's wrapping behave dynamically based on browser window size, etc. (when reading comments).  Otherwise, increasing the limit to 120, say, will force all flowing comment text to be at least that wide.
Comment 3 Maciej Stachowiak 2010-04-19 01:54:49 PDT
(In reply to comment #2)
> Hmm, I guess it would be best if we can also make the box's wrapping behave
> dynamically based on browser window size, etc. (when reading comments). 
> Otherwise, increasing the limit to 120, say, will force all flowing comment
> text to be at least that wide.

My thought is that maybe the comments field can be just white-space: pre-wrap instead of white-space: pre, and therefore wrap to the window width. That way we shouldn't need hard line breaks at all.
Comment 4 Chris Jerdonek 2010-04-29 04:57:01 PDT
I didn't know until looking at the code that Bugzilla already doesn't wrap lines if they begin with ">":

http://trac.webkit.org/browser/trunk/BugsSite/Bugzilla/Util.pm?rev=45520#L367

For example,

123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789

>23456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 

Perhaps the fix can be there (simply always execute the "if" block).
Comment 5 Chris Jerdonek 2010-04-29 05:07:35 PDT
(In reply to comment #4)
> Perhaps the fix can be there (simply always execute the "if" block).

Ignore that last line.  I just thought it was interesting that Bugzilla doesn't wrap lines that begin with ">".  Maybe people already knew that.
Comment 6 Chris Jerdonek 2010-04-29 21:41:27 PDT
Created attachment 54781 [details]
Proposed patch

I'm not 100% sure that increasing the COMMENT_COLS constant won't affect other forms on the site that I might be less familiar with.  Nevertheless, I wanted to get a candidate patch out there.
Comment 7 Chris Jerdonek 2010-04-29 21:51:51 PDT
(In reply to comment #3)
> My thought is that maybe the comments field can be just white-space: pre-wrap
> instead of white-space: pre, and therefore wrap to the window width. That way
> we shouldn't need hard line breaks at all.

By the way, note that the comments field is already "white-space: pre-wrap":

.bz_comment_text, .uneditable_textarea {
     font-family: monospace;
    /* Note that these must all be on separate lines or they stop
       working in Konqueror. */
     white-space: pre-wrap;      /* CSS 3 & 2.1 */
     white-space: -moz-pre-wrap; /* Gecko */
     white-space: -pre-wrap;     /* Opera 4-6 */
     white-space: -o-pre-wrap;   /* Opera 7 */
}

It seems to be the width styling and the quoted lines' "white-space: pre"--

span.quote {
    color: #65379c;
    /* Make quoted text not wrap. */
    white-space: pre;
}

that are both preventing the pre-wrap style from taking effect:
Comment 8 Chris Jerdonek 2010-05-03 14:32:08 PDT
Any takers?  I'm assuming this change will go live when it lands.  Assuming that, I will be online when it lands to check for any unusual or undesired behavior, etc.
Comment 9 David Kilzer (:ddkilzer) 2010-05-04 11:39:05 PDT
Comment on attachment 54781 [details]
Proposed patch

> diff --git a/BugsSite/Bugzilla/Constants.pm b/BugsSite/Bugzilla/Constants.pm
>  # The column length for displayed (and wrapped) bug comments.
> -use constant COMMENT_COLS => 80;
> +#
> +# We choose a large value to prevent Text::Wrap::wrap() from inserting
> +# hard line breaks.  This allows us to use "white-space: pre-wrap"
> +# in the CSS for line-wrapping instead.
> +use constant COMMENT_COLS => 160;

Isn't this going to cause funny wrapping if someone types more than 160 characters in a single "paragraph"?  Is there a way to disable the wrapping altogether (e.g., setting COMMENT_COLS to 0 or -1 or modifying code elsewhere)?

> diff --git a/BugsSite/skins/standard/global.css b/BugsSite/skins/standard/global.css
> index aca0ab5..1ef62e9 100644
> --- a/BugsSite/skins/standard/global.css
> +++ b/BugsSite/skins/standard/global.css
> @@ -260,7 +260,8 @@ div#docslinks {
>  }
>  
>  .bz_comment_text {
> -     width: 50em;
> +    /* Do not specify a width.  Otherwise, comment lines will "pre-wrap"
> +       to the specified width rather than to the window width. */
>  }
>  
>  .bz_first_comment {
> @@ -275,6 +276,14 @@ div#docslinks {
>      padding: 1em 0;
>  }
>  
> +.bz_comment_text > span.quote {
> +    /* Prevent "white-space: pre" from styling quoted lines inside comments.
> +       Otherwise, a single quoted line that extends beyond the window
> +       width will cause all non-quoted long lines to extend beyond the
> +       window width as well. */
> +    white-space: inherit;
> +}
> +
>  span.quote {
>      color: #65379c;
>      /* Make quoted text not wrap. */

All of these changes need to be made to skins/custom/global.css, not skins/standard/global.css, since the custom version overrides the standard version.  (It's how Bugzilla rolls.)
Comment 10 Chris Jerdonek 2010-05-04 12:53:55 PDT
(In reply to comment #9)

Thanks, David.

> > +use constant COMMENT_COLS => 160;
> 
> Isn't this going to cause funny wrapping if someone types more than 160
> characters in a single "paragraph"?  Is there a way to disable the wrapping
> altogether (e.g., setting COMMENT_COLS to 0 or -1 or modifying code elsewhere)?

Oops.  Yes, of course.  We're using this for text and not just code.

I experimented with this a bit and found that Perl's $Text::Wrap::wrap() function automatically increases values of -1 and 0 to a minimum of 2.

The maximum supported value seems to be 32766, so another approach could be to set the value high.  It looks like the code enforces a maximum comment length that is roughly double that:

> # The column length for displayed (and wrapped) bug comments.
> use constant COMMENT_COLS => 80;
> # Used in _check_comment(). Gives the max length allowed for a comment.
> use constant MAX_COMMENT_LENGTH => 65535;

In code seems like it may be the cleanest approach.  BugsSite/Bugzilla/Util.pm contains a wrap_comment() subroutine (which uses COMMENT_COLS) which suggests that no other code will be affected by a change there.
Comment 11 David Kilzer (:ddkilzer) 2010-05-04 16:47:51 PDT
(In reply to comment #10)
> The maximum supported value seems to be 32766, so another approach could be to
> set the value high.  It looks like the code enforces a maximum comment length
> that is roughly double that:
> 
> > # The column length for displayed (and wrapped) bug comments.
> > use constant COMMENT_COLS => 80;
> > # Used in _check_comment(). Gives the max length allowed for a comment.
> > use constant MAX_COMMENT_LENGTH => 65535;
> 
> In code seems like it may be the cleanest approach.  BugsSite/Bugzilla/Util.pm
> contains a wrap_comment() subroutine (which uses COMMENT_COLS) which suggests
> that no other code will be affected by a change there.

That seems fine (setting the value high).
Comment 12 Chris Jerdonek 2010-05-06 08:55:37 PDT
Created attachment 55246 [details]
Proposed patch 2

Addressed comments.  Do changes to the bugs site go live when they land, or is there a manual step?
Comment 13 William Siegrist 2010-05-06 09:05:27 PDT
They should update automatically... if they do not, ping me on irc (_wms) or email me.
Comment 14 Darin Adler 2010-05-06 09:31:29 PDT
Comment on attachment 55246 [details]
Proposed patch 2

Looks good. Lets give this a try. r=me
Comment 15 Chris Jerdonek 2010-05-06 19:05:03 PDT
(In reply to comment #6)
> I'm not 100% sure that increasing the COMMENT_COLS constant won't affect other
> forms on the site that I might be less familiar with.

After taking a closer look at this before landing, it looks like changing COMMENT_COLS will affect other areas of the site.  Namely, COMMENT_COLS seems to control the width of the textareas used to input comments, and not just the wrapping behavior for comment output.

So it seems like we should create a new constant that we know will affect only comment display.  Will post patch shortly.
Comment 16 Chris Jerdonek 2010-05-06 19:09:26 PDT
Created attachment 55334 [details]
Proposed patch 3

This patch is the same as before, but creates COMMENT_COLS_WRAP exclusively for use in wrap_comment().  That way we can leave COMMENT_COLS alone for other parts of the code.
Comment 17 Eric Seidel (no email) 2010-05-08 23:11:30 PDT
Comment on attachment 55246 [details]
Proposed patch 2

Cleared Darin Adler's review+ from obsolete attachment 55246 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 18 Maciej Stachowiak 2010-05-08 23:44:57 PDT
Comment on attachment 55334 [details]
Proposed patch 3

r=me
Comment 19 Chris Jerdonek 2010-05-08 23:50:50 PDT
Comment on attachment 55334 [details]
Proposed patch 3

Clearing flags on attachment: 55334

Committed r59047: <http://trac.webkit.org/changeset/59047>
Comment 20 Chris Jerdonek 2010-05-08 23:50:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Chris Jerdonek 2010-05-08 23:53:18 PDT
Test:

1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 

> 34567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 

1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 
1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890
Comment 22 Chris Jerdonek 2010-05-09 00:13:25 PDT
(In reply to comment #10)

Test:

> I experimented with this a bit and found that Perl's $Text::Wrap::wrap() function automatically increases values of -1 and 0 to a minimum of 2.
> 
> The maximum supported value seems to be 32766, so another approach could be to set the value high.  It looks like the code enforces a maximum comment length that is roughly double that:
> 
> > # The column length for displayed (and wrapped) bug comments.
> > use constant COMMENT_COLS => 80;
> > # Used in _check_comment(). Gives the max length allowed for a comment.
> > use constant MAX_COMMENT_LENGTH => 65535;
> 
> In code seems like it may be the cleanest approach.  BugsSite/Bugzilla/Util.pm contains a wrap_comment() subroutine (which uses COMMENT_COLS) which suggests that no other code will be affected by a change there.