Bug 37792 – bugs.webkit.org: should not hard-wrap comment lines exceeding 80 characters

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This report stems from the following webkit-dev e-mail:

https://lists.webkit.org/pipermail/webkit-dev/2010-April/012528.html

[Chris Jerdonek]

This is configured here:

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

[Chris Jerdonek]

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.

[Maciej Stachowiak]

(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.

[Chris Jerdonek]

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).

[Chris Jerdonek]

(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.

[Chris Jerdonek]

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.

[Chris Jerdonek]

(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:

[Chris Jerdonek]

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.

[David Kilzer (:ddkilzer)]

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.)

[Chris Jerdonek]

(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.

[David Kilzer (:ddkilzer)]

(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).

[Chris Jerdonek]

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?

[William Siegrist]

They should update automatically... if they do not, ping me on irc (_wms) or email me.

[Darin Adler]

Comment on attachment 55246 [details]
Proposed patch 2

Looks good. Lets give this a try. r=me

[Chris Jerdonek]

(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.

[Chris Jerdonek]

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.

[Eric Seidel (no email)]

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.

[Maciej Stachowiak]

Comment on attachment 55334 [details]
Proposed patch 3

r=me

[Chris Jerdonek]

Comment on attachment 55334 [details]
Proposed patch 3

Clearing flags on attachment: 55334

Committed r59047: <http://trac.webkit.org/changeset/59047>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.

[Chris Jerdonek]

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

[Chris Jerdonek]

(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.