Last Comment Bug 538211 - Value-controlled fields do not behave correctly with bookmarkable templates on enter_bug.cgi
: Value-controlled fields do not behave correctly with bookmarkable templates o...
Product: Bugzilla
Classification: Server Software
Component: User Interface (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Max Kanat-Alexander
: default-qa
Depends on:
  Show dependency treegraph
Reported: 2010-01-06 10:58 PST by damontripodi
Modified: 2010-02-17 14:08 PST (History)
4 users (show)
LpSolit: approval+
LpSolit: approval3.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.6+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

v1 (679 bytes, patch)
2010-01-14 14:29 PST, Michael Thomas (Mockodin)
mkanat: review-
Details | Diff | Splinter Review
v2 (571 bytes, patch)
2010-02-07 14:06 PST, Max Kanat-Alexander
LpSolit: review-
Details | Diff | Splinter Review
v3 (688 bytes, patch)
2010-02-16 23:35 PST, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Splinter Review

Description damontripodi 2010-01-06 10:58:47 PST
User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; chromeframe; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: 3.4.1

when using a bookmarkable template that contains conditional fields (fields that appear only based on the value of the parent field), where the parent field contains a value in the template, the conditional fields are not displayed on the page until the parent field is selected and the value re-selected.

Expectation:  the conditional fields are displayed immediately, based on the value of the parent field so as not to cause user inconvenience

Reproducible: Always
Comment 1 Michael Thomas (Mockodin) 2010-01-12 11:32:48 PST
The fix I'm using it to add:
YAHOO.util.Event.addListener(window, "load", handleVisControllerValueChange, [controlled_id, controller, value]);

to the end of the showFieldWhen function in fields.js

this works for tip, not sure what if anything changed from 3.4 for this
Comment 2 Max Kanat-Alexander 2010-01-12 12:33:32 PST
Cool. The end of field.js would be the wrong place for that, and window.load might not be the right handler, but doing an ONDomReady by default or otherwise handling that well within field.html.tmpl might be a valid solution.
Comment 3 Michael Thomas (Mockodin) 2010-01-14 14:29:30 PST
Created attachment 421709 [details] [diff] [review]

Posting a patch more for reference for conversation than anything else.

While binding to window.load might not be the 'best' it does work. The limitation there being the YUI has a 10? 30? second timeout if an object doesn't exist it will keep retrying till it does or time elapses which ever is first

Placing this here means that we don't have to reprocess the cf list later to add the events. Where might be a better place? I guess I'm just not seeing the other possibilities atm, not saying there isn't better.
Comment 4 Michael Thomas (Mockodin) 2010-01-29 09:14:01 PST
Comment on attachment 421709 [details] [diff] [review]

Requesting review in the interest of getting this ticket off the blocking3.4.5 list ;-)
Comment 5 Max Kanat-Alexander 2010-01-31 09:18:36 PST
Comment on attachment 421709 [details] [diff] [review]

>Index: Bugzilla/js/field.js
>     // might not yet exist in the DOM. We just pass along its id.
>     YAHOO.util.Event.addListener(controller, 'change', 
>         handleVisControllerValueChange, [controlled_id, controller, value]);
>+    YAHOO.util.Event.addListener(window, "load", handleVisControllerValueChange, [controlled_id, controller, value]);

  Instead of that, use OnDOMReady. Also, I'm concerned about this causing a performance issue on browsers where there is otherwise no performance issue. Imagine that you load a page that has several selects with thousands of values that are each controlled by a separate field. True, in IE currently, that would suck, but it wouldn't in Firefox.
Comment 6 Frédéric Buclin 2010-02-01 02:33:16 PST
Same root cause as bug 520993, I guess. Dupe?
Comment 7 Max Kanat-Alexander 2010-02-01 06:29:47 PST
(In reply to comment #6)
> Same root cause as bug 520993, I guess. Dupe?

  Yeah, I'd say dupe that one to this one, since this one has a patch.
Comment 8 Michael Thomas (Mockodin) 2010-02-01 08:56:11 PST
(In reply to comment #5)
True, I am unsure off the top of my head how to negate that however.
Unless this were moved perl side in which case we could check for what fields should be visible load that to a hash and have it checked while processing the template and adjusting visibility to suit.

Anyone have an instance with conditions Max mentioned? Some baselines would be useful.
Comment 9 Max Kanat-Alexander 2010-02-01 09:15:29 PST
The Perl solution will be feasible for 3.8, since there are some useful functions getting checked in as part of the Component-as-controller patch.
Comment 10 Max Kanat-Alexander 2010-02-07 13:26:24 PST
*** Bug 520993 has been marked as a duplicate of this bug. ***
Comment 11 Max Kanat-Alexander 2010-02-07 14:06:09 PST
Well, upon further investigation, it appears that the bug I just duped to this bug is in fact *not* a dupe of this bug. This problem is actually being caused by the fact that %default wasn't getting populated with the custom field values properly. Attaching a patch shortly.
Comment 12 Max Kanat-Alexander 2010-02-07 14:06:44 PST
Created attachment 425720 [details] [diff] [review]

I'm pretty sure this will apply back to 3.4.
Comment 13 Frédéric Buclin 2010-02-09 08:41:12 PST
I cannot reproduce this problem in Bugzilla 3.7. I have a custom field which depends on a component value, but this custom field is displayed when the component is already selected. Do you have STR?
Comment 14 Max Kanat-Alexander 2010-02-09 13:20:25 PST
You have to do it with one custom field controlling another custom field.
Comment 15 Frédéric Buclin 2010-02-09 18:20:38 PST
Comment on attachment 425720 [details] [diff] [review]

The custom field which visibility depends on is a multi-select field. With your patch applied, Bugzilla crashes with:

undef error - Can't use string ("3.8-branch") as an ARRAY ref while "strict refs" in use at Bugzilla/Field/ line 174. at Bugzilla/Field/ line 174 Bugzilla::Field::ChoiceInterface::is_set_on_bug('Bugzilla::Field::Choice::cf_branch=HASH(0xa558cf8)', 'HASH(0xa031300)') called at Bugzilla/Field/ line 149 Bugzilla::Field::ChoiceInterface::is_visible_on_bug('Bugzilla::Field::Choice::cf_branch=HASH(0xa558cf8)', 'HASH(0xa031300)') called at template/en/default/bug/field.html.tmpl line 148 eval {...} called at template/en/default/bug/field.html.tmpl line 148 eval {...} called at template/en/default/bug/field.html.tmpl line 164 eval {...} called at template/en/default/bug/field.html.tmpl line 18
Comment 16 Max Kanat-Alexander 2010-02-16 23:35:54 PST
Created attachment 427283 [details] [diff] [review]

Ah, okay. Does this fix it?
Comment 17 Frédéric Buclin 2010-02-17 06:24:25 PST
Comment on attachment 427283 [details] [diff] [review]

Yes, it does. Thanks! r=LpSolit
Comment 18 Frédéric Buclin 2010-02-17 06:29:05 PST
Also works on 3.4.
Comment 19 Max Kanat-Alexander 2010-02-17 14:08:49 PST
Committing to: bzr+ssh://                       
modified enter_bug.cgi
Committed revision 7001.

Committing to: bzr+ssh://                         
modified enter_bug.cgi
Committed revision 6979.                                                       

Committing to: bzr+ssh://                         
modified enter_bug.cgi
Committed revision 6725.

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