developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 12 years ago

Closed 12 years ago

#781 closed bug (fixed)

Bug de validation d'un formulaire quand il y a un tag <radiobuttons> dans le formulaire

Reported by: geekbay Owned by: Julien
Priority: normal Milestone: jelix 1.1
Component: jelix:forms Version: trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

Si dans myform.form.xml on a un tag <radiobuttons> par exemple

<?xml version="1.0" encoding="utf-8"?>
<form xmlns="http://jelix.org/ns/forms/1.1">

   <radiobuttons ref="couleur">
      <label>Couleur de votre voiture:</label>
      <item value="r">Rouge</item>
      <item value="b">Bleue</item>
      <item value="v">Verte</item>
      <item value="j">Jaune</item>
  </radiobuttons>

  <input ref="nom" required="true" maxlength="255">
  <label>nom</label>
</input>

<submit ref="_submit">
  <label>Valider</label>
</submit>
</form>

alors il n'y a plus de validation javascript due a l'erreur suivante:

Erreur : (text
"").replace is not a function

Fichier Source : http://127.0.0.1/jelix/www/jquery/jquery.js Ligne : 23

Attachments (6)

monpatch.diff (1.9 KB) - added by geekbay 12 years ago.
Proposition de patch
#781-trunk-radiobuttons-js-check.patch (2.1 KB) - added by bibo 12 years ago.
fix is better located into getValue prototype
781-jForms-javascript-check-for-multiple-selection-controls-fails.diff (1.6 KB) - added by Julien 12 years ago.
781-jForms-javascript-check-for-multiple-selection-controls-fails.2.diff (3.4 KB) - added by Julien 12 years ago.
781-jForms-javascript-check-for-multiple-selection-controls-fails.3.diff (7.3 KB) - added by Julien 12 years ago.
781-jForms-javascript-check-for-multiple-selection-controls-fails.4.diff (7.1 KB) - added by Julien 12 years ago.
previous patch was bugged

Download all attachments as: .zip

Change History (22)

Changed 12 years ago by geekbay

Proposition de patch

Changed 12 years ago by bibo

fix is better located into getValue prototype

comment:1 follow-ups: Changed 12 years ago by bibo

  • Milestone set to jelix 1.1
  • review set to review?
  • Version changed from 1.0RC1 to trunk

thanks geekBay for your report and first patch. I tend to think the problem is generic to "list values" controls.

My patch fixes this in the two forms builder (jquery and light) and is focused on getValue prototype. getValue should always return an empty string if nothing has been edited/selected/checked even on list controls.

Asking for review.

comment:2 in reply to: ↑ 1 Changed 12 years ago by geekbay

yes it's a solution too since the bug was in the Jquery trim function that can't trim since in the case of a radio the argument was an array. So now if the getValue return always a string, trim() will work.

Replying to bibo:

thanks geekBay for your report and first patch. I tend to think the problem is generic to "list values" controls.

My patch fixes this in the two forms builder (jquery and light) and is focused on getValue prototype. getValue should always return an empty string if nothing has been edited/selected/checked even on list controls.

Asking for review.

comment:3 in reply to: ↑ 1 Changed 12 years ago by Julien

Replying to bibo:

Asking for review.

Hello,

doesn't seem to work, as it still returns an array, that can't be trimmed.

made another patch, that always returns a string. Tested with a real world case in an app.

comment:4 Changed 12 years ago by bibo

Effectively, Julien, my patch is buggy and still raises a jQuery.trim() error...

But the meaning of getValue is to return value(s) of form elements and as so, an array of values if there is multiple selection. And it should return a default failure value being it or false, if no value is retrieved. That failure value should be the same for all elements.

Here i see 2 paths :

1) do the trimming in getValue

2) add another condition before jQuery.trim : if (!val.join && jQuery.trim(...

I would lean toward 1, thoughts guys ?

comment:5 Changed 12 years ago by Julien

I think there's another way : adding a getValue() method in the prototype of every jFormsControl object.

That way, each type of control has it's own getValue method, it will be easier to maintain and to handle qite complex controls I think.

Look at what I did for jFormsJQControlDatetime or jFormsJQControlDate for example.

comment:6 Changed 12 years ago by bibo

That would be the cleaner solution. Also to use with But much more work tough !

comment:7 Changed 12 years ago by bibo

"Also to use with" should be trimmed to "" of course :)

comment:8 Changed 12 years ago by Julien

  • Owner set to Julien
  • Status changed from new to assigned

I think I can do it today.

I take the ticket.

comment:9 Changed 12 years ago by Julien

I think I found a quicker way: returning null when the array is empty (that really mean "there is no value").

Also added a check for "false" value (for unchecked required checkboxes), as we now use strict comparison operator, and

false === '' // will return false

comment:10 follow-up: Changed 12 years ago by bibo

Julien, I see you have taken my proposal #1. ie. do the trimming in getValues. I really feel it is the good way to go.

Is this not possible to always return false on failure in getValues ? I mean : return false at the end and return field values if :

  • trimmed input/textarea values != ''
  • radiobuttons, checboxes, select : one item is selected at min

comment:11 in reply to: ↑ 10 Changed 12 years ago by Julien

Replying to bibo:

Julien, I see you have taken my proposal #1. ie. do the trimming in getValues. I really feel it is the good way to go.

yup, and it's quicker than writing a getValue method for each control type.

Is this not possible to always return false on failure in getValues ?

I would prefer returning null when there's no value:

  • textboxes: trimmed value is an empty string
  • checkbox: unchecked (means no value instead of false)
  • select/checkboxes/radios: nothing selected
  • default returned value at the end of getValue

thus, we'll only have to check against the null value

Is this what you meant ?

Changed 12 years ago by Julien

previous patch was bugged

comment:12 Changed 12 years ago by Julien

last patch: we have to keep returning false for unchecked checkboxes...

so we now fire an error when control is required and value is null or false.

comment:13 Changed 12 years ago by laurentj

  • Component changed from jelix to jelix:forms

Since this not the first regression on jforms.js, we really need some unit tests on this jforms.js script NOW.

So we must have, in testapp, a page which show ALL possible controls, and which includes a script which set/unset some values on each controls, and verify jForms methods/properties each time it does this modifications on the forms. Of course, this page will be display "manually", but a contributor who modify jforms.js will verify it this test page is ok.

If you want to use http://fireunit.org/ or any other js libraries, no problem, but we must do it. We cannot continue to have this kind of regression.

comment:14 Changed 12 years ago by laurentj

I'm going to add tests this next days.

comment:15 Changed 12 years ago by laurentj

  • review changed from review? to review+

Committed some new unit tests + this patch. trunk + 1.1.x branch. r1235

comment:16 Changed 12 years ago by laurentj

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.