Closed Bug 207754 (bugreplies) Opened 22 years ago Closed 21 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, enhancement)

Product:
Bugzilla
Component:
Creating/Changing Bugs
Version:
2.17.4
Platform:
x86
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.18

People

(Reporter: kiko, Assigned: kiko)

References

(
URL
)

Details

Attachments

(1 file, 8 obsolete files)

kiko_v11: you win
5.11 KB, patch
kiko
: review+
Details | Diff | Splinter Review
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 :-)
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
Alias: bugreplies
Severity: normal → enhancement
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.18
> 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
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.
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
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.
Attachment #124649 - Attachment is obsolete: true
Attachment #124672 - Flags: review?(bbaetz)
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
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.
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
Attachment #124672 - Flags: review?(bbaetz)
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-
(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
(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
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
Finally ;) I'll cook up a version with UI for replying to the initial comment.
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
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+
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
Flags: approval+
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:"
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
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)
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." :-)
(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.
Attachment #129829 - Attachment is obsolete: true
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-
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
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; >+ }
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+
Flags: approval?
Flags: approval? → approval+
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
Closed: 21 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: