Closed Bug 707428 Opened 13 years ago Closed 12 years ago

Custom field values whose visibility depends on another field value do not remain selected after editing a bug

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

4.0.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

(Whiteboard: [doesn't affect trunk])

Attachments

(1 file, 1 obsolete file)

I just wrote a new Selenium script to test custom fields and the ability to show/hide custom field values based on another field value, see bug 308253, and the script fails consistently (I can reproduce manually):

I have a custom drop-down field whose value 'ghost' must only be visible when the bug resolution is FIXED. So if I set the resolution to FIXED, the 'ghost' value is selectable, but when I commit changes and revisit the bug, 'ghost' is no longer selected despite the bug history says the change was successful and despite the source code of the page has selected="selected". From what I can see, the problem is in handleValControllerChange() in js/field.js, because it marks this value as unselected:

            if (item.selected) {
                item.selected = false;
                bz_fireEvent(controlled_field, 'change');
            }
            item.disabled = true;

If I comment // item.selected = false; then 'ghost' remains visible when revisiting the bug. This is of course not the right fix as editing the bug resolution to something else doesn't hide 'ghost' again. But this shows that some code is missing to re-select this field value on page load.


I tested with 4.0.x and 4.1.x and I can in both cases reproduce. I'm marking this bug as blocker because the next time you edit the bug, the field value is overriden as the wrong value is selected. Also, all QA boxes are orange due to this bug.
Flags: blocking4.2+
Flags: blocking4.0.3+
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
Attachment #578827 - Flags: review?(wicked)
Note that my patch is for 4.2 and 4.0.3 only. field.js had a major rewrite in 5.0 as part of bug 636416, so it won't apply there (and I couldn't reproduce the issue on trunk).
Comment on attachment 578827 [details] [diff] [review]
patch, v1

This affects 4.0 and 4.2 branches since for trunk, this was fixed in bug 636416. It also affects 3.4 and 3.6 branches but unless this can be considered dataloss bug we won't fix those branches..

Problem seems to be that the handleValControllerChange() is called twice during initialization. First time round the else part (disable option) is executed so the Ghost-option gets disabled and second time round the if part (enable option) is executed so the option is once again enabled. Unfortunately, this also changes the selected value to a default one since the one marked selected (Ghost-option) is disabled and thus not selectable (and the if-part never restores the selection).

Your change modifies this so that the if part (enable option) gets executed both times. This is redundant and affects performance slightly. That might have been acceptable but unfortunately IE now fails with error:

Error: Unable to delete property 'v6_cf_drop_down': object is null or undefined

This error popups twice during initialization and happens because you can't run showOptionInIE() without running hideOptionInIE() before.

Rather than fighting that, you should probably still protect the if-part (enable option) with the item.disabled check (just make sure it doesn't do the else part when controller item is really selected). You can take a look at bug 636416 code to see how it does this correctly.
Attachment #578827 - Flags: review?(wicked) → review-
Attached patch patch, v2Splinter Review
This fixes the problem with IE9. No error thrown.
Attachment #578827 - Attachment is obsolete: true
Attachment #580226 - Flags: review?(wicked)
Comment on attachment 580226 [details] [diff] [review]
patch, v2

Not exactly what I had in mind but this is actually better, and it works as planned as well, AFAICT. :)
Attachment #580226 - Flags: review?(wicked) → review+
Flags: approval4.2?
Flags: approval4.0?
Oh, one thing I did notice during testing. If the controller field is changed so that the selected Ghost-option is no longer valid, it will get sneakily (especially if the Ghost-option is off the screen) changed to default and won't get changed back even if the controller field is changed back.

I'm not sure if this is something we should fix on branches. This actually doesn't affect trunk, which seems to preserve the set value even if it's now invalid.
(In reply to Teemu Mannermaa (:wicked) from comment #6)
> Oh, one thing I did notice during testing. If the controller field is
> changed so that the selected Ghost-option is no longer valid, it will get
> sneakily (especially if the Ghost-option is off the screen) changed to
> default and won't get changed back even if the controller field is changed
> back.

This is not a regression due to my patch, right? In that case, I suggest we approve it, and a separate bug can be filed if required. Thanks for the review! :)
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Whiteboard: [doesn't affect trunk]
(In reply to Frédéric Buclin from comment #7)
> This is not a regression due to my patch, right? In that case, I suggest we

Yeah, it's not a regression, just something I noticed. Actually without your patch the change happens during load and with your patch during changing the controller field so it's a bit better. It's bad UX to sneakily change this but not necessarily something that has to be fixed on branches, IMO. (Or am I just trying to avoid increasing our blocker count?)
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified js/field.js
Committed revision 7982.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified js/field.js
Committed revision 7669.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.