490767 - As much validation as possible should happen with JavaScript when filing bugs

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

pyrzak's students' research indicates various problems with validation during bug filing:

* Validation happens for one field at a time, so users fix a field, re-submit the form, and get another error.
* Some validation (short_desc) happens in JS, and some happens as normal Bugzilla errors.

We should do as much validation as possible using JS, so that the user gets immediate feedback before submitting the form, and to be as consistent as possible about how they receive errors. This is more important during bug filing than during bug editing.

Comment 1

[Jesse Ruderman]

I would like this very much.  The most annoying for me is typing someone's old email address in the CC field, and then having to re-attach a file as a result.  (Of course, that could be fixed in many ways: using a better CC widget, not using email addresses to refer to users, letting CCs fail when filing a bug, or keeping my attachments!)

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Here we go. This even knows if a Description is required, based on the current status workflow.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

I noticed a small bug in the previous patch after I attached it.

Comment 4

[Matt Rogers]

Looks fine to me (I don't have permission to add a review+ flag to the attachment).

One thing I don't see is something letting customizers know that if they use the same names/ids for these fields in their customized templated that they'd get the same checking that the default enter_bug.cgi template gets as well.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> One thing I don't see is something letting customizers know that if they use
> the same names/ids for these fields in their customized templated that they'd
> get the same checking that the default enter_bug.cgi template gets as well.

  Well, they wouldn't get it automatically--they'd have to add the function as the onsubmit handler for their form.

Comment 6

[Guy Pyrzak]

Comment on attachment 425736 [details] [diff] [review]
v2

nit, you added a space for no reason above # Get the default from a template.

I'm going to review this more bug I'm going to R- this right now for one reason. we shouldn't be including error messages that just apply to the edit page on every bug (which is effectively what you're doing by including in global/header.html.

I understand the reason why you did it was because of the need to have access to terms.bug. However, i feel the right way to do it is include it as part of YAHOO.bugzilla.terms.bug = [% bug %] and then include the additional javascript where it is needed. Otherwise this strings file will get to be a bit ridiculous and cause every page we transmit to be a bit bigger and take longer to render (b/c we're putting this in the header).

Other things to note:

Using an alert box to show errors is not very user friendly or pretty. here is a Flickr photo of the sort of thing we should be looking for http://www.flickr.com/photos/rosenfeldmedia/2367269324

We should also make sure to distinguish both the input field and the message on the page so it is clear where we are directing users to fix problems.

Ideally a red border around the fields that caused the error along with some text on the page instead of a dialog. In an amazing world the text would appear at the top as well as next to the appropriate field. 

Lastly. The logic in your code will require 2 sets of validation to happen, first the fields which might not be filled in and THEN the attachment. 

P.S. the bug says "as much validation as possible" which would seem to request to validate the username being typed in as seen by jesse's comment.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> I'm going to review this more bug I'm going to R- this right now for one
> reason. we shouldn't be including error messages that just apply to the edit
> page on every bug (which is effectively what you're doing by including in
> global/header.html.

  We will handle this when it becomes a problem. Once it becomes a problem, what we will do is scan the javascript_urls argument for certain files and only include the strings relevant to that file. At this time it's not a problem, so it doesn't need to be solved.

> I understand the reason why you did it was because of the need to have access
> to terms.bug. 

  No, I did it so that the strings can be localized.


> Using an alert box to show errors is not very user friendly or pretty. here is
> a Flickr photo of the sort of thing we should be looking for
> http://www.flickr.com/photos/rosenfeldmedia/2367269324

  That's something we can look into after we check this in. Right now I think that we should just get some client-side validation.

> We should also make sure to distinguish both the input field and the message on
> the page so it is clear where we are directing users to fix problems.

  The script currently focuses the first box on the page that causes the problem.

> Lastly. The logic in your code will require 2 sets of validation to happen,
> first the fields which might not be filled in and THEN the attachment. 

  Yes, that's for modularity.

> P.S. the bug says "as much validation as possible" which would seem to request
> to validate the username being typed in as seen by jesse's comment.

  That's not going to happen as a part of this bug.

Comment 8

[Guy Pyrzak]

It seems strange that we'd rush a UI change that has clear improvements that need to be made or risk introducing new and different usability bugs in order to fix an existing usability bug. We tend to try to fix a bug correctly the first time instead of "just getting" something in there. You've mentioned this to me before numerous times, especially with UI stuff.

If we're not going to fix the username validation in this bug we should file it in a separate bug and link it to this one because I do think that is part of what the HCI survey included as well as it being mentioned by one of our users on this bug specifically.

As far as the modularity, you could easily update the validateAttachments stuff so that it is modular but at the same time doesn't cause a user to see a new error once they have fixed the ones we mentioned the first time around.

So again. R-

Comment 9

[Max Kanat-Alexander]

Yeah, I suppose that's fair enough. This isn't introducing any new usability issues--we already use modal dialogs to inform users of client-side validation errors. But I see your point about stuff. And by the way, all your suggestions are good, I just wanted to check this in and then iterate on it.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 425736 [details] [diff] [review]
v2

Okay, so YUI 3 will have a full form validation framework. I think what we should do is for now, use the alert dialogs for errors, and then switch to the YUI 3 framework when we switch to YUI 3. Does that sound OK?

Comment 11

[:glob ✱]

Comment on attachment 425736 [details] [diff] [review]
v2

i agree with guy's review points, which haven't been addressed.  r-

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> (From update of attachment 425736 [details] [diff] [review])
> i agree with guy's review points, which haven't been addressed.  r-

  Did you see my comment above, when I re-requested review? Right now this is a question of whether we're going to have this patch or not have it at all, for 4.0, because I'm not going to implement a whole validation framework that inserts dom nodes, when we're just going to have another one in the future with YUI 3.

Comment 13

[Max Kanat-Alexander]

glob: See my response above.

Comment 14

[:glob ✱]

> Did you see my comment above, when I re-requested review?

yes.

this isn't a priority bug (the priority field isn't even set), and i still agree with the ui lead; javascript alerts are not pretty.

a "validation framework" is probably overstating it; what's wrong with having a hidden div, which is made visible and innerhtml populated when there's an error?

Comment 15

[Max Kanat-Alexander]

Okay, hidden div will work, that's easy enough. I just didn't want to get complex and put the div next to each field.

Although honestly, a div out of the way, that just inserts a message onto the page, would probably get missed and actually be less useful than an alert.

Comment 16

[Max Kanat-Alexander]

This is so close to being ready and it's such a useful HCI improvement, we really should make it for 4.0.

Comment 17

[Max Kanat-Alexander]

Attachment: v3

Okay, I went ahead and did this really well. :-) It now displays errors next to the fields that are affected, and gives them a bold border.

I didn't use red because it clashes pretty horrifically with the orange-brown used for required fields. I used a blue that's wihin our color scheme instead. (It works with both the Classic and Dusk color scheme, because it's based on the dark blue top header color.)

Comment 18

[Guy Pyrzak]

Comment on attachment 456173 [details] [diff] [review]
v3

I really dislike the layout of the warnings under the multiselect stuff but I understand it has more to do with the bad layout of the page. But the error code is great. But please please please file a bug to fix the layout.

Comment 19

[Max Kanat-Alexander]

  Thanks! :-)

Comment 20

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified js/field.js
modified skins/standard/global.css
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/header.html.tmpl
Committed revision 7337.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified js/field.js
modified skins/standard/global.css
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/header.html.tmpl
Committed revision 7312.

Comment 21

[Max Kanat-Alexander]

BTW, layout bug filed as bug 578263.

Comment 22

[Max Kanat-Alexander]

Added to the release notes in bug 604256.