Bug 207754 (bugreplies)

It should be possible to produce a quoted reply to a comment

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: Christian Reis, Assigned: Christian Reis)

Tracking

2.17.4
Bugzilla 2.18
x86
Linux
Bug Flags:
approval +

Details

(URL)

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

15 years ago
Even though many people commonly produce replies to bugzilla comments manually
(by cut-n-pasting and adding quote symbols such as "> ", there is no facility to
do this automatically. 

In our local Bugzilla installation we'd really appreciate something like this,
as we have a very small group of users; timeless has shown reservation as to
having this on a large installation because of its abuse potential (I'd like to
point out that editing attachments also produces quoted text). I think the
benefits outweigh the problems, but I propose nevertheless wrapping the code in
a Param() to make sure it's only activated by administrators that think it's a
benefit.

I haven't done a lot of Bugzilla work lately (none would be closer to the truth)
so please be kind :-)
(Assignee)

Comment 1

15 years ago
Created attachment 124633 [details] [diff] [review]
kiko_v1: WIP implementation, implements functionality but Param disabled

I've cobbled a quick JS patch that works, with document.write() and the
required niceties. I'm going to test it from the office w/ IE soon to make sure
it works x-browser.

The changes to the page are minimal (basically adding ids and an extra anchor).


It would really be nice to get feedback on this. The issues I would like to
hear about are:

- Is the Param() worth it?
- Should I use a separate JS file?
- Are the changes I did to the form elements intrusive?
- Should I add an accesskey? :-)

Thanks
(Assignee)

Updated

15 years ago
Alias: bugreplies
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.18

Comment 2

15 years ago
> I've cobbled a quick JS patch that works, with document.write() and the
> required niceties. I'm going to test it from the office w/ IE soon to make sure
> it works x-browser.

It doesn't... IE only prepends the "> " to the first line (IE 6, WinXP).

> - Is the Param() worth it?

I would say no, but I'm not the final call :)

> - Should I use a separate JS file?

Perhaps... it would cause it to not be downloaded by clients that don't support
JavaScript, but it's really not a huge piece of script.

> - Are the changes I did to the form elements intrusive?

I tend to think not.

> - Should I add an accesskey? :-)

Magic 8 ball says "No" (OK, I made that up, I don't have a magic 8 ball).

Why use that nodeValue stuff? I think that's what's causing it to not work
properly in IE (all the text gets added, but only the first line gets a "> "). I
tested briefly with |var text = text_elem.innerHTML;| and it worked fine in IE
and Mozilla.

If a line is really close to that 80 character hard wrap you end up w/one word
on the following line w/out a "> " prepended. Not sure how to solve this, other
than fixing bug 11901 then using the same method the attachment code uses (don't
wrap lines that begin w/">").

Nit: Most of the time you end the JavaScript line w/a semi-colon, but
occationally you don't.
Mozilla already has support for "Paste as Quotation" in email; I tend to use the
email composer for composing long replies, and then paste it back in when I'm
finished.

Perhaps the right way to go here is an XPI which adds "Paste as Quotation" to
browser textboxes in Mozilla/Mozilla Firebird? This does seem more like a
browser feature...

Gerv
(Assignee)

Comment 4

15 years ago
I don't think it's `really a browser issue'; offering a way reply to comments
sounds very much like a bugzilla task. Plus, the code is here, and it almost
works ;) whereas .xpis are still vapour.

An accesskey would be silly, anyway, thinking more about it.

The problem with IE had nothing to do with nodeValue; it was the fact that lines
are split on Windows using "\r" and not "\n" :-)

We can't use innerHTML because it copies entities and tags literally. We can't
use innerText because it is non-standard. I chose to do the recursive iteration
through children for that reason, and it does work.

Updated patch coming up.
(Assignee)

Comment 5

15 years ago
Created attachment 124649 [details] [diff] [review]
kiko_v2: works on IE/Mozilla, Param() still in

Hopefully no ;s are missing now (don't you hate languages that have `optional'
syntax?).

It would be nice to get somebody to test on other browsers. Timeless?

Test install:
http://kiko.kicks-ass.org:8000/~kiko/bugzilla/show_bug.cgi?id=3
Attachment #124633 - Attachment is obsolete: true
(Assignee)

Comment 6

15 years ago
Created attachment 124672 [details] [diff] [review]
kiko_v3: avoid id collisions for long_list and midairs, remove Param

This patch is ready for review. I've incorporated some remarks by bbaetz that
indicated we should avoid id collisions when not editing, and removed the
param:

  <kiko> ah, right. should we add a Param() for this?
  <bbaetz> kiko: Nah, don't param it, I think
  <bbaetz> its a UI feature, so an admin can easily disable it if they want

I've tested on both IE/Mozilla. Will test on Opera Windows/Unix at the office
later.
(Assignee)

Updated

15 years ago
Attachment #124649 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124672 - Flags: review?(bbaetz)
(Assignee)

Comment 7

15 years ago
It doesn't work on the version of Opera I have at the office. I suspect it's
because the DOM support in Opera is subpar, but I'm not sure. Fabian?
Hmm. It bottom-posts. That's probably bad...

Gerv
(Assignee)

Comment 9

15 years ago
Hmmm, I'm unsure what you mean by `it bottom-posts'; I paste the text into the
entry, with a \n prefixed. I can remove the newline to avoid suggesting that new
text should go above the reply; it seems like a good idea.
(Assignee)

Updated

15 years ago
(Assignee)

Comment 10

15 years ago
Created attachment 124753 [details] [diff] [review]
kiko_v4: remove newline, space correctly, and add `... in comment #X' to header

Should be ready to roll, and it's live on the test server at the URL.

One issue that somebody might want to point out is that nick changes are not
supported; you're right, and I think that's a feature :) The comment link I
added makes sure we know what we are referring to.
Attachment #124672 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124672 - Flags: review?(bbaetz)
(Assignee)

Updated

15 years ago
Attachment #124753 - Flags: review?(bbaetz)
Comment on attachment 124753 [details] [diff] [review]
kiko_v4: remove newline, space correctly, and add `... in comment #X' to header

>Index: comments.html.tmpl

>+    /* pre id="comment_name_N" */

s/name/text/

>+    /* make sure we split on all newlines -- IE or Moz use \r and \n
>+     * respectively */
>+    text = text.split(/\r|\n/);

What about \r\n ?

Theres no way of replying to the initial comment. I'm nor sure where the UI for
that would go, though. Thoughts? If you can't come up with a place for it, and
you've tested on various browsers/OSs, r=bbaetz
Attachment #124753 - Flags: review?(bbaetz) → review+
Comment on attachment 124753 [details] [diff] [review]
kiko_v4: remove newline, space correctly, and add `... in comment #X' to header

>Index: comments.html.tmpl
>===================================================================
>-        <a href="mailto:[% comment.email FILTER html %]">[% comment.name FILTER html %]</a>
>+        <a href="mailto:[% comment.email FILTER html %]">
>+        [%# avoid span id collisions if we are not editing a bug #%]

This comment is really confusing. I stared at it for 30 seconds and still
couldn't figure it out. Either clarify it or (probably better) remove it.

> [%# Don't indent the <pre> block, since then the spaces are displayed in the
>-  # generated HTML
>+  # generated HTML. Same id collision avoidance as for span (above)
>   #%]

Same for this one.

>Index: edit.html.tmpl
>===================================================================
>+  <script type="text/javascript" language="JavaScript">
>+  <!--
>+  /* Outputs a link to call doReply(); used to reduce HTML output */
>+  function addReplyLink(count) {
>+    document.write('<a href="#add_comment" onclick="doReply(' + 
>+        count + ');">(Reply)</a>');
>+  }

I don't like the principle here, really - document.write()ing stuff to save
space actually just obfuscates and makes debugging harder.

But the real reason not to do it is this:

<script type="text/javascript" language="JavaScript"><!-- addReplyLink('[%
count %]'); //--></script>
<a href="#add_comment" onclick="doReply(' + count + ');">(Reply)</a>

!

>+  /* Adds the reply text to the `comment' textarea */

Can we use TT comments, to avoid bloating the HTML? ;-)

>+  function doReply(count) {
>+    /* span id="comment_name_N" */
>+    var name_elem = document.getElementById('comment_name_'+count);

There's no protection or error catching here for old browsers. You can
userAgent sniff in TT (probably best).

My last point is that no-one has yet pointed out the big can of worms this
opens up. Auto-quoting in mail user agents has led to some really, really bad
and irritating practices, mostly concerned with people who are too lazy to trim
properly. This is not just restricted to newbies. I am very concerned about
Bugzilla becoming loaded with comments like:

In comment 33, J. Random Hacker wrote:
> ffds gfds gfds gf  gfds gfds gfds gfd
> gf gfds gfds gfds gfdsg ds
... (33 lines later)
> gfdsg gfd gsd gfdsdsg

Exactly!

-- Some idiot

or, even worse:

Exactly!

In comment 33, J. Random Hacker wrote:
> ffds gfds gfds gf  gfds gfds gfds gfd
> gf gfds gfds gfds gfdsg ds
... (33 lines later)
> gfdsg gfd gsd gfdsdsg


(I personally achieve the function of this patch, when I need it, by creating a
new mail message in Moz, doing "paste as quotation" into it, and editing my
comment in there. This works very well.)

Gerv
Attachment #124753 - Flags: review-
(Assignee)

Comment 13

15 years ago
(Wishing I had (Reply) to avoid cut and pasting and fixing up reply)

bbaetz: I can add (Reply) after the Description link, making it like:

  Description:               (Reply) | Opened ...

sound acceptable? Neil also asked for this.

I'm not sure about \r\n -- do any browsers send that? I haven't seen this in the
wild while testing.

It doesn't work with Opera. If somebody can test in iCab and Safari I'd be grateful.

> >+        <a href="mailto:[% comment.email FILTER html %]">
> >+        [%# avoid span id collisions if we are not editing a bug #%]
>
> This comment is really confusing. I stared at it for 30 seconds and still
> couldn't figure it out. Either clarify it or (probably better) remove it.

I get your point. The thing is, when you're not editing a bug -- besides not
being much point in replying -- multiple comment blocks for multiple bugs may be
rendered in the same page (think long_list.cgi). 

> I don't like the principle here, really - document.write()ing stuff to save
> space actually just obfuscates and makes debugging harder.

Actually, justdave recommended I use document.write() to avoid generating the
link for non-JS browsers. What do you suggest I do? Use the plain
document.write() inline?

As to the `can of worms', I don't use Mozilla Mail, and I don't plan to. IE
certainly doesn't allow one to paste as reply, and trunk (unmodified)
Mozilla/FB/etc don't either. I agree it has abuse potential, however, which is
why I proposed the Param() initially -- if we don't want it on bmo, I guess it's
reasonable. If we *really* want, we can make it a per-user option that can be
enabled manually. It could even be assigned by the sysadmin for `special' users.
But I'm not proposing delving down that street.

I think the value provided here is high when the bug is a short, focused log of
what's going on -- many times people make questions that go unanswered (or with
poorly connected answers) simply because there is no easy mechanism to reply to
them. When a bug is rambling, it's not truly going to be the reply link's fault.

Opinions?
> I get your point. The thing is, when you're not editing a bug -- besides not
> being much point in replying -- multiple comment blocks for multiple bugs may be
> rendered in the same page (think long_list.cgi). 

Then we need to pass a parameter into the comment template indicating whether
reply links are needed or not.

>> I don't like the principle here, really - document.write()ing stuff to save
>> space actually just obfuscates and makes debugging harder.
> 
> Actually, justdave recommended I use document.write() to avoid generating the
> link for non-JS browsers. What do you suggest I do? Use the plain
> document.write() inline?

Hmm. I forgot about that. What you could do, actually, is put it in there
unconditionally with a style="display: none" and then reveal it using CSS which
you only print for browsers which support it.

> As to the `can of worms', I don't use Mozilla Mail, and I don't plan to. IE
> certainly doesn't allow one to paste as reply, and trunk (unmodified)
> Mozilla/FB/etc don't either. 

I'm not suggesting you use Mozilla Mail. :-)

> I agree it has abuse potential, however, which is
> why I proposed the Param() initially -- if we don't want it on bmo, I guess it's
> reasonable. If we *really* want, we can make it a per-user option that can be
> enabled manually. 

But the problem with that is that a user's misuse of it affects others, not
themselves. So we can't just let users turn it on for themselves.

The other problem with having it as a Param is that it'll be the source of
arguments. 
- "We want this feature we've seen on other Bugzillas!"
"No, sorry, it'll be abused."
- "But we're sensible - why should we suffer for other people's stupidity. And
how do you know it'll be abused."
<admin turns it on. It's abused. admin turns it off.>
- "Hey! Why have you removed this useful feature?"
"Because it was abused."
- "But I didn't..."
etc. etc.

> It could even be assigned by the sysadmin for `special' users.
> But I'm not proposing delving down that street.

The problem is that these proposals might prevent abuse, but put the
administration burden on the admin.

> I think the value provided here is high when the bug is a short, focused log of
> what's going on -- many times people make questions that go unanswered (or with
> poorly connected answers) simply because there is no easy mechanism to reply to
> them. When a bug is rambling, it's not truly going to be the reply link's fault.

Bugs being rambling is a different issue. You can have a focussed email
conversation with someone who doesn't quote properly.

The bottom line is that currently, Bugzilla bug comments have a very high signal
to noise ratio in a given comment - because people have to quote manually, they
only quote what they need. If we add this feature, we'll get a load more
unnecessary cruft in comments.

Gerv

Comment 15

14 years ago
(Also wishing I had (reply) to avoid cut and pasting and fixing up reply)

> But the problem with that is that a user's misuse of it affects others, not
> themselves. So we can't just let users turn it on for themselves.

Oh please.  You think 'abusers' of the system (quoting a largeish piece and
responding quickly) couldn't abuse the same way using something like Mozilla
Mail?  You think making it a per-user 'turn this on' option in Bugzilla wouldn't
discourage them just as much as they already are?  Calling that 'abuse' is
questionable in the first place, frankly.  Whatsmore, I've participated in
plenty of web forums that have quoting as an option, as well as mailing list
discussions (many e-mail clients allow it), and have seen very, very little
abuse.  You're just blowing out hot air/FUD.  If someone regularly abused the
quote feature, they could be banned from that Bugzilla.

> Bugs being rambling is a different issue. You can have a focussed email
> conversation with someone who doesn't quote properly.

It's not half as easy, or clear.
> Oh please.  You think 'abusers' of the system (quoting a largeish piece and
> responding quickly) couldn't abuse the same way using something like Mozilla
> Mail?

Well, no - because they have that option now, and it doesn't happen.

> You think making it a per-user 'turn this on' option in Bugzilla wouldn't
> discourage them just as much as they already are?  

I don't understand that sentence. I'm not advocating making it any sort of option.

> You're just blowing out hot air/FUD.  If someone regularly abused the
> quote feature, they could be banned from that Bugzilla.

No because, while their behaviour would be irritating, banning them would be an
overreaction.

Bottom line is: currently, most Bugzilla quoting is concise and to the point.
That makes me think that nothing is broke, so we shouldn't fix it.

Gerv
(Assignee)

Comment 17

14 years ago
I'm tiring of this discussion, so let me get to the point.

One sort of problem is what bugzilla.mozilla.org has: too many bugzilla users.
In this case, you might want to go out of your way to make it *less usable* for
posters, since the chance that they abuse the comments is higher than minimal. I
hope we agree on that, that the absence of a reply function implies reduced
usability of Bugzilla (not horribly, but it does)?

However, most sites don't have b.m.o's problem. Most sites are like mine, and
other companies: small, focused group of people commenting, on focused bugs.
Telling those sites that they can't have improved usability for replies via a
Reply button because bugzilla.mozilla.org doesn't want it is not really fair. We
*want* more users, and we *want* them to reply. That's a different problem set.

Now, when we decided to move to templates, it was *exactly* to allow b.m.o (and
others) to customize away bits of the UI they thought was problematic. Just
saying that "Oh, I use Mozilla Mail and it has paste quoted" just glosses over
the issue.

Please, can we have Myk or Justdave arbitrate on this? I really would appreciate
avoiding further discussion when this patch has had code attached and reviewed
for nearly two months. This post sums up my arguments and if owners decide they
aren't enough, WONTFIX it and I'll move on to something else.
> I hope we agree on that, that the absence of a reply function implies reduced
> usability of Bugzilla (not horribly, but it does)?

Actually, we don't agree on that - it might make it more "usable" for the person
doing the replying, but I think it would make it less usable for everyone else,
having to wade through the inevitable untrimmed quoting.

But anyway, if you want this feature so much, I'm not going to argue any more.
I'm going on holiday for three weeks - get it in before I get back ;-)

Gerv
(Assignee)

Comment 19

14 years ago
Finally ;)

I'll cook up a version with UI for replying to the initial comment.
(Assignee)

Comment 20

14 years ago
Created attachment 129467 [details] [diff] [review]
kiko_v5: add reply link for initial comment, and fix up commenting somewhat

I've added the reply link after the opened date. It's now also all-lowercase.
It works okay, but there is a problem with the link changing position
horizontally, since name lengths vary considerably. I don't see a way out there
beyond adopting fixed layout (a table or something) for the comment block, or
placing the reply link on the left. I don't like any of them very much.

