Closed Bug 538211 Opened 11 years ago Closed 10 years ago

Value-controlled fields do not behave correctly with bookmarkable templates on enter_bug.cgi

Categories

(Bugzilla :: User Interface, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: damontripodi, Assigned: mkanat)

Details

Attachments

(1 file, 2 obsolete files)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.4.5+
OS: Windows XP → All
Hardware: x86 → All
Summary: template does not show conditional fields → Bookmarkable template does not show value-controlled fields on enter_bug.cgi
Target Milestone: --- → Bugzilla 3.4
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
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.
Attached patch v1 (obsolete) — Splinter 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 on attachment 421709 [details] [diff] [review]
v1

Requesting review in the interest of getting this ticket off the blocking3.4.5 list ;-)
Attachment #421709 - Flags: review?(mkanat)
Assignee: ui → mockodin
Comment on attachment 421709 [details] [diff] [review]
v1

>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.
Attachment #421709 - Flags: review?(mkanat) → review-
Flags: blocking3.4.5+ → blocking3.4.6+
Same root cause as bug 520993, I guess. Dupe?
(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.
(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.
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.
Duplicate of this bug: 520993
Summary: Bookmarkable template does not show value-controlled fields on enter_bug.cgi → Value-controlled fields do not behave correctly when parent field values are already set in the template
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.
Summary: Value-controlled fields do not behave correctly when parent field values are already set in the template → Value-controlled fields do not behave correctly with bookmarkable templates on enter_bug.cgi
Attached patch v2 (obsolete) — Splinter Review
I'm pretty sure this will apply back to 3.4.
Assignee: mockodin → mkanat
Attachment #421709 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #425720 - Flags: review?(LpSolit)
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?
You have to do it with one custom field controlling another custom field.
Attachment #425720 - Flags: review?(LpSolit) → review-
Comment on attachment 425720 [details] [diff] [review]
v2

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/ChoiceInterface.pm line 174. at Bugzilla/Field/ChoiceInterface.pm line 174 Bugzilla::Field::ChoiceInterface::is_set_on_bug('Bugzilla::Field::Choice::cf_branch=HASH(0xa558cf8)', 'HASH(0xa031300)') called at Bugzilla/Field/ChoiceInterface.pm 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
Attached patch v3Splinter Review
Ah, okay. Does this fix it?
Attachment #425720 - Attachment is obsolete: true
Attachment #427283 - Flags: review?(LpSolit)
Attachment #427283 - Flags: review?(LpSolit) → review+
Comment on attachment 427283 [details] [diff] [review]
v3

Yes, it does. Thanks! r=LpSolit
Also works on 3.4.
Flags: approval3.6+
Flags: approval3.4+
Flags: approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified enter_bug.cgi
Committed revision 7001.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified enter_bug.cgi
Committed revision 6979.                                                       

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/                         
modified enter_bug.cgi
Committed revision 6725.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.