Last Comment Bug 514703 - Bug reference is not a link when linebreak between "bug" and number (comment wrapping breaks autolinkification)
: Bug reference is not a link when linebreak between "bug" and number (comment ...
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.4
: All All
: -- major with 2 votes (vote)
: Bugzilla 3.4
Assigned To: Gervase Markham [:gerv]
: default-qa
Mentors:
: 528882 538871 (view as bug list)
Depends on: 105865
Blocks: bmo-regressions-0911
  Show dependency treegraph
 
Reported: 2009-09-04 09:59 PDT by Markus Keller
Modified: 2013-07-22 02:38 PDT (History)
17 users (show)
LpSolit: approval+
mkanat: blocking3.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.5+
mkanat: blocking3.4.4-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v.1 (4.60 KB, patch)
2009-12-23 08:05 PST, Gervase Markham [:gerv]
LpSolit: review+
Details | Diff | Review
Patch v.1 for 3.6 (2.52 KB, patch)
2010-01-04 04:33 PST, Gervase Markham [:gerv]
no flags Details | Diff | Review
Patch v.2 for 3.6 (2.51 KB, patch)
2010-01-05 01:42 PST, Gervase Markham [:gerv]
LpSolit: review+
Details | Diff | Review

Description Markus Keller 2009-09-04 09:59:41 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: 3.4

A reference to a bug is not rendered as a link when a linebreak happened between "bug" and the actual number.

This was OK in Bugzilla 3.2 but is broken in 3.4, probably due to fix for bug 105865.

Reproducible: Always

Steps to Reproduce:
1. Enter a comment that contains a bug reference at the end of a visual line, without typing any line breaks. I entered this line on landfill, see https://landfill.bugzilla.org/bugzilla-3.4-branch/show_bug.cgi?id=8188#c3 :
This is some dummy text, This is some dummy text, This is some x. Bug 123, bug 123

2. Commit
Actual Results:  
Line ends with "bug", next line starts with bug number, but no bug link is generated.

Expected Results:  
Like in 3.2, bug links should go across line breaks.

If it's not possible to find out whether the line break was a manual break from the commenter or a hard wrap that was inserted by the bugzilla server, then err on the side of linking too much.

I've only been working with Bugzilla 3.4 for a week now, and this annoys me several times a day.
Comment 1 Frédéric Buclin 2009-09-04 15:19:18 PDT
The problem is that we have

 [%- wrapped_comment FILTER quoteUrls(bug.bug_id, comment.already_wrapped) -%]

in bug/comments.html.tmpl, i.e the wrapped comment is passed to quoteUrls() instead of the original comment itself. Consequently, all newlines are interpreted as real newlines, meaning we shouldn't try to linkify "bug\nNNN", as expected. To fix this problem, we should pass the original comment first, and do the wrapping later. But this may be tricky to do.
Comment 2 Frédéric Buclin 2009-09-04 15:23:36 PDT
Fixing this bug may also fix bug 509152, or vice versa.
Comment 3 Frédéric Buclin 2009-09-04 16:38:27 PDT
I wonder if we shouldn't set $Text::Wrap::separator to something like "\0\n" in wrap_comment(), so that quoteUrls() can distinguish original newlines and those added while wrapping the text.
Comment 4 Markus Keller 2009-11-18 01:14:07 PST
It's even worse when a comment refers to a bug and a comment: See e.g Bug 123 comment 2: "comment 2" now links to the comment in this bug not the one in bug 123.
Comment 5 Markus Keller 2009-11-18 01:15:16 PST
Oh, looks like it's been fixed in bugzilla 3.4.3.
Comment 6 Frédéric Buclin 2009-11-18 07:20:51 PST
I'm going to repeat your comment for testing (it's still broken on my test installations):

It's even worse when a comment refers to a bug and a comment: See e.g Bug 123 comment 2: "comment 2" now links to the comment in this bug not the one in bug 123.
Comment 7 Frédéric Buclin 2009-11-18 07:21:24 PST
(In reply to comment #5)
> Oh, looks like it's been fixed in bugzilla 3.4.3.

So no, this isn't fixed at all in 3.4.3.
Comment 8 Frédéric Buclin 2009-11-18 11:03:57 PST
*** Bug 528882 has been marked as a duplicate of this bug. ***
Comment 9 Reed Loden [:reed] (use needinfo?) 2009-11-18 11:06:05 PST
This is a pretty bad bug... requesting blocking for next point release.
Comment 10 Markus Keller 2009-11-18 13:00:08 PST
Very strange... I could swear it was looking good right after I posted comment 4. Now, it's broken for me too.
Comment 11 Max Kanat-Alexander 2009-11-18 13:02:10 PST
Well, not a blocker for this release, because 3.4.4 is going out really soon, and the fix for this is pretty complicated, I suspect.
Comment 12 Frédéric Buclin 2009-11-18 13:48:39 PST
(In reply to comment #10)
> Very strange... I could swear it was looking good right after I posted comment
> 4. Now, it's broken for me too.

I know, this happened to me too. And I'm unable to reproduce.
Comment 13 Frédéric Buclin 2009-11-22 10:55:04 PST
I think we should let the browser do the wrapping itself instead of forcing it ourselves in wrap(). We should only specify the width of comments. After all, we don't care if the wrapping is at exactly 80 characters or not, because nobody cares. When you really want to wrap something, you insert \n manually.

So <pre></pre> should go away, and we should instead replace \n by <br> and use a monospace font. This way, ASCII art would still work. Doing so would fix this bug as well as bug 425606. And it would also let you override the width of comments client-side thanks to e.g. chrome/userContent.css in Firefox.
Comment 14 Max Kanat-Alexander 2009-11-22 14:05:41 PST
(In reply to comment #13)
> After all,
> we don't care if the wrapping is at exactly 80 characters or not, because
> nobody cares.

  That's the only specific fact that needs to be verified. Once that is verified, we can proceed, but only on HEAD (because that's a pretty significant behavior change).

> When you really want to wrap something, you insert \n manually.
> 
> So <pre></pre> should go away, and we should instead replace \n by <br> and use
> a monospace font. This way, ASCII art would still work.

  No, you can't fake pre that way. You have to use white-space: pre, and that's exactly the same as using a pre block anyway. White space is significant in a <pre>, and it's not significant elsewhere.

> Doing so would fix this
> bug as well as bug 425606. And it would also let you override the width of
> comments client-side thanks to e.g. chrome/userContent.css in Firefox.

  Yeah, it seems like a generally workable solution, but Bugzilla has wrapped comments at 80 characters for 10 years, so we need to think a bit, first, about what will happen when we change that.
Comment 15 Max Kanat-Alexander 2009-12-13 14:56:19 PST
Since it looks like the only way that we're going to fix this is to stop wrapping comments server-side, we have to look into:

a) Whether or not all popular browsers can actually wrap a <pre> client-side (I recall this being an issue when I looked into it originally)

b) If wrapping at an arbitrary number of characters is going to cause a problem for some significant number of Bugzilla users.

So that's something we should definitely look into before 3.6, but it's not a change we're going to be able to make for 3.4.
Comment 16 Gervase Markham [:gerv] 2009-12-17 10:35:38 PST
As Max points out, my understanding is that there are only two ways in browsers that you can do:

Fooo   Bar    Baz
       Quux   Fred

with spaces retained. One is to use pre, or CSS white-space: pre, which I think has exactly the same effect but has the disadvantage of being less well supported cross-browser. The other is to replace every space with &nbsp. And you don't want to go down that route.

If the bug is that "Bug 123" does not linkify, why can we not fix it by adding "\n" to the list of characters in the linkification regexp which are permitted to separate the word "bug" from the bug number?

