I've decided to split up my work on bug 373105 into separate patches. This one is the basic one, that just hides the edit form for people who aren't looking for it (which is most users). I also changed the interface on TUI.js to be a bit more sensible. This is the first time we've actually used it in Bugzilla code, and there *is* a comment at the top that says the interface may change. So I've changed it.

A URL where the patch is active is included in the URL field.

Comment on attachment 258079 [details] [diff] [review] v1 Tru, do you feel comfortable enough with JS to review this?

Comment on attachment 258079 [details] [diff] [review] v1 Note to others: navigator.cookieEnabled is not just for Netscape Navigator. It works for Firefox, Internet Explorer, and other browsers (JavaScript The Definitive Guide 5ed p279). Javascript portion - r+ Note: At end of chunk 2, the comment needs to be changed to make sense. It reads... // If cookies availability not checked yet since browser does // not has navigator.cookieEnabled property, let's check it manualy May want to change this to: // navigator.cookieEnabled property unavailable. Check cookies manually. I need more time to finish the rest of this review though other priorities are preventing me from getting to that right now. LpSolit - do you feel good about doing the remainder of the review?

Comment on attachment 258079 [details] [diff] [review] v1 >Index: js/TUI.js > // Cookies are definitely disabled for JS. > if (TUI_getCookie(cookiesuffix).length == 0) >- TUICookiesEnabled = 0; >+ TUICookiesEnabled = true; > else >- TUICookiesEnabled = 1; >+ TUICookiesEnabled = false; > } These changes look wrong to me: 0 -> true and 1 -> false ??

Comment on attachment 258079 [details] [diff] [review] v1 >Index: skins/standard/show_bug.css Please add the license block, as in global.css. >+.bz_bug #bugzilla-body { >+ font-size: small; >+} We use an indentation of 4 spaces in all other CSS files. Please do it here too. >Index: template/en/default/bug/edit.html.tmpl >+ <a href="show_bug.cgi?id=[% bug.bug_id FILTER html %]">[% terms.Bug %] >+ [%+ bug.bug_id %]</a> Use FILTER bug_link() instead. >+ // Writes out a link that controls Maybe it's just me, but I don't understand what this sentence means. >+ document.write('<p class="edit_link">' >+ + "(<a href=\"javascript:TUI_toggle_id('bug', '" + field_id + "');\">" >+ + default_text + '</a>)</p>'); </foo> must be escaped when used with document.write() -> <\/foo>. >+[%# Hide the UI elements that are hidden by default. We have to >+ # do this here, after the elements have already been rendered. %] >+<script type="text/javascript"> >+ var init_states = new Array(); >+ init_states['bug_edit_fields'] = 'collapse'; >+ TUI_tweak('bug', [], init_states); >+</script> http://developer.mozilla.org/en/docs/DOM:window.onload reports that "The load event fires at the end of the document loading process. At this point, all of the objects in the document are in the DOM, and all the images and sub-frames have finished loading.", so this shouldn't come here, but in a function triggered by onload. >Index: template/en/default/bug/simple-table.html.tmpl >+ <th>[% field_descs.product %]</th> >+ <th>[% field_descs.component %]</th> These fields and the following ones must be FILTERed. >Index: template/en/default/global/field-descs.none.tmpl >- "assigned_to" => "Assignee", >+ "assigned_to" => "Assigned To", No. We changed "Assigned To" by "Assignee", as suggested by myk on IRC. Don't change that again. This was a very quick review. I didn't look at it in great details. So more comments may come in the updated patch.

I addressed all of LpSolit's comments, in one way or another. I have a lot of reviews for him in the queue, so Wurblzap--do you think you could get to this in the next few days?

The only thing I forgot to mention is: The <script> element is inline because that way it happens right after the appropriate section is rendered. If we make it an onload, it actually doesn't happen until the whole page is loaded, which sometimes is way too long. Also, I've had experiences with onloads generally not working well.

Oh, the previous version was missing show_bug.css.

Comment on attachment 274153 [details] [diff] [review] v2.1 I'm not a JS guy, so I will never r+ a patch which changes so much code in TUI.js. myk seems the right reviewer for this patch. That being said, here is what I saw so far: - Use relative URLs in show_bug, i.e. omit correct_urlbase. - You say you don't want the bug ID to be crossed out when the bug is resolved, but the alias is and it really doesn't look nice. - I personally don't want this table, at all. Having it displayed at the same time as the editable fields is something I cannot understand. Either add a user pref to let me hide your table in all cases, or hide your table when editable fields are displayed, and only redisplay your table when editable fields are hidden again. Else seeing it all the time will make me crazy very quickly. - Your table is broken when usetargetmilestone is off. Columns are shifted by one because you still try to display the target milestone despite the column header is gone.

Okay, I fixed the actual bugs that LpSolit pointed out. As far as a user preference, that's unnecessary because cookies remember the state of whether or not the fields are displayed. As far as hiding the simple fields when the complex fields are displayed, we can do that in another bug and decide if it's necessary there.

Comment on attachment 281870 [details] [diff] [review] v3 Whoever gets to reviewing this first is fine with me.

Comment on attachment 281870 [details] [diff] [review] v3 This is pretty confusing to look at... The first time I visit a bug, I'm presented with both the small table and the edit fields, with the toggle in the middle between them. I click the edit/show link and the edit table disappears. huh? I think this should show one or the other not both, and the link to toggle should change to reflect which is currently showing. I'm at a tossup whether this is worth having a cookie remembering it... on the one hand, it's an extra step to edit. On the other hand, you're gong to wind up with the edit fields showing until you close it after you've edited one bug, and that might not be the desired behavior if you're just wanting to view the other bugs without editing them (since the short form is easy to read). I also wonder if we still want the help links even on the short version. Just because you're not editing the fields doesn't mean you're not going to want to know what they are.

(In reply to comment #12) > The first time I visit a bug, I'm presented with both the small table and the > edit fields, with the toggle in the middle between them. I click the edit/show > link and the edit table disappears. huh? You must have reviewed a previous version of the patch, or perhaps played with the mock-up in bug 373105. > I think this should show one or the other not both, and the link to toggle > should change to reflect which is currently showing. Yes, I certainly agree with that. I just thought that this was the simpler code change for now. > I'm at a tossup whether this is worth having a cookie remembering it... on the > one hand, it's an extra step to edit. On the other hand, you're gong to wind > up with the edit fields showing until you close it after you've edited one bug, > and that might not be the desired behavior if you're just wanting to view the > other bugs without editing them (since the short form is easy to read). Sure, but if you close it on the next bug, it will remain closed on all the others. > I also wonder if we still want the help links even on the short version. Just > because you're not editing the fields doesn't mean you're not going to want to > know what they are. Oh, yeah, we could do that. That's not too hard.

Okay, this one does what you want! (And what I wanted.) In order to do it reliably, I have to use the DOM. So now we only support IE6+ with this, and any other DOM1 browser. I made it so that if JS is off, or DOM1 isn't supported, everything is shown, so that things still work fine when we don't support DOM1.

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment on attachment 281977 [details] [diff] [review] v4 Canceling this review request, as I'm no longer able to do Bugzilla reviews.

Comment on attachment 281977 [details] [diff] [review] v4 Severe bitrot

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved. I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.