[In my personal opinion, the comment block needs an overhaul (I don't consider
the initial reporter a very crucial role, in which case he could be pointed out
as the first `commenter', but IMBW). That can come later.]
Attachment #124753 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
(Assignee)

Comment 21

14 years ago
Comment on attachment 129467 [details] [diff] [review]
kiko_v5: add reply link for initial comment, and fix up commenting somewhat

I've updated the URL, and I've created a test user for others. Use:

eleet@async.com.br / bogus
Attachment #129467 - Flags: review?(preed)
Let's put it in.  There's 3 lines of HTML to comment out in the template if you
don't want it.  That's a small enough change that I feel comfortable making that
patch to b.m.o if they don't want it.
Oh, and the reason I added Jake to the CC... :)

We should add this to the docs in the "things you might want to customize in the
templates" section.
Comment on attachment 129467 [details] [diff] [review]
kiko_v5: add reply link for initial comment, and fix up commenting somewhat

Nit: I think I'd actually like the [Reply] link for the description over by the
description text, a la:
Description: [Reply]

Where it is now: it's in the right-hand side of the screen, with a bunch of
whitespace separating it, and I could easily miss it.

Other than, I deployed it in 2.17.4; looks good.

Have I mentioned how much I love this feature?
Attachment #129467 - Flags: review?(preed) → review+
(Assignee)

Comment 25

14 years ago
Created attachment 129493 [details] [diff] [review]
kiko_v8 (internal versions omitted!)

I think I agree about the Reply link. However, my real plan is to move towards
a layout similar to http://bugs.async.com.br/show_bug.cgi?id=472 -- a nice
header for each comment which allows us to space things out evenly. 

I'll attach my (hopefully final) patch, and the URL site is updated to it.

Changes: the reply link position (and an extra secret change)

I did some pondering on spacing from the header. See 
http://www.async.com.br/~kiko/mybugzilla/show_bug.cgi?id=2#c7
and
http://www.async.com.br/~kiko/mybugzilla/show_bug.cgi?id=2#c12
for examples. This is a one-liner change, but I'm not sure I prefer the latter,
so I left it alone for now.

I'll let this sit and if nobody complains, I'll check it in on Monday.
Attachment #129467 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Flags: approval+
(Assignee)

Comment 26

14 years ago
Doh.
Flags: approval?
Without looking at any patches, and going based solely off of what kiko has told
me, I have a veto on at least one point.

- We should so NOT store realnames or email addresses in comment replies if we
can help it.  These can be changed by the user, and it would be great to
dynamically update them.

I suggest kiko writes a GetCommentLink() subroutine similar to GetBugLink() and
GetAttachmentLink() which will link to the correct named anchor within the
current bug, and offer a tooltip (via the title attribute) which contains text
more or less like "Posted by Foo <foo@bar.com> on 2003-05-31 06:09".

Then, the reply comment will look similar to attachment replies, and not have
obsolete user information.


------- Additional Comment #4 From _Foo_ 2003-08-08 20:00 -------
(From update of _comment 3_)
> Please RSVP.
Okay.
Actually, that should say:

(In reply to _comment 3_)

I copied an attachment link and forgot to change that ;-)
re: comment 27:

Email addresses *are* a problem (a la bug 120030), but I don't have a problem
with comments referencing people's old names; Bugzilla prides itself on creating
an accurate historical record, and if that's what their name was at the time,
then I don't see a problem with it going in comments for posterity. That
information may actually be useful (if someone puts information like "on
vacation" or whatever in their 'name', for instance.)

Having said that, I also don't have a problem with removing the name altogether,
so it just said "In reply to comment #53:"
(Assignee)

Comment 30

14 years ago
Created attachment 129829 [details] [diff] [review]
kiko_v9: use simple "in reply to" comment text, and cleanups.

Okay. To avoid further controversy, I'm placing this patch which is quite a bit
simpler -- it removes all name handling from the code, and uses simply:

  In reply to comment #2

or

  In reply to the bug description

That should be enough to satisfy people. I'll open a bug on adding a title to
comment links.
Attachment #129493 - Attachment is obsolete: true
(Assignee)

Comment 31

14 years ago
Comment on attachment 129829 [details] [diff] [review]
kiko_v9: use simple "in reply to" comment text, and cleanups.

and jaypee can give his blessing too.

I ran this through the w3c validator, and added a tiny hack to make it stop
complaining about the </a> that appears in the document.write statement.
Attachment #129829 - Flags: review?(caillon)
(Assignee)

Updated

14 years ago
Blocks: 216231
Comment on attachment 129829 [details] [diff] [review]
kiko_v9: use simple "in reply to" comment text, and cleanups.

>Index: template/en/default/bug/edit.html.tmpl

>@@ -48,6 +48,63 @@
>   </script>
> [% END %]


If we're going to have one <script> enabled unconditionally here, please
combine the two <script> tags together.

<script>
[% IF ... %]
  ...
[% END %]
  ...
</script>


> 
>+  <script type="text/javascript" language="JavaScript">

is the language attribute really necessary?

>+  <!--
>+  /* Outputs a link to call doReply(); used to reduce HTML output */
>+  function addReplyLink(count) {
>+    document.write('[<a href="#add_comment" onclick="doReply(' + 
>+        count + ');">reply<' + '/a>]');

Please pass integers.  The implicit string conversions will happen where you
need them to.


>+  }
>+
>+  /* Adds the reply text to the `comment' textarea */
>+  function doReply(count) {

count is a poor name for this argument.  comment, commentNum, etc. would all be
better names.

Maybe a good signature would be:

  function replyToComment(id)


>+    /* pre id="comment_name_N" */
>+    var text_elem = document.getElementById('comment_text_'+count);
>+    var text = getText(text_elem);
>+
>+    /* make sure we split on all newlines -- IE or Moz use \r and \n
>+     * respectively */

No platforms use \r\n ?

>+    text = text.split(/\r|\n/);
>+
>+    var newtext = "";

This probably would be better named "replytext"

>+    for (var i=0; i < text.length; i++) {
>+        newtext = newtext + "> " + text[i] + "\n"; 

Use +=.

Also, don't you want to use the correct line ending for the platform/browser to
make sure the wraps are properly fed?

>+    }
>+
>+    if (count == 0) {
>+        /* the description needs special text */

I don't really see why comment #0 needs special text.  It is comment #0. 
Description won't get linked (afaik) but comment #0 will... Note that here you
are using an integer, but passing a string.  Please change the argument as I
noted above (and below).

>+        newtext = "In reply to the bug description:\n" + newtext + "\n";
>+    } else {
>+        newtext = "In reply to comment #" + count + ":\n" + newtext + "\n";


I would prefer if the reply text matched the attachment text style of using
parentheses.  

>+    }
>+
>+    /* <textarea id="comment"> */
>+    var textarea = document.getElementById('comment');
>+    textarea.value = textarea.value + newtext;

Again, +=.


>+
>+    textarea.focus();
>+  }
>+
>+  /* Concatenates all text from element's childNodes. This is used
>+   * instead of innerHTML because we want the actual text (and
>+   * innerText is non-standard) */
>+  function getText(element) {
>+    var child, text = "";
>+    for (var i=0; i < element.childNodes.length; i++) {
>+        child = element.childNodes[i];
>+        /* nodeType 3 is a textNode, which holds text as a nodeValue */

Ack.  Cringe.  Yucky.

IE6 does not define a Node object, but please define one yourself.  Something
like:

if (!Node) {
  var Node = {
    ELEMENT_NODE: 1,
    TEXT_NODE: 3,
    // etc.
  };
}

And then check nodeType == Node.TEXT_NODE

I also think you want to check for Node.ENTITY_REFERENCE_NODE types as well
(&foo;)

That makes the code so much more readable, and less error prone.


>+        if (child.nodeType == 3) {
>+            text = text + child.nodeValue;

More +=

>+        } else {
>+            /* if a non-text node, recurse into it to grab text */
>+            text = text + getText(child);

Still more +=.

>+        }
>+    }
>+    return text;
>+  }
>+  //-->
>+  </script>
>+
> <form name="changeform" method="post" action="process_bug.cgi">
> 
>   <input type="hidden" name="delta_ts" value="[% bug.delta_ts %]">
>@@ -389,7 +446,8 @@
>     <input type="checkbox" name="commentprivacy" value="1"> Private
>   [% END %]
>   <br>
>-  <textarea wrap="hard" name="comment" rows="10" cols="80"
>+  <a name="add_comment"></a>
>+  <textarea wrap="hard" name="comment" id="comment" rows="10" cols="80"
>             accesskey="c"></textarea>
>   <br>
> 
>@@ -575,19 +633,20 @@
> 
> [%# *** Additional Comments *** %]
> 
>+<hr>
> <table>
>   <tr>
>     <td align="left">
>-      <b>
>-        <a name="c0" href="#c0">Description</a>:
>-      </b>
>+      <b><a name="c0" href="#c0">Description</a>:</b>&nbsp;&nbsp;<script 
>+        type="text/javascript" language="JavaScript"><!-- 
>+          addReplyLink('0');

Please pass integer 0.	Implicit string conversions will happen where you need
them.

>+        //--></script>
>     </td>
>     <td align="right" width="100%">
>-      Opened: [% bug.creation_ts FILTER time %]
>+      Opened: [% bug.creation_ts FILTER time %]    

What's the deal with this change?  Extra whitespace sucks.

>     </td>
>   </tr>
> </table>
>-<hr>
> 
> [% PROCESS bug/comments.html.tmpl
>    comments = bug.longdescs
Attachment #129829 - Flags: review?(caillon) → review-
Kiko: I'll be happy to review/deploy this patch when you get through the
"caillon-gauntlet." :-)
(Assignee)

Comment 34

14 years ago
(Praying for [reply])

>If we're going to have one <script> enabled unconditionally here, please
> combine the two <script> tags together.                         

Sure. Moved the open and close tags out. Note that I don't agree with the
indentation here but I chose to follow what was set in the file.

> >+  <script type="text/javascript" language="JavaScript">
> is the language attribute really necessary?

I'm not sure, I just followed the script line in the other block. Should I
remove it?

Changed signatures to pass in integers (what was I thinking?)

Changed count -> id in *reply* functions.

About \r\n: I'm not sure no platforms use \r\n. I haven't been able to find any
cases, and I even removed some code from the first patches that iterated through
the text array removing empty strings because it never got exercised on Moz or
IE. You suggest using the correct line ending to make sure the wraps are
correct, but as far as I can see on both Win and Unix this works correctly. Are
you sure textareas use different linebreaks on different platforms?

I really think parenthesis have worse readability. The style I used is
reminiscent of email replies, which is what most people will be used to (I
expect) -- though you made me remove the name :-P If you stamp your foot I might
reconsider ;)

> I don't really see why comment #0 needs special text.  It is comment #0.
> Description won't get linked (afaik) but comment #0 will...

Hmmm. But nowhere is the description *labelled* as comment #0. So it produces
significant confusion to have a comment 0 that doesn't exist. I'm not sure right
now what to do about it, so I left it as it is, but if you have a good proposal,
shoot. 

> I also think you want to check for Node.ENTITY_REFERENCE_NODE types as well
> (&foo;)

Hmmm. Enlighten me: what would this be used for? The current code seems to work
correctly for entities, at least (see URL).

Fixed the silly whitespace change.

Patch coming up shortly. Tested with IE5 and IE6.
(Assignee)

Comment 35

14 years ago
Created attachment 129955 [details] [diff] [review]
kiko_v10: address caillon's comments
Attachment #129829 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #129955 - Flags: review?(caillon)
Flags: approval?
Flags: approval+
>The style I used is reminiscent of email replies, which is what most people
>will be used to (I expect) -- though you made me remove the name :-P If you
>stamp your foot I might reconsider ;)

/me stamps his foot.  We already have precedence with the attachment replies. 
This is not email, and may cause confusion when replying to bugmails which have
been fwded.

>Hmmm. But nowhere is the description *labelled* as comment #0. So it produces
>significant confusion to have a comment 0 that doesn't exist. I'm not sure
>right now what to do about it, so I left it as it is, but if you have a good
>proposal, shoot.

The labelling is not as important as what it links to.  I think it is more
important to link to the original post than to make it "clear".  Having the user
manually scroll up to comment 0 in a bug is so not acceptable in my mind since
it opens up the potential for taking a quoted comment out of context.

Note that I do not think "Description" is really all that clear to begin with,
because it is so hidden.  Finding "Description" is quite difficult, and is
easily confused to be the Summary.  For ages, I always thought the original
description was just the original summary.  Even now that I know where it is, it
is easy to make the "mistake" that there is no description, because it says

Description:                 Opened: 2003-05-31 05:57
-------------------------------------------------------

To me, that looks like the description field is empty.  So I do not think there
is going to be any less confusion by saying comment 0.  When people click it,
they will realize what it means.  And the fact that it lets people see the
comment being quoted in its entirety wins out, IMO.


>> I also think you want to check for Node.ENTITY_REFERENCE_NODE types as well
>> (&foo;)
>Hmmm. Enlighten me: what would this be used for? The current code seems to work
>correctly for entities, at least (see URL).

Yes, because of browser bugs.  Let's do this right and be ready for when
browsers start supporting entity reference nodes (it may be soon, i think i saw
a patch lying around to do so in mozilla).
Attachment #129955 - Flags: review?(caillon) → review-
(Assignee)

Comment 37

14 years ago
Created attachment 130499 [details] [diff] [review]
kiko_v11: you win

Changes:
  - Use (In reply to comment #XXX) header
  - Add ENTITY_REFERENCE_NODE: 5 and check for it too when grabbing nodevalue
Attachment #129955 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #130499 - Flags: review?(caillon)
Comment on attachment 130499 [details] [diff] [review]
kiko_v11: you win

>Index: template/en/default/bug/comments.html.tmpl

>@@ -41,12 +41,17 @@
>     <div [% "class=\"bz_private\"" IF comment.isprivate %]>
>       [% IF count > 0 %]
>         <br>
>-        <i>------- Additional Comment
>+        ------- <i>Additional Comment
>         <a name="c[% count %]" href="#c[% count %]">#[% count %]</a> From 
>-        <a href="mailto:[% comment.email FILTER html %]">[% comment.name FILTER html %]</a>
>+        <a href="mailto:[% comment.email FILTER html %]">
>+            [% comment.name FILTER html %]</a>

You may want to pre-chomp here, or even just don't change the mailto line at
all since some browsers (Netscape 4) will include the spaces in the anchor
contents.


>         [%+ comment.time FILTER time %] 
>-        -------
>         </i>
>+        [% IF mode == "edit" %]
>+        <script type="text/javascript" language="JavaScript"><!-- 
>+        addReplyLink([% count %]); //--></script>

How about an indent?

>+        [% END %]
>+        -------

>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v
>retrieving revision 1.33
>diff -u -r1.33 edit.html.tmpl
>--- template/en/default/bug/edit.html.tmpl	3 Jul 2003 21:31:37 -0000	1.33
>+++ template/en/default/bug/edit.html.tmpl	28 Aug 2003 03:38:13 -0000
>@@ -24,9 +24,66 @@
> 
> [% PROCESS bug/time.html.tmpl %]
> 
>-[% IF UserInGroup(Param('timetrackinggroup')) %]
>   <script type="text/javascript" language="JavaScript">
>   <!--
>+
>+  /* Outputs a link to call replyToComment(); used to reduce HTML output */
>+  function addReplyLink(id) {
>+    document.write('[<a href="#add_comment" onclick="replyToComment(' + 
>+        id + ');">reply<' + '/a>]');
>+  }

Would it be possible to turn this into something standard, such as
document.createElement() stuff?  It's supported by IE, just not NS4x...  But
since this is a feature, and not required to use the product, I think we should
be striving for standards compatibility rather than quirks.  In fact, since you
are using document.getElementById(), it already will not work in NS4x, so I
would say just go ahead and do that rather than use document.write()


>+  function getText(element) {
>+    var child, text = "";
>+    for (var i=0; i < element.childNodes.length; i++) {
>+        child = element.childNodes[i];
>+        tp = child.nodeType;

You need to declare |tp|.  But |nodeType| may be a better var name.

>+        if (tp == Node.TEXT_NODE || tp == Node.ENTITY_REFERENCE_NODE) {
>+            text += child.nodeValue;
>+        } else {
>+            /* if a non-text node, recurse into it to grab text */

Comment needs updating.

>+            text += getText(child);
>+        }
>+    }
>+    return text;
>+  }
(Assignee)

Comment 39

14 years ago
Comment on attachment 130499 [details] [diff] [review]
kiko_v11: you win

<caillon> so, verbal r=caillon with my nits fixed then.  you wanted someone
else to review it, so go transfer the request.
<caillon> kiko: i am breaking my browser now, so why don't you paste it in for
me?  :-)
Attachment #130499 - Flags: review?(preed)
Attachment #130499 - Flags: review?(caillon)
Attachment #130499 - Flags: review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 40

14 years ago
Checked into trunk:

/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v
 <--  comments.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.34; previous revision: 1.33
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Attachment #130499 - Flags: review?(preed)
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.