In fact, we appear to have code which is supposed to get this right. From quoteUrls:

    # If the comment is already wrapped, we should ignore newlines when
    # looking for matching regexps. Else we should take them into account.
    my $s = $already_wrapped ? qr/\s/ : qr/[[:blank:]]/;

So we need to figure out why this code is not doing what we expect.

Gerv
Comment 17 Max Kanat-Alexander 2009-12-17 13:38:57 PST
(In reply to comment #16)
> So we need to figure out why this code is not doing what we expect.

  Don't worry about that. The code is doing what I expect it to do. I've already investigated this bug, and the resolution is already known, we just have to decide whether or not we want to do it.
Comment 18 Gervase Markham [:gerv] 2009-12-17 14:23:38 PST
Max: that seems unncessarily cryptic :-) What is your proposed resolution?

Given how complicated it was last time to produce something which met all the constraints, I am deeply worried about any plan to change the way we submit, store or format comments. I strongly feel such a plan should be documented and discussed, with full reference to why we decided to go with what we've got now, before implementation.

Gerv
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-17 14:38:43 PST
Given that this was (a) a regression and (b) an *intentional* change (bug 105865), it seems like it shouldn't be hard to fix by simply reverting the change.  This bug seems like a bigger problem than bug 105865.
Comment 20 Max Kanat-Alexander 2009-12-17 14:43:44 PST
(In reply to comment #18)
> Max: that seems unncessarily cryptic :-) What is your proposed resolution?

  It's in the bug, in the two comments before yours.

  As far as implementation, I've personally done the implementation of all the wrapping code for the last several years, and I am aware of all the constraints, so I'm not worried about it. 

  There is already a discussion happening about this bug, that's how you found it.
Comment 21 Max Kanat-Alexander 2009-12-17 14:46:01 PST
(In reply to comment #19)
> Given that this was (a) a regression and (b) an *intentional* change (bug
> 105865), it seems like it shouldn't be hard to fix by simply reverting the
> change.  This bug seems like a bigger problem than bug 105865.

  That seems reasonable for 3.4 and 3.6, though it would be slightly more difficult than a simple reversion (since the code there has been touched after that point). LpSolit?
Comment 22 Frédéric Buclin 2009-12-17 14:53:32 PST
I first want to understand what went wrong before backing out the patch.
Comment 23 Gervase Markham [:gerv] 2009-12-21 09:50:49 PST
Why is wrapping comments client-side instead of server-side the only possible solution to this bug (as comment 15 claims)? Why can't we just do the quoting and wrapping in the correct order?

Gerv
Comment 24 Gervase Markham [:gerv] 2009-12-21 10:02:35 PST
Also, if we aren't going to sort out wrapping in all cases for 3.4 and 3.6, can we please do the backout sooner rather than later? For six-digit bug numbers, I would expect this bug to affect the autolinking of, on average, 6/80ths of bug references - one in 13. That's enough to really annoy people who read hundreds of Bugzilla comments every day.

We should put things back the way they were ASAP, in the next stable point release, and _then_ figure out why it didn't work, and whether there's a safe fix which solves all cases that we can re-do on the branch or whether it'll need to wait until 3.8. That work will take time and care - which is absolutely fine, as long as we don't leave things broken while we are doing it.

Gerv
Comment 25 Markus Keller 2009-12-21 10:23:44 PST
(In reply to comment #24)
I fully support that. This bugs me multiple times a day, whereas I only see the problem from bug 105865 a few times a year. This bug is so bad that I even hacked a Greasemonkey script that turns wrapped references into links again (without tooltips, though).
Comment 26 Max Kanat-Alexander 2009-12-21 15:33:28 PST
(In reply to comment #23)
> Why is wrapping comments client-side instead of server-side the only possible
> solution to this bug (as comment 15 claims)?

  Because <a href="show_bug.cgi?id=123456" title="some summary goes here">bug 123456</a> is considerably longer than "bug 123456" and wrapping would reflect that.
Comment 27 Max Kanat-Alexander 2009-12-21 15:39:18 PST
I'd be happy to review a patch that backs it out.
Comment 28 Gervase Markham [:gerv] 2009-12-21 15:40:01 PST
What is wrong with the way it works now, which I believe involves using a null character to denote a commenter-inserted newline, so that you can:

- replace \n with \0\n
- wrap
- quoteURLs, allowing \n as whitespace (but not \0\n)
- replace \0\n with \n

?

Gerv
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-12-21 16:40:20 PST
(In reply to comment #26)
> (In reply to comment #23)
> > Why is wrapping comments client-side instead of server-side the only possible
> > solution to this bug (as comment 15 claims)?
> 
>   Because <a href="show_bug.cgi?id=123456" title="some summary goes here">bug
> 123456</a> is considerably longer than "bug 123456" and wrapping would reflect
> that.

Why does that prevent wrapping server-side?
Comment 30 Gervase Markham [:gerv] 2009-12-23 08:05:09 PST
Created attachment 419011 [details] [diff] [review]
Patch v.1

This patch, against 3.4, backs out bug 105865.

Gerv
Comment 31 Frédéric Buclin 2009-12-23 08:16:18 PST
(In reply to comment #28)
> - replace \n with \0\n
> - wrap
> - quoteURLs, allowing \n as whitespace (but not \0\n)
> - replace \0\n with \n

That's more or less how things should work if we keep wrapping server-side, yes, i.e. distinguish real newlines inserted by the user from those added by wrap().
Comment 32 Max Kanat-Alexander 2009-12-23 19:50:51 PST
Comment on attachment 419011 [details] [diff] [review]
Patch v.1

>-    # If the comment is already wrapped, we should ignore newlines when
>-    # looking for matching regexps. Else we should take them into account.
>-    my $s = $already_wrapped ? qr/\s/ : qr/[[:blank:]]/;

  Actually, you know, thinking about this--if we give all the regexes the /s modifier, does that fix the whole problem, without having to know whether or not the comment is wrapped? That is, would that fix both cases?
Comment 33 Frédéric Buclin 2009-12-27 06:55:21 PST
Comment on attachment 419011 [details] [diff] [review]
Patch v.1

r=LpSolit for 3.4.x. But the patch doesn't apply cleanly for 3.6.
Comment 34 Frédéric Buclin 2009-12-27 06:57:15 PST
Waiting for a patch for 3.6 before approving it.
Comment 35 Gervase Markham [:gerv] 2010-01-04 04:33:31 PST
Created attachment 419889 [details] [diff] [review]
Patch v.1 for 3.6

This should be what's needed for 3.6.

Gerv
Comment 36 Frédéric Buclin 2010-01-04 06:24:44 PST
Comment on attachment 419889 [details] [diff] [review]
Patch v.1 for 3.6

>Index: Bugzilla/Template.pm

>@@ -167,15 +167,10 @@ sub quoteUrls {

The third parameter $comment is no longer used in quoteUrls(). It was only used to determine if the comment was already wrapped or not. It should be removed too (also from the hook). So this basically means doing the same changes as you did in your patch for 3.4.
Comment 37 Gervase Markham [:gerv] 2010-01-04 07:08:46 PST
Yes it is used - it's passed to a hook on (new) line 188.

    Bugzilla::Hook::process('bug-format_comment',
        { text => \$text, bug => $bug, regexes => \@hook_regexes,
          comment => $comment });

Gerv
Comment 38 Frédéric Buclin 2010-01-04 07:14:06 PST
(In reply to comment #37)
> Yes it is used - it's passed to a hook on (new) line 188.

That's what I said. It's not used in quoteUrls() itself, only in the hook. And even the hook example in extensions/Example doesn't use it. So I guess it can be removed from the hook too. dkl, mkanat: is the comment object really useful to you? If not, we should remove it.
Comment 39 Gervase Markham [:gerv] 2010-01-04 07:36:49 PST
Surely the whole _point_ of a hook is that people might use it in ways not envisaged by the Bugzilla authors? If the comment object contains information about the comment other than its text (which I believe it does) then why would you _not_ want to pass it to a hook called bug-format_**comment**? How do you know what criteria people might want to use in order to decide how to format a comment?

The Bugzilla team's attitude to extension points seems to be that we don't need them unless we have a use for them ourselves (see the JSON filter bug for another example of this). This seems the wrong way round to me.

Gerv
Comment 40 David Lawrence [:dkl] 2010-01-04 07:45:14 PST
(In reply to comment #39)
> Surely the whole _point_ of a hook is that people might use it in ways not
> envisaged by the Bugzilla authors? If the comment object contains information
> about the comment other than its text (which I believe it does) then why would
> you _not_ want to pass it to a hook called bug-format_**comment**? How do you
> know what criteria people might want to use in order to decide how to format a
> comment?
> 
> The Bugzilla team's attitude to extension points seems to be that we don't need
> them unless we have a use for them ourselves (see the JSON filter bug for
> another example of this). This seems the wrong way round to me.
> 
> Gerv

I tend to agree with Gerv in that having the other comment data could prove useful in the future and is easier to leave it in now than to have to add it back later. Besides the comment object is not that large and should not prove
to be any harm in my opinion.

Dave
Comment 41 Frédéric Buclin 2010-01-04 07:54:27 PST
(In reply to comment #39)
> Surely the whole _point_ of a hook is that people might use it in ways not
> envisaged by the Bugzilla authors?

Sure, but we could pass the whole DB in a hook too, in that case, as you don't know what developers want to do with your code. The one who asked for the hook (Colin, bug 364254) just wanted to linkify some strings, which doesn't require the whole comment object. And the one who included the comment object to the hook is mkanat. And I saw no other requests to have the comment object included in the hook. So if nobody needs it, we can remove it.

And the only reason the comment object is passed to the hook (unless otherwise demonstrated) is because I initially passed comment.already_wrapped to quoteUrls() for bug 105865, and so it was already available there independently of the hook.
Comment 42 Gervase Markham [:gerv] 2010-01-04 08:05:17 PST
Passing the comment object in a hook to do with formatting comments is not the same thing as passing the whole DB to any hook. (If anyone suggests doing that, I will join you in objecting.) The "slippery slope" is not really all that slippery.

You can never say "nobody needs it" about an externally-facing API; you can only say "nobody I/we know of needs it". There may be people already using it, you don't know.

Gerv
Comment 43 Max Kanat-Alexander 2010-01-04 18:20:39 PST
Please don't remove $comment, hooks need that information.
Comment 44 Frédéric Buclin 2010-01-04 18:24:58 PST
Comment on attachment 419889 [details] [diff] [review]
Patch v.1 for 3.6

Hum, ok. So I will give it another look. The patch looks correct; I just want to test it briefly.
Comment 45 Gervase Markham [:gerv] 2010-01-05 01:42:42 PST
Created attachment 420049 [details] [diff] [review]
Patch v.2 for 3.6

Patch which applies to current tip.

Gerv
Comment 46 Frédéric Buclin 2010-01-05 01:49:30 PST
Comment on attachment 420049 [details] [diff] [review]
Patch v.2 for 3.6

r=LpSolit
Comment 47 Frédéric Buclin 2010-01-06 07:05:08 PST
gerv, could you commit this patch, please?
Comment 48 Gervase Markham [:gerv] 2010-01-07 07:06:22 PST
Fixed on trunk and 3.4 branch:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.124; previous revision: 1.123
done


Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.41.2.2; previous revision: 1.41.2.1
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.99.2.5; previous revision: 1.99.2.4
done

Gerv
Comment 49 Frédéric Buclin 2010-01-10 10:08:41 PST
*** Bug 538871 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.