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

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Interface
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: damontripodi, Assigned: Max Kanat-Alexander)

Tracking

unspecified
Bugzilla 3.4
Bug Flags:
approval +
approval3.6 +
approval3.4 +
blocking3.4.6 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

v3
688 bytes, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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
(Assignee)

Updated

8 years ago
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
(Assignee)

Comment 2

8 years ago
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.
Created attachment 421709 [details] [diff] [review]
v1

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)

Updated

8 years ago
Assignee: ui → mockodin
(Assignee)

Comment 5

8 years ago
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-

Updated

8 years ago
Flags: blocking3.4.5+ → blocking3.4.6+

Comment 6

8 years ago
Same root cause as bug 520993, I guess. Dupe?
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 520993
(Assignee)

Updated

8 years ago
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
(Assignee)

Comment 11

8 years ago
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
(Assignee)

Comment 12

8 years ago
Created attachment 425720 [details] [diff] [review]
v2

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)

Comment 13

8 years ago
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?
(Assignee)

Comment 14

8 years ago
You have to do it with one custom field controlling another custom field.

Updated

8 years ago
Attachment #425720 - Flags: review?(LpSolit) → review-

Comment 15

8 years ago
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
(Assignee)

Comment 16

8 years ago
Created attachment 427283 [details] [diff] [review]
v3

Ah, okay. Does this fix it?
Attachment #425720 - Attachment is obsolete: true
Attachment #427283 - Flags: review?(LpSolit)

Updated

8 years ago
Attachment #427283 - Flags: review?(LpSolit) → review+

Comment 17

8 years ago
Comment on attachment 427283 [details] [diff] [review]
v3

Yes, it does. Thanks! r=LpSolit

Comment 18

8 years ago
Also works on 3.4.
Flags: approval3.6+
Flags: approval3.4+
Flags: approval+
(Assignee)

Comment 19

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.