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

[Form] Make sure errors are a part of the label on bootstrap 4 - this is a requirement for WCAG2 #24435

Merged
merged 1 commit into from Feb 4, 2018

Conversation

Projects
None yet
7 participants
@Nyholm
Member

Nyholm commented Oct 5, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#...

I recently let Europe's leading accessibility experts (Funkanu.se) review a site of mine, they gave me (among other) the feedback that errors should be a part of the label.

They said that it makes no sense for blind users to read label, read input and then read errors.

I know the implementation might look strange. But I wish something like this would be merged. That would be great for accessibility for all apps using Symfony.

We could also make sure it prints something like:

<label for=”name”>Name: <span class=”hidden”>Error message</span></label>
<input id=”name” type=”text”>
<span aria-hidden=”true”>Error message</span>
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 5, 2017

Member

Is it really something that we could only implement for Bootstrap 4? Wouldn't it make sense to make the change to the other themes? Also, for Bootstrap 4, it should be done on 3.4, not master.

Member

fabpot commented Oct 5, 2017

Is it really something that we could only implement for Bootstrap 4? Wouldn't it make sense to make the change to the other themes? Also, for Bootstrap 4, it should be done on 3.4, not master.

@ostrolucky

This comment has been minimized.

Show comment
Hide comment
@ostrolucky

ostrolucky Oct 8, 2017

Contributor

Wouldn't changing this in older themes be BC break?
Bootstrap 4 theme has been added in Symfony 3.4 so it's ok to do this there.

Contributor

ostrolucky commented Oct 8, 2017

Wouldn't changing this in older themes be BC break?
Bootstrap 4 theme has been added in Symfony 3.4 so it's ok to do this there.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 8, 2017

Member

@ostrolucky I know that, but it's kind of weird to fix a real issue only for one theme.

Member

fabpot commented Oct 8, 2017

@ostrolucky I know that, but it's kind of weird to fix a real issue only for one theme.

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 Oct 8, 2017

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 8, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 8, 2017

Member

@Nyholm rebase on 3.4 needed (I already retargeted on github)

Member

nicolas-grekas commented Oct 8, 2017

@Nyholm rebase on 3.4 needed (I already retargeted on github)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

@Nyholm I just rebased on 3.4. Can you please review other comments?

Member

nicolas-grekas commented Oct 12, 2017

@Nyholm I just rebased on 3.4. Can you please review other comments?

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Oct 12, 2017

Member
Member

Nyholm commented Oct 12, 2017

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Oct 13, 2017

Member

I've updated all the themes.
Please note that the design (depending on CSS) might break with these fixes.

If we are happy with the implementation I'll try to write a paragraph or two in the docs about this and how to remove the error from the label.

Member

Nyholm commented Oct 13, 2017

I've updated all the themes.
Please note that the design (depending on CSS) might break with these fixes.

If we are happy with the implementation I'll try to write a paragraph or two in the docs about this and how to remove the error from the label.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 13, 2017

Member

tests seem to need an update too

Member

xabbuh commented Oct 13, 2017

tests seem to need an update too

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Oct 17, 2017

Member

I updated some tests. It passes locally. Travis is not all happy, did I do it correctly?

Member

Nyholm commented Oct 17, 2017

I updated some tests. It passes locally. Travis is not all happy, did I do it correctly?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 12, 2017

Member

@Nyholm could you rebase + check tests please?

Member

nicolas-grekas commented Nov 12, 2017

@Nyholm could you rebase + check tests please?

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Nov 26, 2017

Member

I've rebased and fixed one test.. I cannot figure out how to fix AbstractTableLayoutTest::testNestedFormError. Where is the documentation of DomCrawler?

Member

Nyholm commented Nov 26, 2017

I've rebased and fixed one test.. I cannot figure out how to fix AbstractTableLayoutTest::testNestedFormError. Where is the documentation of DomCrawler?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Nov 26, 2017

Member

@Nyholm Tests for the TwigBridge are green now. But it looks like you need to update some PHP templates too (see the failing FrameworkBundle tests).

Member

xabbuh commented Nov 26, 2017

@Nyholm Tests for the TwigBridge are green now. But it looks like you need to update some PHP templates too (see the failing FrameworkBundle tests).

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Nov 26, 2017

Member

Tests are green except for low/high. Also, Fabbot is a false negative

Member

Nyholm commented Nov 26, 2017

Tests are green except for low/high. Also, Fabbot is a false negative

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Nov 30, 2017

Member

Don't merge such a bc break in a minor version

Member

Tobion commented Nov 30, 2017

Don't merge such a bc break in a minor version

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 16, 2018

Member

@Nyholm how could we move forward here?

Member

nicolas-grekas commented Jan 16, 2018

@Nyholm how could we move forward here?

@ostrolucky

This comment has been minimized.

Show comment
Hide comment
@ostrolucky

ostrolucky Jan 16, 2018

Contributor

I think this is still ok to go into bootstrap 4 theme, since that one needs to be changed in BC breaking way anyway to support newer bootstrap 4 release

Contributor

ostrolucky commented Jan 16, 2018

I think this is still ok to go into bootstrap 4 theme, since that one needs to be changed in BC breaking way anyway to support newer bootstrap 4 release

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 22, 2018

Member

@Nyholm because we cannot break BC, except on bootstrap 4 because it was not even stable before, what about focusing this PR only on bootstrap 4? That'd be a nice step. Would you have some time to make it before next release? (ie this week would be great)

Member

nicolas-grekas commented Jan 22, 2018

@Nyholm because we cannot break BC, except on bootstrap 4 because it was not even stable before, what about focusing this PR only on bootstrap 4? That'd be a nice step. Would you have some time to make it before next release? (ie this week would be great)

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Jan 22, 2018

Member

Hey. Sorry for the delay. I will make sure to only change the bootstrap theme this week.

Member

Nyholm commented Jan 22, 2018

Hey. Sorry for the delay. I will make sure to only change the bootstrap theme this week.

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Jan 28, 2018

Member

There we go. The PR does only edit bootstrap 4 and added some tests to make sure it works.

I've also rebased on branch 3.4. But I guess it should be changed to master, right?

Member

Nyholm commented Jan 28, 2018

There we go. The PR does only edit bootstrap 4 and added some tests to make sure it works.

I've also rebased on branch 3.4. But I guess it should be changed to master, right?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 28, 2018

Member

This should really go to 3.4 as bug fix. Later would mean BC breaks. Doing now is also a BC break, but matches the fact that BS4 just when out of beta.

Member

nicolas-grekas commented Jan 28, 2018

This should really go to 3.4 as bug fix. Later would mean BC breaks. Doing now is also a BC break, but matches the fact that BS4 just when out of beta.

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Jan 28, 2018

Member

Great great great. Then Im done with this PR. Im ready for a review.

Member

Nyholm commented Jan 28, 2018

Great great great. Then Im done with this PR. Im ready for a review.

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Jan 29, 2018

Member

Thank you

Member

Nyholm commented Jan 29, 2018

Thank you

@fabpot

fabpot approved these changes Jan 29, 2018

@nicolas-grekas nicolas-grekas changed the title from [Form] Make sure errors is a part of the label. This is a requirement for WCAG2 to [Form] Make sure errors are a part of the label on bootstrap 4 - this is a requirement for WCAG2 Feb 4, 2018

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 4, 2018

Member

Thank you @Nyholm.

Member

nicolas-grekas commented Feb 4, 2018

Thank you @Nyholm.

@nicolas-grekas nicolas-grekas merged commit a0b40f5 into symfony:3.4 Feb 4, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 4, 2018

bug #24435 [Form] Make sure errors are a part of the label on bootstr…
…ap 4 - this is a requirement for WCAG2 (Nyholm)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Make sure errors are a part of the label on bootstrap 4 - this is a requirement for WCAG2

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

I recently let Europe's leading accessibility experts (Funkanu.se) review a site of mine, they gave me (among other) the feedback that errors should be a part of the label.

They said that it makes no sense for blind users to read label, read input and then read errors.

I know the implementation might look strange. But I wish something like this would be merged. That would be great for accessibility for all apps using Symfony.

We *could* also make sure it prints something like:

```
<label for=”name”>Name: <span class=”hidden”>Error message</span></label>
<input id=”name” type=”text”>
<span aria-hidden=”true”>Error message</span>
```

Commits
-------

a0b40f5 [Form] Make sure errors are a part of the label on bootstrap 4 - this is a requirement for WCAG2

@Nyholm Nyholm deleted the Nyholm:form-wcag-error branch Feb 4, 2018

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Feb 4, 2018

Member

Thank you for merging.

Member

Nyholm commented Feb 4, 2018

Thank you for merging.

nicolas-grekas added a commit that referenced this pull request Feb 15, 2018

bug #26167 [TwigBridge] Apply some changes to support Bootstrap4-stab…
…le (mpiot, Nyholm)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBridge] Apply some changes to support Bootstrap4-stable

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25655 #25635 #24435
| License       | MIT
| Doc PR        | -

Follow up of #25715, see discussion there.

This fixes the following errors:
- Delete form-control-label, don't used in Bootstrap 4
- Replace col-form-legend by col-form-label
- Separate the label and input (before the input was in the label)
- Use form-check-inline to put radio and/or checkboxes inline
- Add support of custom form for radio and checkboxes
- Fix input-group: MoneyType (Issue #25655), PercentType
- Remove form-control duplication (Issue #25635)
- Fix Errors in label (#24435)

Commits
-------

14e2282 Fixed broken tests
cf4e956 [TwigBridge] Apply some changes to support Bootstrap4-stable

This was referenced Mar 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment