1183398 - Mandatory custom fields block form submission if they are hidden and have no value

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Ben]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

We have a custom field named: cf_fix_in
Description: Build(s)\Version Fixed In
Type: Free Text
Is Mandatory: check
Field only appears when: Resolution (resolution)
Is set to any of: Fixed


Actual results:

If you look at a new bug where the status is NEW, enter a text comment, and click "Save changes", then nothing happens.

The browser output window complains about: An invalid form control with name='cf_fix_in' is not focusable.

If I change the custom field to always be visible, then it does not allow a change to be saved, showing a popup on this field asking for a value.


Expected results:

Since the field is not visible, it should not matter what the value is.

So it seems that it correctly decides to not show the field, but then incorrectly decides that it needs a value for it even though it is not visible.

Comment 1

[Ben]

This bug is preventing us from upgrading to bugzilla 5.0. If a fix or patch or work around could be made available, that would greatly help, please.

Comment 2

[Ben]

This also happens if you log a new bug. One work around is to set the resolution to fixed, then supply something for Build(s)\Version Fixed in, and change the resolution back to new. You can then enter comments and save them.

But the user interface is hiding this field from you correctly, but incorrectly wants a value for it anyway.

Comment 3

[Ben]

This was working fine in bugzilla 4.x. So it broken in the 5.0 release.

Comment 4

[Frédéric Buclin]

Bugzilla 5.0 adds the 'required' HTML5 attribute to mandatory custom fields, even if they are hidden. So the browser will refuse to submit the form if such fields have no value.

We could certainly fix this by removing the 'required' attribute on hidden fields when submitting the form, assuming events related to form submission are executed before the browser checks input data.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

I use data-required to store the information about mandatory fields so that the 'required' attribute can be restored when mandatory fields are visible again. data-* are element attributes which can be used freely to store extra data, and are valid with HTML5.

Comment 6

[Frédéric Buclin]

Attachment: patch, v1.1

I forgot to fix the case where the field was initially hidden and never displayed.

Comment 7

[Ben]

Comment on attachment 8633652 [details] [diff] [review]
patch, v1.1

Thank you, the last patch appears to resolve the issue successfully and allows us to upgrade.

Comment 10

[Dean Price]

Hi Frederic,

I applied this patch to my sandbox "Clean" 5.0 install and it worked --> Great!!!

Then I went to my Sandbox "4.4.9" install and ran the 5.0 upgrade via git. Then ran the patch. My results were not so great.

I am still getting 11 of the "An invalid form control with name='cf_*' is not focusable" Errors

On the upside I was getting 79 of these errors.

Thanks,
Dean Price

Comment 11

[Dean Price]

Hi Frederic,

After doing some detective work in the DB... I have found that the errors are coming from the only custom fields that are of "Type 4" (Large Text Box)

I hope that helps you out

Thanks
Dean Price

Comment 12

[Frédéric Buclin]

This doesn't help me, no. :)

IMO, this is not specific to this field type. If you detected which custom field causes trouble, then look at the HTML source code of the form which your browser refuses to submit, check attributes of this (hidden?) field, and look at conditions under which this field is displayed.

When you say that your clean 5.0 installation works, do you mean you first recreated all the custom fields you had in 4.4.9 before doing testing? Else it's pretty hard to compare the data.

Comment 13

[Dean Price]

Hi Frederic,

What I found this AM is, of the 11 remaining errors I am getting,

Three of them have "showFieldWhen" statements in the html and those are dependant on other custom fields.

The rest of them do not have the "showFieldWhen" statement

Here is the section of code of one of the fields that do not have the "showFieldWhen" statement:
this is the only reference to this field in the html.

    <tr ><th class="field_label  bz_hidden_field required"
    id="field_label_cf_paareareq">

    <label for="cf_paareareq">

  <a 
      title="A description of why the request is being made"
      class="field_help_link"
      href="page.cgi?id=fields.html#cf_paareareq"
  >Reason for Request:</a>
</label>
</th>
  <td class="field_value  bz_hidden_field"
      id="field_container_cf_paareareq"  colspan="3">
       <div id="cf_paareareq_edit_container" class="bz_default_hidden">
         <div>
             (<a href="#" id="cf_paareareq_edit_action">edit</a>)
         </div>
       </div>
       <div id="cf_paareareq_input"><textarea name="cf_paareareq" id="cf_paareareq"
            rows="4"
          cols="60"
            onFocus="this.rows=8"
            aria-required="true" required></textarea>
       </div>
       <script type="text/javascript">
         hideEditableField('cf_paareareq_edit_container',
                           'cf_paareareq_input',
                           'cf_paareareq_edit_action',
                           'cf_paareareq',
                           '',
                           '',
                           true);
       </script>

</td>
    </tr>

let me know if I can provide you with anything else...

Comment 14

[Frédéric Buclin]

(In reply to Dean Price from comment #13)
> Here is the section of code of one of the fields that do not have the
> "showFieldWhen" statement:
> this is the only reference to this field in the html.
> 
>     <tr ><th class="field_label  bz_hidden_field required"
>     id="field_label_cf_paareareq">


Why does this field have both bz_hidden_field and required classes?? You say this field has no showFieldWhen condition attached to it, so why is it there at all? :) Can you tell me more about this custom field?

Comment 15

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #14)
> Why does this field have both bz_hidden_field and required classes??

Ah, I know why! :) New patch coming!

Comment 16

[Dean Price]

(In reply to Frédéric Buclin from comment #15)
> (In reply to Frédéric Buclin from comment #14)
> > Why does this field have both bz_hidden_field and required classes??
> 
> Ah, I know why! :) New patch coming!

Thank you ...

Comment 17

[Frédéric Buclin]

Attachment: patch, v2

Textarea fields are generated from a separate template, which is why I missed it in my previous patch.

Comment 18

[Dean Price]

> patch, v2
> 
> Textarea fields are generated from a separate template, which is why I
> missed it in my previous patch.

It seemed to have worked ... I will continue to test my sandbox upgrade 

Thank you much.

Comment 20

[Frédéric Buclin]

Comment on attachment 8643683 [details] [diff] [review]
patch, v2

gerv, are you faster than dkl to review this? :)

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 8643683 [details] [diff] [review]
patch, v2

Review of attachment 8643683 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like hiddenness is toggleable in JS (see js/field.js) - is that like the "Advanced fields" twisty on the enter_bug page? Have I missed something, or does this mean that a user can avoid a field's mandatory-ness simply by hiding it? 

Gerv

Comment 22

[Frédéric Buclin]

No, a user cannot hide mandatory (custom) fields. But a mandatory custom field can be hidden if it's configured to be only displayed on some condition (for instance: custom field 'cf_foo' is only displayed if severity = critical). You configure these conditions from editfields.cgi.

Comment 23

[Gervase Markham [:gerv]]

Then what's the JS for?

Gerv

Comment 24

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #23)
> Then what's the JS for?

If a user set a field to a value which triggers displaying/hidding a custom field, then you need JS to trigger this change. In my example above, if the user select severity = critical, then JS must display the cf_foo custom field.

Comment 25

[Gervase Markham [:gerv]]

Comment on attachment 8643683 [details] [diff] [review]
patch, v2

Review of attachment 8643683 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like hiddenness is toggleable in JS (see js/field.js) - is that like the "Advanced fields" twisty on the enter_bug page? Have I missed something, or does this mean that a user can avoid a field's mandatory-ness simply by hiding it? 

Gerv

Comment 26

[Gervase Markham [:gerv]]

Oops, ignore the comment there. Bugzilla "restored it from draft"...

Gerv

Comment 27

[Frédéric Buclin]

Thanks for the quick review. :)

Comment 28

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   baed571..684bee4  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   a2ae154..6f616e1  5.0 -> 5.0