374020 - Improve the bug editing form

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P1)

Description

[Max Kanat-Alexander]

So, I like fieldsets. LpSolit likes fieldsets. However, from reports, some users apparently, don't like them. :-) So, this bug is a general bug for cleaning up the bug editing form. That is, just the fields that show up in fieldsets on show_bug.cgi, right now.

I suspect we'll use a combination of the UI that's on bmo currently and some of our own creations. I also think we should take a look at some of the other common customized Bugzilla UIs, mainly Red Hat, GNOME, and Novell, and see what we can take from each that's ideal.

Comment 1

[Max Kanat-Alexander]

Attachment: Red Hat: Top Half of UI

Here's what the top half of the Red Hat bug editing UI looks like, when you have the right to edit a bug.

Comment 2

[Max Kanat-Alexander]

Attachment: Red Hat: Bottom Half of UI

And here's the bottom half. Right above this is the comment box.

Here's what I think is good abou the Red Hat UI:

* Clear separation between labels and form elements.
* Two-column layout is fairly clean.
* The colors are well-integrated into their general site theme.
* "Save Changes" is probably a better term than "Commit," and I like the
  way their buttons are styled.

Here's what I don't like:

* Having fields after the comments makes them less useful, because they aren't
  seen. For example, the Keywords field may seem like a good candidate to
  put out-of-the-way, because it's just for searching. But it makes it
  less used, because people don't see it. The Status Whiteboard becomes
  almost useless, down there.
* Flags at the bottom is a pain. In general, having to scroll to the bottom
  of the page to make changes is a pain.
* Having the knob at the bottom is a pain.
* Having attachments at the bottom means that they become much less important,
  and also probably less used.

In general, having the UI elements at the bottom is irritating. Also, even though those are long fields, having all of that in a single column isn't good, I don't think. They should just use shorter fields and have two columns. Particularly as wide screens become more popular, horizontal space is easier to come by than vertical space (to a certain extent, of course).

Comment 3

[Max Kanat-Alexander]

Attachment: GNOME: First Half of Editing UI

Here's the first part of the GNOME editing UI, for a bug that I'm the reporter of. It's all at the bottom of the page.

Comment 4

[Max Kanat-Alexander]

Attachment: GNOME: Second Half of Editing UI

Here's the second half of the GNOME UI.

Here's what I like about it:

* Having all of the UI at the bottom puts the description at the very top of 
  the page, which is great when you're reading a lot of bugs. (Unfortunately
  GNOME puts stack traces into the comments, so the first comment is a mile
  long and you can't easily see the discussion without scrolling down.)
* Having *all* of the UI at the bottom isn't as irritating as splitting it,
  because I can still make all of my changes in one place.
* The pre-set comments thing is really cool. :-) (Those links below the
  comment box automatically insert certain text into the box.)
* If you look at the top of the page (I'll attach a screenshot as the next
  item), you'll see the basic bug info off to the right, in a little box.
* The action links at the top of the page are big and hard to miss.
* Once again, we see "Save Changes" and that little flat button. If this
  were a vote, that button would be winning.

Overall, there are a lot of things like like about the UI. 

Some minor nits that I have with it, just in consideration of what we'd like to see upstream:

* The fonts are a bit big, so it is a bit hard to get a lot of the UI on to
  one screen.
* Attachments are at the bottom--I'm not sure how I feel about that.
* The field layout is basically the old Bugzilla 2.22 layout, which is
  pretty cluttered.

Comment 5

[Max Kanat-Alexander]

Attachment: GNOME: Top of Page

Here's the top of the page on a GNOME bug, just so that anybody can get an idea of what I was talking about in that last comment.

Comment 6

[Max Kanat-Alexander]

Attachment: Novell Bug Editing UI

Here's what the editing UI looks like on Novell.

We see Status Changes come first. I'm not sure I really like separating status changes from the rest of the UI.

The <select> boxes must have some overly long values in them, because they're huge. :-)

We see here, once again, a two-column layout. This seems to be pretty popular.

However, the actual editing interface isn't all that different from Bugzilla 2.22--it's a bit cluttered.

The colors are nice, but it would be good to see the field labels have a background color like they do on bugzilla.redhat.com.

This UI is also at the bottom, with the comment box right above what you see here.

Comment 7

[Max Kanat-Alexander]

So, I've CC'ed a number of people on this bug who use all of these interfaces on a daily basis, including the bugzilla.mozilla.org interface.

If you'd like to give your opinions, I'd be really curious to know--what do you like about the interfaces, and what do you find irritating about them? (The ones you use on a daily basis.)

So far, I think the winner is some combination of Red Hat's field/label separation with Mozilla's two-column layout. (And, of course, bug 373105.)

Feel free to CC any other UI experts you'd like, on this bug, and they can give their impressions.

Comment 8

[David Lawrence [:dkl]]

(In reply to comment #7)
> So, I've CC'ed a number of people on this bug who use all of these interfaces
> on a daily basis, including the bugzilla.mozilla.org interface.
> 
> If you'd like to give your opinions, I'd be really curious to know--what do you
> like about the interfaces, and what do you find irritating about them? (The
> ones you use on a daily basis.)
> 
> So far, I think the winner is some combination of Red Hat's field/label
> separation with Mozilla's two-column layout. (And, of course, bug 373105.)
> 
> Feel free to CC any other UI experts you'd like, on this bug, and they can give
> their impressions.
> 


The vote by developers in Red Hat a long time ago was they wanted the comments to be near the top instead of near the bottom. Give the new layout of b.m.o with the comments, moved to beneath the bug attributes near the top, and also move the groups checkboxes below the status change are, then you would pretty much have what our layout is now at bugzilla.redhat.com.

Comment 9

[Max Kanat-Alexander]

The current bug editing form is disliked by many users--we hear lots of complaints. So we are going to fix this before 3.2, no matter what.

Comment 10

[Justin Dolske [:Dolske]]

I think there are 3 general types areas for improvement:

1) Page layout / style -- this is basically what we changed with the bugzilla.mozilla.org cleanup about a year ago, mostly CSS/HTML changes.

2) The blob of radio buttons for changing the state of a bug ("Leave as NEW" et al). I think it can all be direct-entry like the other fields, and simply be removed. EG, make Status/Resolution a <select> like Severity. [IIRC there were concerns about complicating common tasks, but I think that can be mitigated.]

3) Other usability. The main one I'm thinking of was someone's suggestion to add some ajaxy-autocomplete/search to fields which require specific valid values (eg, keywords, anything with a bugzilla username). There's also still a large amount of visual clutter and verbosity that could use some ruthless reduction.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> 1) Page layout / style -- this is basically what we changed with the
> bugzilla.mozilla.org cleanup about a year ago, mostly CSS/HTML changes.

  Yeah, and that's what this bug focuses on. For other improvements, see bug 345674.

> 2) The blob of radio buttons for changing the state of a bug ("Leave as NEW" et
> al). I think it can all be direct-entry like the other fields, and simply be
> removed. EG, make Status/Resolution a <select> like Severity. [IIRC there were
> concerns about complicating common tasks, but I think that can be mitigated.]

  Yes, I agree. That could be done in this bug or a later bug.

> 3) Other usability. The main one I'm thinking of was someone's suggestion to
> add some ajaxy-autocomplete/search to fields which require specific valid
> values (eg, keywords, anything with a bugzilla username)

  We have a cool keyword thing in 3.1.x right now, and the autocomplete stuff I think we have a bug for somewhere. It's one of the things we'll use YUI for.

> There's also still a
> large amount of visual clutter and verbosity that could use some ruthless
> reduction.

  Yes. A lot of that will be helped by making various parts of the UI collapseable and hidden by default. That's bug 373105.

> 

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #11)

>   Yes. A lot of that will be helped by making various parts of the UI
> collapseable and hidden by default. That's bug 373105.

Perhaps, although one does need to be careful doing that... It's often tempting to just hide something rather than to figure out how to simplify it in the first place (out of sight, out of mind).

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> Perhaps, although one does need to be careful doing that... It's often tempting
> to just hide something rather than to figure out how to simplify it in the
> first place (out of sight, out of mind).

  Right. It will be simplified, and then also hidden where that's useful.

Comment 14

[Guy Pyrzak]

Adding this link of mocks to make a note of where I am going with this one. I might combine two. Feel free to post other suggestions. http://people.mozilla.com/~dolske/bugzilla/

Comment 15

[Guy Pyrzak]

Attachment: Top Half before adding some nice css

Comment 16

[Guy Pyrzak]

Attachment: Bottom half pre-pretty css

Comment 17

[Guy Pyrzak]

Attachment: The full Screen shot

Comment 18

[Guy Pyrzak]

Comment on attachment 297220 [details]
The full Screen shot

As requested in meeting

Comment 19

[Guy Pyrzak]

Attachment: The full Screen shot

This has a small text change.

Comment 20

[Guy Pyrzak]

Comment on attachment 297223 [details]
The full Screen shot

as per our meeting

Comment 21

[Max Kanat-Alexander]

Comment on attachment 297223 [details]
The full Screen shot

Okay, a few comments:

* I'm wondering if the label really needs to be "Status/Resolution" or just "Status".

* I'm thinking that having the two importance boxes together might be confusing? Perhaps survey some people who are just reading bugs.

* The CC list is important to casual bug readers, while the Assignee and QA Contact are only important to engineers. So I don't think they need to be put together.

* You probably don't need the Votes twice on the page.

* For the Depends On and Blocks fields, I prefer how they work on bmo now, where they show up as links and the box only shows up if you click (edit).

* Most users will not see the groups, so they shouldn't be so prominent. 

* Flags are much more important than groups.

* Many installations will have no groups and no flags, in which case the right side of the page will seem "wasted" to them, probably.

* If we could edit the status and resolution directly in the form instead of using the knob, that would be great. If you want to save that for another bug, that would be fine.

These aren't all a "hard" r-. They're mostly suggestions and points for discussion.

Comment 22

[Guy Pyrzak]

Attachment: Full screen shot with suggestions

Comment 23

[Max Kanat-Alexander]

Comment on attachment 297471 [details]
Full screen shot with suggestions

This looks pretty good!

* What do you do for moving bugs?

* The (edit) links for Depends On and blocks should probably show up right at the end of the list, otherwise it's too confusing.

* I think the "Show dependency tree / graph" is a bit too small to notice easily.

* Platform isn't bold for some reason?

* I see what you mean about all that text for the groups stuff. Maybe you could make that a popup using the current Help System in Bugzilla, and then you could put those back, at the bottom of the right column? (Which isn't used that much currently other than on query.cgi.)

Comment 24

[Max Kanat-Alexander]

Comment on attachment 297471 [details]
Full screen shot with suggestions

Oh also, where does the dup_id box show up for marking a bug as a duplicate? Or, how does one mark a bug as a duplicate in general?

Comment 25

[Guy Pyrzak]

(In reply to comment #23)
> (From update of attachment 297471 [details])
> This looks pretty good!
> 
> * What do you do for moving bugs?
It show up next to the commit button
> 
> * The (edit) links for Depends On and blocks should probably show up right at
> the end of the list, otherwise it's too confusing.
Agreed
> * I think the "Show dependency tree / graph" is a bit too small to notice
> easily.
Agreed
> * Platform isn't bold for some reason?
Agreed 
> * I see what you mean about all that text for the groups stuff. Maybe you could
> make that a popup using the current Help System in Bugzilla, and then you could
> put those back, at the bottom of the right column? (Which isn't used that much
> currently other than on query.cgi.)
WFM

I'll have a post of the suggested changes in a few.

Comment 26

[Justin Dolske [:Dolske]]

A few drive-by comments...

* Can the Modified date have the seconds trimmed, as with Reported?

* Instead of checkboxes for "Reset Assignee to default" [and QA], perhaps have clearing these fields be the equivalent action?

* The Reporter name should just be the plain name, without the email address appended (mailto: link, though) [On that note, it seems many people use this field for both their name and to add a note. This is kind of a sucky hack, it might be interesting to explore alternate ways of giving people the functionality they're trying to create (eg, standard flags/icons to mark accounts as inactive, on vacation, etc.). Out of scope for this bug, probably. :-)]

* Should comment headers be cleaned up here too?

* Pet peeve: explanatory text. :-) Some already mentioned in the last few comments, some of the existing stuff should be minimized too:
"Add an attachment (proposed patch, testcase, etc.) " --> "Add attachment"
"Summarize time (including time for bugs blocking this bug)" --> "Summarize Time"
"Collapse all comments - Expand all comments" --> one link that toggles text when you click it. Better yet, just remove this entirely. I don't think it's very useful.

* Still a lot of crufty stuff in the header in the top inch of the page. Reworking the reader is presumably also for another bug, but it might make sense to go ahead and remove the redundant info from there.

Comment 27

[Guy Pyrzak]

(In reply to comment #26)
> A few drive-by comments...
> 
> * Can the Modified date have the seconds trimmed, as with Reported?
> 
> * Instead of checkboxes for "Reset Assignee to default" [and QA], perhaps have
> clearing these fields be the equivalent action?
> 
> * The Reporter name should just be the plain name, without the email address
> appended (mailto: link, though) [On that note, it seems many people use this
> field for both their name and to add a note. This is kind of a sucky hack, it
> might be interesting to explore alternate ways of giving people the
> functionality they're trying to create (eg, standard flags/icons to mark
> accounts as inactive, on vacation, etc.). Out of scope for this bug, probably.
> :-)]
> 
> * Should comment headers be cleaned up here too?
> 
> * Pet peeve: explanatory text. :-) Some already mentioned in the last few
> comments, some of the existing stuff should be minimized too:
> "Add an attachment (proposed patch, testcase, etc.) " --> "Add attachment"
> "Summarize time (including time for bugs blocking this bug)" --> "Summarize
> Time"
> "Collapse all comments - Expand all comments" --> one link that toggles text
> when you click it. Better yet, just remove this entirely. I don't think it's
> very useful.
> 
> * Still a lot of crufty stuff in the header in the top inch of the page.
> Reworking the reader is presumably also for another bug, but it might make
> sense to go ahead and remove the redundant info from there.
> 

All sound reasonable. Stay tuned for the update. (except for the note thing :) )

What is the "reader"? Yes reworking the header is another bug, but I'm happy to take that one on as well. a LOT of ideas there. Is there a bug for "cleaning up the header"?

Comment 28

[Max Kanat-Alexander]

(In reply to comment #26)
> * Instead of checkboxes for "Reset Assignee to default" [and QA], perhaps have
> clearing these fields be the equivalent action?

  That won't work, because the QA Contact can actually be cleared and set to blank. In the future you may be able to do the same for the Assignee.

> * The Reporter name should just be the plain name, without the email address
> appended (mailto: link, though) 

  I personally like having the email address visible. It tells me who that person is (what organization they work at) without further UI interaction.

> * Should comment headers be cleaned up here too?

  Not in this bug.

> "Collapse all comments - Expand all comments" --> one link that toggles text
> when you click it. Better yet, just remove this entirely. I don't think it's
> very useful.

  I agree that this feature is useless.

> * Still a lot of crufty stuff in the header in the top inch of the page.
> Reworking the reader is presumably also for another bug, but it might make
> sense to go ahead and remove the redundant info from there.

  Yeah, for another bug.

Comment 29

[Frédéric Buclin]

(In reply to comment #28)
> > "Collapse all comments - Expand all comments" --> one link that toggles text
> > when you click it. Better yet, just remove this entirely. I don't think it's
> > very useful.
> 
>   I agree that this feature is useless.

This feature is not useless at all, and we just implemented it in Bugzilla 3.2 a few months ago in bug 262275. I will put my veto if the new UI removes it.

Comment 30

[Max Kanat-Alexander]

(In reply to comment #29)
> This feature is not useless at all, and we just implemented it in Bugzilla 3.2
> a few months ago in bug 262275. I will put my veto if the new UI removes it.

  This bug will not touch comments.

  We can discuss this in another bug.

Comment 31

[Guy Pyrzak]

Attachment: Full SS after suggestions

Removing the knob is another patch as is changing the comments as is basically everything. I left the help there but a VERY small plugin could turn it into help text via YUI.

Comment 32

[Guy Pyrzak]

Attachment: Full SS after suggestions

Removing of knob is a separate bug as is editing the comments as is adding the help system to this page.

Comment 33

[Max Kanat-Alexander]

Comment on attachment 297643 [details]
Full SS after suggestions

Looks good to me! The "Show dependency tree / graph" text should possibly be a bit more left-aligned since it's hard to see how it's related when the dependson and blocks lists are small (as they usually are).

Comment 34

[Max Kanat-Alexander]

Comment on attachment 297643 [details]
Full SS after suggestions

Oh actually, one other thought, too--I think the Assignee/QA Contact block is more important than the URL, keywords, etc, and so should probably be higher up on the page.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 297643 [details]
Full SS after suggestions

Oh, and I kind of do want the Assignee and QA Contact to have (edit) links just like the other stuff does now, so that it can show the person's full name.

Also, version is a function of product and component, so maybe should be higher up. Right now it looks like it's a function of OS.

And perhaps status should be somewhere different?

Comment 36

[Guy Pyrzak]

Also suggested was making the URL follow the click to edit construct with the URL appearing as a hyperlink.

Comment 37

[Guy Pyrzak]

Attachment: Full SS with the suggestions that max made and others.

Comment 38

[Guy Pyrzak]

Attachment: Patch for the changes shown in the previous Screen Shot

Yippe!!

Comment 39

[Max Kanat-Alexander]

Comment on attachment 297677 [details]
Full SS with the suggestions that max made and others.

I just want to say: this is beautiful. :-)

Comment 40

[Max Kanat-Alexander]

Comment on attachment 297679 [details] [diff] [review]
Patch for the changes shown in the previous Screen Shot

  This is a really great patch, especially for your first really big submission, it's incredibly impressive.

  I do have some comments, though:

>Index: bugzilla/skins/standard/global.css
>+body, td, th, input {
>+  font-family:verdana,sans-serif;

  Nit: Verdana

  Also, is it actually necessary to specify "td, th, input"?

>+  font-size:small;
>+}

  This should probably be at the top of the file, since it affects everything.

>+#header .txt, #header .btn {
>+  font-size:100%;
>+}

  And this should be grouped with the other things that affect the header.

  Also, spaces after the colons.

>Index: bugzilla/template/en/default/attachment/created.html.tmpl
>-  style_urls = [ "skins/standard/yui/calendar.css" ]
>+  style_urls = [ "skins/standard/yui/calendar.css"]

  No real change here.

>Index: bugzilla/template/en/default/bug/comments.html.tmpl
>-                [% " bz_first_comment" IF count == description %]">
>+                [% " bz_first_comment" IF count == description %]"><a name="c[% loop.count %]"></a>

  This looks likely to be unrelated to this patch.
>Index: bugzilla/template/en/default/bug/edit.html.tmpl
>   <script type="text/javascript">
>   <!--
>-
>+  /* Hide input fields and show the text with (edit) next to it */

  Okay. Since this whole JS block doesn't contain any template variables, it needs to live inside js/field.js.

>+  function hide_input_field_display_container( container, input, action, field_id, original_value ){

  I generally prefer a four-space indentation for JavaScript, even though two spaces is our standard for other HTML-related things.

>+    YAHOO.util.Event.addListener(action, 'click', function(e, array){

  Nit: Space before the open-bracket.

>+      YAHOO.util.Dom.setStyle(array[1], 'display', 'inline');
>+      YAHOO.util.Dom.setStyle(array[0], 'display', 'none');
>+      YAHOO.util.Event.preventDefault(e);
>+    }, new Array(container, input, action ) );

  Maybe set a variable to that function expression and then just pass the variable to the function--I think that'd be easier to read.

>+      if(el){

  Nit: "if (el)" (Always put a space after if, for Bugzilla code style.)

>+        //window.alert("el.value>" + el.value + "   :   array[4]>" +  array[4]);

  Remove the debugging line. :-)

>+        if(el.value != array[4]){

  "array" is kind of a confusing variable name, and array[4] doesn't make things any easier. :-)

>@@ -137,85 +159,161 @@
>-<form name="changeform" method="post" action="process_bug.cgi">
>+<form name="changeform" method="post" action="process_bug.cgi" >

  Nit: No need to add that space.

>+  <div style="margin: 8px 0px; padding: 0.3em; background-color: rgb(208, 208, 208); -moz-border-radius-topleft: 0.5em; -moz-border-radius-topright: 0.5em; -moz-border-radius-bottomright: 0.5em; -moz-border-radius-bottomleft: 0.5em;font-size:125%; font-weight:bold;">

  style="" is a big no-no. :-) Put that in a CSS file.

>+     <a href="[% urlbase FILTER html %]show_bug.cgi?id=[% bug.bug_id %]">

  Is there some reason to put urlbase there? Doesn't seem like it'd be necessary.

>+     <span id="nonedit_display_container" style="display:none;"> 

  Once again, no style=. Also, doesn't setting this to "none" by default mess things up for users without JS?

>+      [% IF Param("usebugaliases") %]
>+        [% IF bug.alias != "" %]

  Nit: You could just do "IF bug.alias", since an alias of "0" is illegal.

>+      <small class="editme" >(<a href="#" id="editme_action">edit</a>)</small>

  Nit: No space at the end of the HTML tag.

>+          <label for="alias" title="a name for the [% terms.bug %] that can be used in place of its ID number, f.e. when adding it to a list of dependencies">Alias</label>:&nbsp;

  Nit: "e.g." is a more standard construction than "f.e.", though "f.e." might actually make more sense to some people...

>+          [% PROCESS input inputname => "short_desc" size => "60" colspan => 2
>+                                   maxlength => 255 spellcheck => "true" no_td => 1 %]

  Nit: Align "inputname" and "maxlength" there.

>+  <script type="text/javascript">
> [snip]
>+      // also check for the alias being different    

 This block looks like it should go into a function somewhere else. Any inline JS that's more than a few lines should probably get a function or something.

>+      YAHOO.util.Event.addListener(window, 'load', function(e, array ){

  Remember that if a bug has 700 comments, it could take quite a long time for the load event to fire.

>+  <table >

  Nit: Space.

>+          <table >

  And here. I'm going to stop mentioning it, but just fix it everywhere you find it.

>+            [%# *** ID, product, component, status, resolution, Hardware, and  OS *** %]
>+            [% PROCESS section_details1 %]

  Ooh, I like how this is all in PROCESS blocks. It makes it really easy to read! :-) Thank you!

>+      <td >
>+        <div style="width:2em;">&nbsp;</div>

  style=

>+      [%# Force the layout to be displayed now, before drawing the second column of the table.
>+        # This should prevent bug 370739 when using Firefox 2. %]
>+      <script type="text/javascript">
>+        <!--
>+        var v = document.body.offsetHeight;
>+        //-->
>+      </script>

  Do we still need that with this new layout? I think we're fine now.

>+        <table cellpadding="3" cellspacing="1">

  Hrm, were cellpadding and cellspacing in here before? I'd really like to remove them and use CSS instead, if possible.

>+                <input type="checkbox" id="addselfcc_first" name="addselfcc"
>+                    [% " checked=\"checked\""

  Nit: Just so you know, you can do ' checked="checked"' which often looks a bit cleaner.

>+                         IF user.settings.state_addselfcc.value == 'always'
>+                            || (!has_role
>+                                && user.settings.state_addselfcc.value == 'cc_unless_role') %]

  Or if they're factually in the CC list, right? Even if they have "never" set, this should be checked if they *are* in the CC list.

><label for="addselfcc_first">Myself (<a href="mailto:[% user.name FILTER html %]<[% user.login FILTER html %]>">[% user.name FILTER html %]</a>)</label><br> 

  Shouldn't that be a [% PROCESS user_identity %] (or whatever that block is called) instead?

>+              [% bug.cc.size %]&nbsp;total users.<span id="cc_edit_area_showhide_container" style="display:none;">(<a href="#" id="cc_edit_area_showhide" >edit</a>)</span>

  Ohh, style=! :-)

>+                <div>
>+                  <div>
>+                    <label for="cc">
>+                      <b>Add</b>

  Hrm, you moved this below the list of current CCs? Any particular reason?

>+      <td colspan="3">
>+          <div id="bz_top_half_spacer"><hr></div>
>       </td>

  An <hr> is already a block element. Any reason to put it inside a div?



>+    [%######### PRODUCT  #################%]

  Any particular reason for the uneven #'s? (And all the other places that happens.)


>     <tr>
>       <td align="right">
>         <label for="component" accesskey="m"><b><a href="describecomponents.cgi?product=[% bug.product FILTER url_quote %]">Co<u>m</u>ponent</a></b></label>:

  Very long lines should be avoided, when possible.

>+      <td nowrap >

  nowrap="nowrap". Is that a valid HTML Transitional item, also? That seems like it really should be done in CSS, not in HTML. (white-space: nowrap, and all that)

>+          <div id="bz_assignee_edit_container" style="display:none;"><span>[% INCLUDE user_identity user=> bug.assigned_to %](<a href="#" id="bz_assignee_edit_action">edit</a>)</span></div>

  style=.

>+          [% IF bug.qa_contact != "" %]
>+           <div id="bz_qa_contact_edit_container" style="display:none"><span><span id="bz_qa_contact_edit_display">[% INCLUDE user_identity user=> bug.qa_contact %]</span>(<a href="#" id="bz_qa_contact_edit_action">edit</a>)</span></div>

  style=. Also, very long line. (I'm going to stop mentioning both of these from here on out--just fix them all.)

>+   [% IF false  %]

  IF false? What kind of a variable name is that? :-) (Looks like you just forgot to remove this section?)

>+[% BLOCK section_url_keyword_whiteboard %]
>+[%# *** URL Whiteboard Keywords *** %]
>+
>+            <tr>

  Way too much indentation, here. Just indent it relative to the BLOCK, not to where the block is processed.

>+[%############################################################################%]
>+[%# Block for Restrict Visibility                                            #%]

  "Restricting"?

>+          <b>
>+            Only members of a group can change the visibility of [% terms.abug %] for
>+            that group

  Period at the end of that sentence. (Probably not your error, but it wouldn't hurt to fix it here.)

>+    <tr>
>+     [% IF inagroup %]
>+      <td align="right" valign="top">
>+        <label id="bz_enable_role_visibility_label"><b>Enable Role Visibility</b>:</label>
>+      </td>
>+      <td>
>+       
>+          <div id="bz_enable_role_visibility_help">

  Weird indentation all around, here. Sometimes one space, then four?

>+        [% bug.delta_ts FILTER time | replace('[:]..$', '') | replace('[:].. ', ' ')%]

  You don't need to wrap the colon in brackets. A better regex there is ":\d\d " I would think. (That also makes it clearer what you're doing.)

  It might be better just to modify "FILTER time" to take a precision argument.

  Also, do FILTER time FILTER replace, which is what we usually do.

>+        hide_input_field_display_container(

  You know, thinking about it, you could probably remove the word "display_" from that container and save some typing and screen space.

>@@ -828,7 +998,9 @@
> [% BLOCK select %]
>+  [% IF no_td != 1 %]

  Do "IF NOT no_td", I think, instead. Also, it's better to always have *positive* booleans, so "td" would be a better parameter than "no_td".

>Index: bugzilla/template/en/default/bug/navigate.html.tmpl
>+[%# ALSO WANT TO ADD the XML etc to here for easier use %]

  Is that supposed to be an XXX comment to be removed later, or something?

>+[% IF bottom_navigator == 1 %]
>+  <style type="text/css">
>+    .related_actions{font-size: 0.85em; float: right;}
>+  </style>

  That should be in a separate CSS file, not inline here.

>+  <span class="related_actions">
>+    <a href="show_bug.cgi?format=multiple&amp;id=[% bug.bug_id FILTER url_quote %]">Format For Printing</a>
>+    &nbsp;-&nbsp;<a href="show_bug.cgi?ctype=xml&amp;id=[% bug.bug_id  FILTER url_quote %]">XML</a>
>+    &nbsp;-&nbsp;<a href="enter_bug.cgi?cloned_bug_id=[% bug.bug_id  FILTER url_quote %]">Clone This [% terms.Bug %]</a>
>+    [%# Links to more things users can do with this bug. %]
>+    <div class="related_actions">
>+    [% Hook.process("links") %]
>+    </div>
>+    &nbsp;-&nbsp;<a href="#">Top of page </a>
>+    </span>

  Hrm, maybe make it a <ul> instead of a <span>? That's a more standard menu construct.

>+[% ELSE %]
>+  [% IF bug.longdescs.size > 0 %]
>+    <span style="float: right; font-size: 0.85em;">

  style=.

>Index: bugzilla/template/en/default/global/userselect.html.tmpl

  I don't exactly understand what you changed in this file, other than the indentation.

Comment 41

[Albert Ting]

The screenshot looks great.  But could the classification also be listed as a label?  In other words, instead of "Product", perhaps "[<class-name>] Product"?

Comment 42

[Frédéric Buclin]

Why does the QA field appear as a text field while the assignee field appears as plain text with an "(edit)" link? Also, how do I reset the assignee to default?

Also, instead of (Activity), I think (History) would be better.

Comment 43

[Max Kanat-Alexander]

(In reply to comment #41)
> The screenshot looks great.  But could the classification also be listed as a
> label?  In other words, instead of "Product", perhaps "[<class-name>] Product"?

  It is, in the patch. I think he just took a screenshot without a Classification.

(In reply to comment #42)
> Why does the QA field appear as a text field while the assignee field appears
> as plain text with an "(edit)" link?

  I think that's because the QA Contact is blank (which makes sense to me), but I could be wrong.

>  Also, how do I reset the assignee to default?

  It's in the (edit) link.

> Also, instead of (Activity), I think (History) would be better.

  Oh, I agree. :-) Although old users of Bugzilla are kind of used to the word "Activity". But "History" is better.

Comment 44

[Guy Pyrzak]

> (In reply to comment #42)
> > Why does the QA field appear as a text field while the assignee field 

You are correct. I thought it was confusing to have a blank field.

I'm happy to change it to history.

Comment 45

[Frédéric Buclin]

(In reply to comment #44)
> You are correct. I thought it was confusing to have a blank field.

Display (none) instead.

Comment 46

[Guy Pyrzak]

Attachment: Full SS, basically the same thing as before.

Comment 47

[Guy Pyrzak]

Attachment: Le Patch!

Here it is with max's suggested changes.

Comment 48

[Max Kanat-Alexander]

Comment on attachment 298777 [details] [diff] [review]
Le Patch!

>Index: bugzilla/js/field.js
>+/* Hide input fields and show the text with (edit) next to it */  
>+function hide_input_field_display_container( container, input, action, field_id, original_value ) {

  That's like, the longest function name ever. Could we make it "hide_editable_field" instead?

>+function hideFieldShowEditLink (e, ContainerInputArray) {

  "hideFieldShow" is confusing. Also, decide between CamelCaps and underscore_names. (Use whatever the rest of this file uses.)

  Also, isn't this code kind of duplicated with the above function?

  And also, you could adopt a consistent naming scheme for these and then you wouldn't have to pass an array of names.

>Index: bugzilla/skins/standard/global.css
>+    body, td, th, input {

  Did I get an answer on why "td, th, input" are necessary?

>     #header .btn,
>     #header .txt {
>-        font-size: 80%;
>+        font-size: 100%;

  That looks unrelated.

>Index: bugzilla/template/en/default/bug/edit.html.tmpl
>+  // check the short desc field
>+  hide_input_field_display_container( 'nonedit_display_container', 'edit_display_container', 'editme_action', 'short_desc', '[% bug.short_desc FILTER js %]');  

  Ahhhhh! Attack Of The Long Lines!!

>+      <td valign="top">     

  Man, valign makes me cry. (I understand it was already there.)


>+          <tr>
>+              <td align="right" valign="top">
>+                <label for="newcc" accesskey="a"><b>CC List</b>:</label>

  Shouldn't this get something like section_cclist?

>+              [%IF bug.cc.size %]
>+                [% bug.cc.size %]
>+              [% ELSE %]
>+                0
>+              [% END %]

 [% bug.cc_size || 0 %] doesn't work, eh?

>+              <script type="text/javascript">
>+                hide_input_field_display_container( 'cc_edit_area_showhide_container', 'cc_edit_area', 'cc_edit_area_showhide', '', '');  

  Another one! (Long line.)

>+[%#########  STATUS  #########%]

  By the way, I usually find this construction stands out a bit better:

  ##########
  # Status #
  ##########

> [snip]
>+      <td nowrap>

  Oh no, it's old-school HTML, come to eat us! (nowrap="nowrap". Also, that's a presentational attribute that would be better replaced with CSS.)

>+        [% IF bug.bug_file_loc 
>+           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]

  That's kind of a weak attempt at security. Was that there before? This assumes that only those two URL schemes will ever be dangerous, for all time.


>+      [%%]</b></label>:

  Whazzat?

>+[% BLOCK section_spacer %]
>+  <tr>
>+    <td colspan="2">
>+      &nbsp;<br>

  Instead of &nbsp;<br>, could we do that with CSS?

>+    <span id="[% dep.fieldname %]_input_area">

  Do we have to give these all (on the whole page) individual ids, or can they just all have a single class that hides them?

>Index: bugzilla/template/en/default/bug/navigate.html.tmpl
>+    <li><div class="related_actions">
>+    [% Hook.process("links") %]
>+    </div></li>

  Why wrap that in <li>? Couldn't we just have people make their related_actions into <li> tags?

>--- /dev/null	2008-01-23 13:18:23.000000000 -0800
>+.alias_title_container {
>+  margin: 8px 0px; 

  Four spaces indentation for CSS.

  "0" instead of "0px" is more standard.

>+  -moz-border-radius-topleft: 0.5em; 
>+  -moz-border-radius-topright: 0.5em; 
>+  -moz-border-radius-bottomright: 0.5em; 
>+  -moz-border-radius-bottomleft: 0.5em;

  Um, shouldn't those all be combined as -moz-border-radius?

> font-size:125%; 

  And that should be on a separate line, and have a space after the colon.

  Make sure all the directives have a space after the colon.

Comment 49

[Guy Pyrzak]

(In reply to comment #48)
> >Index: bugzilla/skins/standard/global.css
> >+    body, td, th, input {
> 
>   Did I get an answer on why "td, th, input" are necessary?
because some browsers (ie6) don't like to like their table inheriting td and th from the body. This fixes that and beats it into submission.

> >     #header .btn,
> >     #header .txt {
> >-        font-size: 80%;
> >+        font-size: 100%;
> 
>   That looks unrelated.
This is because at 80% the text in the input and the button become unreadable (at least in firefox)

Comment 50

[Guy Pyrzak]

(In reply to comment #48)
>>+        [% IF bug.bug_file_loc 
>>+           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
>
>  That's kind of a weak attempt at security. Was that there before? This
>assumes that only those two URL schemes will ever be dangerous, for all time.
>
>
>>+      [%%]</b></label>:
>
>  Whazzat?
>
All that was already there. I just copied it.

Comment 51

[Guy Pyrzak]

(In reply to comment #48)
>>+    <span id="[% dep.fieldname %]_input_area">
>
>  Do we have to give these all (on the whole page) individual ids, or can they
>just all have a single class that hides them?

yeah they need their own ids so we can show/hide them correctly.

Comment 52

[Frédéric Buclin]

(In reply to comment #48)
> >+        [% IF bug.bug_file_loc 
> >+           AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
> 
>   That's kind of a weak attempt at security. Was that there before? This
> assumes that only those two URL schemes will ever be dangerous, for all time.

This has been implemented since 2.16 as a security fix, see bug 250730.

Comment 53

[Guy Pyrzak]

Attachment: Patch v3

had to alter the test case to not be upset about filter collapse.

Comment 54

[Guy Pyrzak]

Attachment: Patch v4

forgot some spaces in show_bug.css but that's it.

Comment 55

[Guy Pyrzak]

Attachment: With and without font-size:100 on firefox

Comment 56

[Max Kanat-Alexander]

Comment on attachment 298973 [details] [diff] [review]
Patch v4

>Index: bugzilla/t/008filter.t
>     return 1 if $directive =~ /FILTER\ (html|csv|js|base64|url_quote|css_class_quote|
>                                         ics|quoteUrls|time|uri|xml|lower|html_light|
>-                                        obsolete|inactive|closed|unitconvert|
>+                                        obsolete|inactive|closed|unitconvert|collapse|

  Nooooooooo. That means that suddenly 008filter.t will think that "collapse" makes unsafe data safe!!

  The line you need to modify is:

  return 1 if $directive =~ /^(IF|END|UNLESS|FOREACH|PROCESS|INCLUDE|

>@@ -401,6 +224,17 @@
>+      [%# *** Additional Comments *** %]
>+
>+<hr>

  Spacing, there?

>+    [%##############################%]    
>+    [%#########  PLATFORM  #########%]
>+    [%###############################%]    

  Nit: One too many #'s at the bottom, there.

>+[%############################%]
>+[%#########  STATUS  #########%]
>+[%############################%]

  FWIW, I always dislike the ##### Something #### because then when I add new sections like that I have to get all worried about how to center my text.

>+  [% IF user.id %]
>+  <tr>

  That should be indented, no?

>       [% ELSE %]
>-        <td colspan="2"><input type="hidden" name="cc" value=""></td>
>+        <input type="hidden" name="cc" value="">

 What is that there for, do you know? (I see and understand it was there before.)

>+[% BLOCK section_customfields%]

  Nit: space

>+  <table cellspacing="0" cellpadding="4" border="1">
>+    <tr>
>+      <th align="center" bgcolor="#cccccc">

  Man, I just noticed this has bgcolor and "align". :-( And cellpadding and border! We don't have to fix it here, but if we don't, could you file a separate bug for it?

>+        <a class="email" href="mailto:[% user.email FILTER html %]" title="[% user.email FILTER html %]"
>+          ><span class="fn">[% user.name FILTER html %]</span
>+        ></a>

  What's up with that >? Oh, to keep the alignment. Well, okay. :-)

>Index: bugzilla/template/en/default/bug/navigate.html.tmpl
>+[% IF bottom_navigator == 1 %]
>+  <ul class="related_actions">
>+    <li><a href="show_bug.cgi?format=multiple&amp;id=[% bug.bug_id FILTER url_quote %]">Format For Printing</a></li>

  These lines are really long. Since TT always pre-chomps whiespace for us in Bugzilla, you could move bug.bug_id on to the next line, if you wanted.

>+[% bottom_navigator = 0 %]

  Whazzat? Why is that in here? This smacks of a design problem.


> show_bug.css:
>
>+.bz_nowrap {
>+    white-space: nowrap;
>+}

  That's a very presentational class name.

>+.bz_field_label {
>+    text-align: right;
>+}

  We already have .field_label in global.css.

>+.bz_field_label_multiline {
>+    text-align: right;
>+    vertical-align: top;

  Why not just always valign all field_value tags top?

Comment 57

[Guy Pyrzak]

(In reply to comment #56)
>>+.bz_field_label_multiline {
>>+    text-align: right;
>>+    vertical-align: top;
>
>  Why not just always valign all field_value tags top? 

A. Because it's the label not the value.

B. Because v aligning top all the field labels looks funny. 

Comment 58

[Guy Pyrzak]

(In reply to comment #56)
>>       [% ELSE %]
>>-        <td colspan="2"><input type="hidden" name="cc" value=""></td>
>>+        <input type="hidden" name="cc" value="">
>
> What is that there for, do you know? (I see and understand it was there
>before.)

The only thing i can think of is that bugzilla might get upset if that field isn't there, even if it is blank?

Comment 59

[Max Kanat-Alexander]

(In reply to comment #58)
> The only thing i can think of is that bugzilla might get upset if that field
> isn't there, even if it is blank?

  Ah, that used to be the case, but isn't anymore, so I'm guessing you can remove it.

Comment 60

[Guy Pyrzak]

Attachment: Patch v5

Comment 61

[Guy Pyrzak]

Attachment: Patch v6

Comment 62

[Guy Pyrzak]

Attachment: patch v7

replaced field_label_multiline with field_label (i missed it)

Comment 63

[Guy Pyrzak]

Attachment: Patch v8

fix for bitrot

Comment 64

[Max Kanat-Alexander]

Comment on attachment 299330 [details] [diff] [review]
Patch v8

  Okay, on testing I have a few comments:

  * When you're logged out, there's no space between OS and Platform.

  * The word "Flags:" shouldn't appear if there aren't any flags that appear on the bug.

  * Let's change "Activity" to "History".

  * QA Contact appears as an editable box if it's empty, but URL doesn't.

  * Say "including you" instead of "including Me", I think.

  * You moved the comment box to the bottom of the screen without a user preference for where it goes? That's not part of this bug.

  Everything else looks fine. :-)

Comment 65

[Guy Pyrzak]

Attachment: Patch v9

Comment 66

[Max Kanat-Alexander]

Comment on attachment 299351 [details] [diff] [review]
Patch v9

The edit boxes for the summary & alias get shown if just the alias is empty, which is a problem.

Otherwise, this is great. :-)

Comment 67

[Max Kanat-Alexander]

Comment on attachment 299351 [details] [diff] [review]
Patch v9

Also, FILTER url_quote is not the right filter for full URLs. It makes them unusuable.

I think you should be using the "url" filter instead, if that exists in TT 2.15.

Comment 68

[Frédéric Buclin]

(In reply to comment #67)
> I think you should be using the "url" filter instead, if that exists in TT
> 2.15.

No, use |FILTER html| when you want to filter a full URL.

Comment 69

[Guy Pyrzak]

(In reply to comment #68)
> (In reply to comment #67)
> > I think you should be using the "url" filter instead, if that exists in TT
> > 2.15.
> 
> No, use |FILTER html| when you want to filter a full URL.
> 
Doh, that's what it was and we changed it! That's what i get for trying to be clever.

Comment 70

[Guy Pyrzak]

Attachment: Patch v10

come on lucky number 10... made 2 changes switched the filters back to html for URL stuff. also added a conditional in field.js that it shouldn't show the summary and alias fields if alias is blank. This does make me concerned that people won't know where to find the alias field if it is blank though, small usability concern.  But I couldn't think of a fix for this moment and alias seems like an advanced-ish feature... maybe?

Comment 71

[Max Kanat-Alexander]

Comment on attachment 299423 [details] [diff] [review]
Patch v10

Yes, the alias thing is fine, because it's discovered any time you edit the summary, which is a very common action.

However, now when you are logged out there's an (edit) link for the Summary line, when there's a blank alias. (Maybe it always happens, but I only noticed it with a blank alias.)

Also, it's not necessary to show the label "Alias" or "Summary" in the header for logged out users (or ever, really). At the very least, the "Alias" label shouldn't show up if the alias is blank.

By the way, when you attach patches that need to be applied with -p1 instead of -p0, make sure to specify that in the description: "(-p1)" or "(use -p1)".

Comment 72

[Guy Pyrzak]

Attachment: Patch v11

I don't know how to use patch well, but i ran patch from the bugzilla directory this time so i think that makes it a -p0? maybe? and um... i hate CVS :)

Comment 73

[Guy Pyrzak]

Attachment: Patch v12

added show_bug.css and yay for cvsdo!

Comment 74

[Max Kanat-Alexander]

Comment on attachment 299588 [details] [diff] [review]
Patch v12

Looks good to me! :-)

Comment 75

[Max Kanat-Alexander]

I made one small change on checkin--I fixed the Dusk .cvsignore so that this file won't show up at the top of diffs for the Dusk directory.

Checking in js/field.js;
/cvsroot/mozilla/webtools/bugzilla/js/field.js,v  <--  field.js
new revision: 1.3; previous revision: 1.2
done
Checking in skins/contrib/Dusk/.cvsignore;
/cvsroot/mozilla/webtools/bugzilla/skins/contrib/Dusk/.cvsignore,v  <--  .cvsignore
new revision: 1.3; previous revision: 1.2
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.43; previous revision: 1.42
done
Checking in skins/standard/show_bug.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/show_bug.css,v  <--  show_bug.css
new revision: 1.4; previous revision: 1.3
done
Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.27; previous revision: 1.26
done
Checking in template/en/default/attachment/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v  <--  created.html.tmpl
new revision: 1.19; previous revision: 1.18
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.110; previous revision: 1.109
done
Checking in template/en/default/bug/navigate.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/navigate.html.tmpl,v  <--  navigate.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/bug/show.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v  <--  show.html.tmpl
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/bug/create/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/created.html.tmpl,v  <--  created.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/bug/process/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.9; previous revision: 1.8
done

Comment 76

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.