Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Make sure errors are a part of the label on bootstrap 4 - this is a requirement for WCAG2#24435
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedOct 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 commentedOct 8, 2017
Wouldn't changing this in older themes be BC break? |
fabpot commentedOct 8, 2017
@ostrolucky I know that, but it's kind of weird to fix a real issue only for one theme. |
nicolas-grekas commentedOct 8, 2017
@Nyholm rebase on 3.4 needed (I already retargeted on github) |
7ef748d to485c929Comparenicolas-grekas commentedOct 12, 2017
@Nyholm I just rebased on 3.4. Can you please review other comments? |
Nyholm commentedOct 12, 2017 via email
Thank you for reviews and comments. I'll take care of this tonight.//Tobias NyholmSkickat på språng … On 12 Oct 2017, at 09:16, Nicolas Grekas ***@***.***> wrote:@Nyholm I just rebased on 3.4. Can you please review other comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread. |
Nyholm commentedOct 13, 2017
I've updated all the themes. 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 commentedOct 13, 2017
tests seem to need an update too |
Nyholm commentedOct 17, 2017
I updated some tests. It passes locally. Travis is not all happy, did I do it correctly? |
nicolas-grekas commentedNov 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Nyholm could you rebase + check tests please? |
Nyholm commentedNov 26, 2017
I've rebased and fixed one test.. I cannot figure out how to fix |
| [ | ||
| ./tr/td/ul[./li[.="[trans]Error![/trans]"]] | ||
| ./tr/td/label/ul[./li[.="[trans]Error![/trans]"]] | ||
| /following-sibling::table[@id="name_child"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line is wrong. I cannot figure out how to make it right. It should match:
<tableid="my&id"class="my&class"><tr><td><labelclass="required">[trans]Child[/trans]<ul><li>[trans]Error![/trans]</li></ul></label></td><td><tableid="name_child"><tr><td><labelclass="required">[trans]Grand child[/trans]</label></td><td><tableid="name_child_grandChild"></table></td></tr></table></td></tr><trstyle="display: none"><tdcolspan="2"><inputtype="hidden"id="name__token"name="name[_token]"></td></tr></table>
xabbuh commentedNov 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 commentedNov 26, 2017
Tests are green except for low/high. Also, Fabbot is a false negative |
| {%-else -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' form-control-label')|trim}) -%} | ||
| {%-endif -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' form-control-label')|trim}) -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the indention is broken
Tobion commentedNov 30, 2017
Don't merge such a bc break in a minor version |
nicolas-grekas commentedJan 16, 2018
@Nyholm how could we move forward here? |
ostrolucky commentedJan 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 commentedJan 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 commentedJan 22, 2018
Hey. Sorry for the delay. I will make sure to only change the bootstrap theme this week. |
82ee2da to3eb9e41CompareNyholm commentedJan 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 commentedJan 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedJan 28, 2018
Great great great. Then Im done with this PR. Im ready for a review. |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(with minor nitpicking comments)
| {%-ifwith_invert %}{{ form_widget(form.invert) }}{%endif -%} | ||
| </div> | ||
| {%-endif -%} | ||
| {%-endblockdateinterval_widget -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the last dash could be removed, just to remove an unneeded change in the patch
| {%-setelement='legend' -%} | ||
| {%-setlabel_attr=label_attr|merge({class: (label_attr.class|default('') ~' col-form-legend')|trim}) -%} | ||
| {%-else -%} | ||
| {%-setlabel_attr=label_attr|merge({'for':id,class: (label_attr.class|default('') ~' form-control-label')|trim}) -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need for quotes around key (same below)
Nyholm commentedJan 29, 2018
Thank you |
… is a requirement for WCAG2
cee6a52 toa0b40f5Comparenicolas-grekas commentedFeb 4, 2018
Thank you@Nyholm. |
…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 commentedFeb 4, 2018
Thank you for merging. |
…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 testscf4e956 [TwigBridge] Apply some changes to support Bootstrap4-stable
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.
Wecould also make sure it prints something like: