Closed Bug 514703 Opened 15 years ago Closed 14 years ago

Bug reference is not a link when linebreak between "bug" and number (comment wrapping breaks autolinkification)

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: markus.kell.r, Assigned: gerv)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.4
Version: unspecified → 3.4
Fixing this bug may also fix bug 509152, or vice versa.
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.
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.
Oh, looks like it's been fixed in bugzilla 3.4.3.
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.
(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.
This is a pretty bad bug... requesting blocking for next point release.
Flags: blocking3.4.4?
Keywords: regression
Very strange... I could swear it was looking good right after I posted comment 4. Now, it's broken for me too.
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.
Flags: blocking3.4.4? → blocking3.4.4-
(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.
Flags: blocking3.4.5?
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.
(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.
Flags: blocking3.4.5? → blocking3.4.5-
Flags: blocking3.6?
Flags: blocking3.4.6?
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.
Flags: blocking3.6?
Flags: blocking3.6+
Flags: blocking3.4.6?
Flags: blocking3.4.6-
Target Milestone: Bugzilla 3.4 → Bugzilla 3.6
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
(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.
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
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.
(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.
(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?
I first want to understand what went wrong before backing out the patch.
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
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
(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).
Severity: normal → major
(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.
I'd be happy to review a patch that backs it out.
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
(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?
Attached patch Patch v.1Splinter Review
This patch, against 3.4, backs out bug 105865.

Gerv
Assignee: create-and-change → gerv
Status: NEW → ASSIGNED
Attachment #419011 - Flags: review?(mkanat)
Blocks: 105865
(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 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 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.
Attachment #419011 - Flags: review?(mkanat) → review+
Waiting for a patch for 3.6 before approving it.
Flags: blocking3.4.6-
Flags: blocking3.4.6+
Flags: approval3.4?
Target Milestone: Bugzilla 3.6 → Bugzilla 3.4
Attached patch Patch v.1 for 3.6 (obsolete) — Splinter Review
This should be what's needed for 3.6.

Gerv
Attachment #419889 - Flags: review?(LpSolit)
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.
Attachment #419889 - Flags: review?(LpSolit) → review-
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
(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.
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
(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
(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.
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
Please don't remove $comment, hooks need that information.
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.
Attachment #419889 - Flags: review- → review?(LpSolit)
Patch which applies to current tip.

Gerv
Attachment #419889 - Attachment is obsolete: true
Attachment #420049 - Flags: review?(LpSolit)
Attachment #419889 - Flags: review?(LpSolit)
Comment on attachment 420049 [details] [diff] [review]
Patch v.2 for 3.6

r=LpSolit
Attachment #420049 - Flags: review?(LpSolit) → review+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+
gerv, could you commit this patch, please?
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Bug reference is not a link when linebreak between "bug" and number → Bug reference is not a link when linebreak between "bug" and number (comment wrapping breaks autolinkification)
Flags: blocking3.4.6+
Flags: blocking3.4.5-
Flags: blocking3.4.5+
Depends on: 652757
No longer depends on: 652757
No longer blocks: 105865
Depends on: 105865
You need to log in before you can comment on or make changes to this bug.