Last Comment Bug 707428 - Custom field values whose visibility depends on another field value do not remain selected after editing a bug
: Custom field values whose visibility depends on another field value do not re...
Status: RESOLVED FIXED
[doesn't affect trunk]
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 4.0.2
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on: 308253
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-03 04:37 PST by Frédéric Buclin
Modified: 2011-12-15 07:20 PST (History)
3 users (show)
LpSolit: approval4.2+
LpSolit: blocking4.2+
LpSolit: approval4.0+
LpSolit: blocking4.0.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (630 bytes, patch)
2011-12-03 05:05 PST, Frédéric Buclin
wicked: review-
Details | Diff | Review
patch, v2 (509 bytes, patch)
2011-12-08 16:02 PST, Frédéric Buclin
wicked: review+
Details | Diff | Review

Description Frédéric Buclin 2011-12-03 04:37:11 PST
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.
Comment 1 Frédéric Buclin 2011-12-03 05:05:27 PST
Created attachment 578827 [details] [diff] [review]
patch, v1
Comment 2 Frédéric Buclin 2011-12-05 08:40:08 PST
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 3 Teemu Mannermaa (:wicked) 2011-12-06 15:20:23 PST
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.
Comment 4 Frédéric Buclin 2011-12-08 16:02:21 PST
Created attachment 580226 [details] [diff] [review]
patch, v2

This fixes the problem with IE9. No error thrown.
Comment 5 Teemu Mannermaa (:wicked) 2011-12-14 21:30:24 PST
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. :)
Comment 6 Teemu Mannermaa (:wicked) 2011-12-14 21:56:04 PST
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.
Comment 7 Frédéric Buclin 2011-12-14 23:58:31 PST
(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! :)
Comment 8 Teemu Mannermaa (:wicked) 2011-12-15 00:19:28 PST
(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?)
Comment 9 Frédéric Buclin 2011-12-15 07:20:16 PST
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.

Note You need to log in before you can comment on or make changes to this bug.