JavaScript error on bottom left while editing a field

I tried to replicate the issue against the latest dev. I do not see an error message, but following the instructions above and clicking 'Save' throws a JavaScript exception:
An invalid form control with name='field_storage[cardinality_number]' is not focusable.

We probably shouldn't be able to set the field to '0' at all - the minimum should be 1.

The original error description is probarbly due to the validation implementation in the browser that PieterJanPut uses.
Using chrome i get the same error as .John.

The number field is set to have a min of 1 but that is only enforced when the form is submitted (in chrome, havn't tested in others, yet).

this seems to be an issue with the HTML5 Validation. 0 is no allowed value in the "field_storage[cardinality_number]" field. But validation is still triggered if the field is invisible.
This leads to misplaced Message in Firefox and an exception in chrome

I'm at Drupalcon Amsterdam and this will be the first issue I'll work on.
My IRC nick: jmoreira

I just talked to thomasth, I have to leave now, so I'll leave that you guys. I'll check later if I can help after that.

So here is a proposed fix.
If the input number field is disabled, when hidden, it won't be validated and it won't stop the submit.

tested in chrome 37 and firefox 34 on linux

This patch doesn't seem to apply, but I'm working on it.

Re-rolled the patch. It didn't seem to have been created from the Drupal root, so didn't apply. This now applies from root, and seems to fix the issue with the JavaScript exception, and the 'misplaced' message in Firefox (and no issues in IE that I've found).

This seems quite a broad scope though, best check it doesn't impact anything else in core...

Patch now applies fine (from root folder) and has been re-rolled correctly and fixes original issue.

I'm going to disagree with the exact implentation of the fix in states.js.
Beeing so generic, as it is, for all forms, what happens if a number element has a default value under 0?

Sounds like that could be a problem

The patch in #9 seems to work for me.

I've tested manually the patch in #11. The message is showing at the top of the page, as it should.
Screenshoot:

The patch applies, but the logic is probably incorrect. How about we return to the default value regardless of whether it is less than 0 then?

Also, what happens if there isn't a default value set?

So this patch is arguably even more generic, but fixes the issues mentioned in #13

If a field becomes visible again, and has a default value set, then it is returned to the default value. This will need a lot of review though because of the broad scope. Perhaps there is a better way to tackle this?

I'd agree that changing states.js feels like the wrong approach - much too broad.

What are the objections to the approach in #9?

The patch in #18 fixes the issue for numbers, but if text is entered in the number field then the original issue occurs. I'm not sure this is the right approach at all.

Looks like we've been on a wild goose chase here then.

The patch in #7 does seem to fix the original issues, and is much more sensible than trying to edit states.js.

So let's focus on a review of #7 then, which seems like the best approach.

(Edit: I think malcomio was referring to #7 too, since #9 is still changing the states.js)

Agreed - #7 seems to work fine and does not need re-rolling.

@.John - yes I was referring to #7 - not sure what was going on in my head there.

+++ b/core/misc/states.js
@@ -533,7 +533,11 @@
+      if (target.context.defaultValue) {

Will this always be set, or does this open the possibility of undefined warnings?

tagging

I've reviewed the patch from #7 and it's working fine for me.

The question in #25 applies to the other proposed patch, not #7.

please rename the variable to $target, we prefix all jQuery related stuff with $ to be explicit. Thanks.

Also it'd be good to have a UX opinion on reseting the value when making a field visible.

@nod_ The patch doesn't involve changes to states.js. That approach was rejected around #19-21. So not sure if we want to change states.js for this issue.

ALSO I don't even see the line of code mentioned in #25 in states.js in the 8.0.x branch. ??

I don't see that there is a UX problem here. Now that the error is fixed, if you repeat the steps to reproduce the problem you will see that the error is gone, the save works, and then you are taken back to the manage fields screen. I have tested the patch while adding new fields as well as editing fields and it appears to fix both situations.

Applied patch #07 and it resolves the issue related to js error and form submitted.

Ok thanks, can you reup the right patch so that the latest one is the one to review? It's clearer that way, especially when there is a change of approach in the middle of the issue.

@nod_ Sure thing. Here's the patch again.

#7 patch works. The last submitted patch #33 won't apply cleanly.

applying right patch for review. #dcm2015

#35 apples cleanly and validation problem resolved (tested by simpletest.me). moving to RTBC.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed a05890e and pushed to 8.0.x. Thanks!