314036 - Make "Reply" link code use DOM methods and not document.write()

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P4)

Description

[Jeff Walden [:Waldo]]

This looks like it shouldn't be difficult to implement.  The way I currently plan to do this is as follows:

1. Define a function which takes as parameters a DOM node reference and a
   comment number and appends a reply link to the node (probably just a
   modification of |addReplyLink|).
2. Define a function which takes no parameters, creates an array of all
   <span/>s with the class "bz_replylink", and iterates over this array,
   calling function #1 on each element in the array.
3. Make sure function #2 is called during page onload.
4. Replace the current inline JS for reply links with
   <span class="bz_replylink"></span>.

Exact locations for where functions #1 and #2 should be defined and how #3 should be done are as yet uncertain, although edit.html.tmpl and show.html.tmpl look like the likely places.  Someone with more familiarity with the interactions involved in possible invocations of show.html.tmpl should ensure that the final patch doesn't fail on edge cases.

Comment 1

[Teemu Mannermaa (:wicked)]

Are these DOM methods supported in all major browsers?

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Use DOM methods, not document.write

getElementsByTagName, createTextNode, createElement, setAttribute, appendChild, and document.getElementById are all supported in Safari, Mozilla, and Opera (I couldn't find Opera documentation, but everything worked in Opera 8.5 on Linux) and have been for a while.  IE supports all of them, although according to MSDN getElementsByTagName has only been supported since IE5.  This doesn't look like a problem, however, because the className property (which is also new in IE5) is already used in Bugzilla in at least one location.

The ability to display comments reversed, means that comment 0 is wrong; the way I fixed this was to output <span id="bz_replylink_[num]"></span>, where "[num]" is the comment number for the reply link.  This required that I use some regular expressions to determine whether or not a span was a reply link; I checked MSDN's documentation for String.search and String.match to verify that the behavior would be correct in IE.

I also discovered that adding an onload listener in show.html.tmpl won't work for running the reply link generating function, because the function won't be run on "results processed" pages when the next bug in a search list is displayed.  I worked around this by adding a load listener via script to call addReplyLinks; unfortunately it means attachEvent hackery for IE, but I don't see a clean way to call the function without doing so (unless it's always safe to assume window.onload has never been previously set, in which case I could remove six lines from the patch).

On a final note, I haven't tested that the event attaching part of this patch works correctly in IE because my testing environment is Linux-based.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 206503 [details] [diff] [review]
Use DOM methods, not document.write

Looks good, and works well in my testing environment, but someone should test in IE before we check this patch in.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 206503 [details] [diff] [review]
Use DOM methods, not document.write

I got around to setting up a web server with CVS Bugzilla on it to test the patch on Windows:

https://whereswaldo.mit.edu/bz/show_bug.cgi?id=2

The code changes work correctly, and reply links are generated correctly.  Unfortunately, in IE (6, at least) the reply links don't pre-fill any reply text with the patch, even tho I never touched |replyToComment|.  I have no idea why this is happening; the link is generated correctly according to:

javascript:void(alert(document.getElementById('bz_replylink_1').innerHTML));

...and the following also works correctly:

javascript:void(replyToComment(1));

I tried adding an alert immediately inside replyToComment to see where things break, and the alert never happens.  I also tried an alert immediately at the beginning of the emitted onclick, and that similarly did nothing.  I tried replacing the code to call replyToComment entirely, and still the alert didn't happen.

End result: IE's buggy, the patch doesn't work (and probably won't work unless someone's motivated to find a workaround for IE's bugginess).

Comment 5

[:glob ✱]

Comment on attachment 206503 [details] [diff] [review]
Use DOM methods, not document.write


onclick needs a reference to a function not a string..

  var link = document.createElement('a');
  link.setAttribute('i', num);
  link.onclick = function(){replyToComment(this.getAttribute('i'))};
  link.setAttribute('href','#add_comment');
  link.appendChild(text);

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Use the onclick property, not the attribute

(In reply to comment #5)
> onclick needs a reference to a function not a string..

The onclick _property_, yes -- the _attribute_, no.

Oddly enough, tho, switching over to using the onclick property seems to work correctly; this patch is up and works fine in IE and Firefox.

Regarding the change, I don't think for loops create scope in JS, so I couldn't just do |link.onclick = function() { replyToComment(num); };| -- num would be the same (the last calculated value) for all links.  The workaround feels grungy but works fine.

Comment 7

[Frédéric Buclin]

Jeff, when a reviewer reviews a patch, you are not allowed to override this review, even if he granted review and the patch finally has a bug. In this case, simply mark the patch as obsolete, but leave the review on it.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #7)

I don't remember encountering this policy before; thanks for letting me know, and I'll make sure to remember it in the future.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 209334 [details] [diff] [review]
Use the onclick property, not the attribute

>+  /* call addReplyLinks() on page load */
>+  if (window.addEventListener) {
>+      window.addEventListener('load', addReplyLinks, false);
>+  } else if (window.attachEvent) {
>+      window.attachEvent('onload', addReplyLinks);
>+  } else {
>+      window.onload = addReplyLinks;

Nit: it'd be nice if there were comments here explaining which browser/API each block was targeting.

Otherwise looks good and works well.  r=myk

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Patch which documents "load" target browsers

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Real updated patch

I'm betting lynx doesn't like using "~" as a shortcut for a file, but I'm not going to try testing that assumption here. ;-)  Sorry for the previous bugspam...

Comment 12

[Frédéric Buclin]

Fixed by bug 653634.