418895 - Assignee and QA contact fields always become revealed (editable)

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Guy Pyrzak]

This is caused by the reassign to default checkbox value being 1 and that not matching the value that it is being compared to the wrong value.

This is also a problem because it is checking the wrong value in the JS. If the value being compared is a checkbox it should check if the box is checked or not.

Comment 1

[Guy Pyrzak]

Another bug i found related to this (why i didn't find it in testing it the first time around). This won't happen if the QA field is disabled b/c it doesn't go through the whole function. So... I need to fix that as part of this patch.

Comment 2

[Guy Pyrzak]

Attachment: Patch v1

Also made some fixes to the comments on the blocks in edit and added myself to the contributor lists :)

Comment 3

[Max Kanat-Alexander]

Comment on attachment 304829 [details] [diff] [review]
Patch v1

Looks good. Fix some of those super-long JS lines when you check this in, though. Lines should be limited to 80 characters.

Also, change that one comment to "Depends On / Blocks" instead of "Depends on blocks"

Comment 4

[Guy Pyrzak]

Attachment: Patch v2

You approved this already. However, that last patch had a bunch of crap in there that wasn't supposed to be in there. This patch is all cleaned up.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 306669 [details] [diff] [review]
Patch v2

>-    YAHOO.util.Event.addListener(action, 'click', showEditableField, new Array(container, input) );
>+    YAHOO.util.Event.addListener(action,
>+                                 'click',
>+                                 showEditableField,
>+                                 new Array(container, input)
>+                                 );

  Nit: That's *too many* lines. You can just make things look like this:

YAHOO.util.Event.addListener(action, 'click', showEditableField,
                             new Array(container, input));

  Also, I think we should add a global alias YUE for YAHOO.util.Event, and YUD for YAHOO.util.Dom.

  Otherwise, this looks fine. I assume that you swear on a stack of...whatever's important to HCI people, Neilsen or something...that you tested this and it works, because I did not test this patch.

Comment 6

[Frédéric Buclin]

Before committing this patch, I would like the "That's *too many* lines" comment to be addressed first. I agree that's too many, and this doesn't make the code more readable.

Also, how do you explain that the patch increases from 4.76 Kb to 10.52 Kb? I don't get it. Could we focus on the topic of the bug only? It seems to me that many changes are unrelated to this bug (indentation only, or something like that).

Comment 7

[Guy Pyrzak]

ack! I committed the patch last night b/c i thought it was approved so i committed it. I thought the cvs automatically updated the bmo page... but it didn't. If someone can revert the change that would be good or I can just make another patch to remove the new lines. I'll not commit again till I hear otherwise.

To address LpSolit's concerns about why it got so much bigger. I went through the whole fields.js file and fixed ALL the lines that were longer than 80 characters long (which was more than just the original 3 line fix that i did) if you do a quick diff of them you can see that. Sorry about that. I was just trying to meet max's concerns about the 80 lines.

I agree with max's suggestion of making an alias to YAHOO.util.Event and YAHOO.util.Dom.

Let me know what you guys would like I'll make another patch with max's suggestions. And also fix the other things with the too many lines issue as well. 

Comment 8

[Guy Pyrzak]

filed bug 420494 to address mkanat's suggestion.

Comment 9

[Guy Pyrzak]

Attachment: Indent Fix

fixes the committed code to not be so many lines. see bug 420494 which addresses mknanat's other suggestions. Sorry again for prematurely committing the patch with too many lines in it.

Comment 10

[Frédéric Buclin]

Comment on attachment 306778 [details] [diff] [review]
Indent Fix

Nit: you should be consistent about spaces after commas. But that's minor.

r=LpSolit a=LpSolit for this patch. Do not forget to paste the output of CVS here this time, and mark this bug as FIXED. (it completely went out of my radar because the previous checkin has never been reported to this bug)

Comment 11

[Frédéric Buclin]

Checking in js/field.js;
/cvsroot/mozilla/webtools/bugzilla/js/field.js,v  <--  field.js
new revision: 1.6; previous revision: 1.5
done