Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abide only invalidates last radio button (Foundation 6.6.3) #12081

Closed
3 tasks done
amygilbert opened this issue May 22, 2020 · 5 comments · Fixed by #12082
Closed
3 tasks done

Abide only invalidates last radio button (Foundation 6.6.3) #12081

amygilbert opened this issue May 22, 2020 · 5 comments · Fixed by #12082

Comments

@amygilbert
Copy link

amygilbert commented May 22, 2020

What should happen?

A group of radio buttons with the 'required' attribute on one of them: Clicking submit without selecting one should add the is-invalid-input and is-invalid-label classes, and the data-invalid attribute to each radio button in the group.

Using the browser's inspector after submit (Foundation 6.6.1):

<fieldset>
<legend>Radio Group</legend>
<input type="radio" name="exampleRadio" id="exampleRadioA" value="A" class>
<label for="exampleRadioA" class>A</label>
<input required type="radio" name="exampleRadio" id="exampleRadioB" value="B" class>
<label for="exampleRadioB" class>B</label>
<input type="radio" name="exampleRadio" id="exampleRadioC" value="C" class="is-invalid-input" data-invalid aria-invalid="true">
<label for="exampleRadioC" class="is-invalid-label">C</label>
</fieldset>

...

What happens instead?

Only the last element (even if that's not the one that has the 'required' attribute) gets the classes and data attribute.

Using the browser's inspector after submit (Foundation 6.6.3):

<fieldset>
<legend>Radio Group</legend>
<input type="radio" name="exampleRadio" id="exampleRadioA" value="A" class>
<label for="exampleRadioA" class>A</label>
<input required type="radio" name="exampleRadio" id="exampleRadioB" value="B" class>
<label for="exampleRadioB" class>B</label>
<input type="radio" name="exampleRadio" id="exampleRadioC" value="C" class="is-invalid-input" data-invalid aria-invalid="true">
<label for="exampleRadioC" class="is-invalid-label">C</label>
</fieldset>

...

Possible Solution

...

Test Case and/or Steps to Reproduce (for bugs)

Test Case: https://codepen.io/amygilbert/pen/RwWvQQp
this codepen uses the 6.6.3 cdnjs. If you switch it to 6.6.1, you'll see that it works as expected.

How to reproduce:

  1. Create a form with data-abide and novalidate attributes
  2. Put in a set of radio buttons ()
  3. Add a submit button
  4. Click the submit button

Context

...

Your Environment

  • Foundation version(s) used: 6.6.3
  • Browser(s) name and version(s): Version 81.0.4044.138 (Official Build) (64-bit) (but it happens in all browsers I've checked)
  • Device, Operating System and version: Windows Version 10.0.18363 Build 18363
  • Link to your project: https://phoenixsoftware.com/download_entrypoint.htm (I have a workaround for the issue so you won't see the problem on my site.)

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • There are no other issues similar to this one.
  • The issue title and template are correctly filled.
@DanielRuf
Copy link
Contributor

DanielRuf commented May 22, 2020

Hi @amygilbert,

thanks for reporting this.

It seems this was already broken in v6.6.2.
I did some bisecting and this causes this:

5eac681#diff-37998d120faf57b3e7580109c8e306e2

5eac68172d86e27e8cbc0e76cf3ffc007b8e63f0 is the first bad commit
commit 5eac68172d86e27e8cbc0e76cf3ffc007b8e63f0
Author: soylent <soylent>
Date:   Sat Feb 23 18:08:40 2019 -0700

    feat(abide): add validator-specific error messages
    
    Now you can add different error messages for each validator using the
    `[data-form-error-on]` attribute.
    
    <input required type="email">
    <span class="form-error" data-form-error-on="required">Required</span>
    <span class="form-error" data-form-error-on="pattern">Invalid</span>
    
    BREAKING CHANGE: `Foundation.Abide#validateText()` no longer checks if
    the input is required. Use the `Foundation.Abide#requiredCheck()` method
    for that.
    
    Closes #10799

 docs/pages/abide.md                   |  22 +++++++
 js/foundation.abide.js                |  64 +++++++++++-------
 test/visual/abide/error-messages.html | 120 ++++++++++++++++++++++++++++++++++
 3 files changed, 181 insertions(+), 25 deletions(-)
 create mode 100644 test/visual/abide/error-messages.html

@soylent can you take a look at this? This would be really great.

@DanielRuf
Copy link
Contributor

This might be the cause:

5eac681#diff-37998d120faf57b3e7580109c8e306e2L485

I will prepare a PR but I need a review - cc @soylent @SassNinja @joeworkman

@lirael
Copy link
Contributor

lirael commented Jun 3, 2020

Hi @DanielRuf, thanks for quickly finding the solution! Do you know when it might get merged?

Also, I noticed another problem which might be related to this, so wanted to verify if that PR fixes it too. When I have this group of checkboxes:

<fieldset>
    <label class="checkbox_label">
        <input type="checkbox" name="answer" value="a">Option A
    </label>
    <label class="checkbox_label">
        <input type="checkbox" name="answer" value="b">Option B
    </label>
    <label class="checkbox_label">
        <input type="checkbox" name="answer" value="c" required>Option C
    </label>
    <label class="checkbox_label">
        <input type="checkbox" name="answer" value="d" required>Option D
    </label>
</fieldset>

it is supposed to make sure that c and d are always selected, and it was working like this in Foundation 6.5.3. However, after the upgrade, validation passes if at least 1 checkbox is selected (any of them), and also highlights all of them red (even not required ones).

Please, let me know if this is unrelated and I should create a separate issue for it. Thanks!

@DanielRuf
Copy link
Contributor

Hi @lirael,

I am waiting for feedback from the team, including @SassNinja and @joeworkman.

Your issue might be related to the changes that we introduced in the past.
See #11720

Now one required checkbox will make the whole group/fieldset required.

See also the addition in the documentation: https://github.com/foundation/foundation-sites/pull/11720/files

Does this help with your problem?

@DanielRuf
Copy link
Contributor

If you can tests the changes from #12082, a review would be very helpful.

DanielRuf added a commit that referenced this issue Jul 9, 2020
…asses

fix: keep the already set input error classes - closes #12081
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants