I noticed that hiding title/label for radio buttons are not working with '#title_display' => 'invisible'
in forms.
Below are the pages i noticed the same -
1)
url : admin/structure/types/manage/article/fields/node.article.comment (screenshot attached)
File : /core/modules/comment/src/Plugin/Field/FieldWidget/CommentWidget.php
2)
url (Edit any view) : admin/structure/views/view/content -> PAGE SETTINGS -> Access -> Permissions
url (Edit any view) : admin/structure/views/view/content -> PAGE SETTINGS -> Caching
File : /core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
3)
url : install.php
File : /core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
check these issue - https://www.drupal.org/node/2595501
There are many other pages. I will be adding more once community confirms that this is a bug.
Comment | File | Size | Author |
---|---|---|---|
#72 | 2595613-72.patch | 5.56 KB | royal121 |
#57 | 2595613-57-fail.patch | 5.49 KB | swentel |
#57 | 2595613-57.patch | 4.68 KB | swentel |
#57 | 2595613-interdiff.txt | 1.99 KB | swentel |
Radio button title hiding.jpg | 37.29 KB | krknth |
Comments
Comment #2
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedComment #3
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedPatch attached!
suggestions required
Comment #4
ameymudras CreditAttribution: ameymudras as a volunteer and commentedThis issue also existed for the "checkboxes" and the above patch seems to fix it. Looks like RTBC to me
Comment #5
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedComment #6
Bojhan CreditAttribution: Bojhan commentedAwesome :) This fixes quite a few issues around core.
Comment #8
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedRestarted test & passing. Moving to RTBC as it was moved by BOT
Comment #9
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedComment #10
xjmThanks for the patch!
I think we probably add some test coverage for this?
Comment #11
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedNo idea how to write tests for these :( .
Comment #12
alexpottIt would be also great to have a screenshot of a site with the patch applied.
Comment #13
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedAttached 2 images. Before patch & after patch for install page.
Comment #14
aerozeppelin CreditAttribution: aerozeppelin commentedAdded test for the screenshots uploaded in #13. Just wondering, do we need more tests?
Comment #15
malcomio CreditAttribution: malcomio commentedThe patch applies cleanly, and fixes the issue on the three places mentioned.
Comment #16
catchThis means we create the Attribute instance once, then immediately overwrite it. Why not the empty Attribute in an else?
Comment #17
krknth CreditAttribution: krknth as a volunteer and at Valuebound commentedI'm working on
Comment #18
krknth CreditAttribution: krknth as a volunteer and at Valuebound commented@catch : done changes.
added patch & interdiff files
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedTested locally, still works fine. Time to get it in.
Comment #21
swentel CreditAttribution: swentel as a volunteer commentedTestbot random failure
Comment #22
alexpottThe test added is a nice implicit test but we should have an explicit test as well - perhaps as part of \Drupal\system\Tests\Form\ElementsLabelsTest
Comment #23
swentel CreditAttribution: swentel as a volunteer commentedAlso, getting confused, the issue title talks about radios, but the patch is actually adding code to the fieldset preprocess function ..
Comment #24
heykarthikwithuworking on this.
Comment #25
tstoecklerRe #23: Radios (and checkboxes, ...) are wrapped in a fieldset for accessibility. See
CompositeFormElementTrait
.Comment #26
heykarthikwithuAs per #22, added a test case at Elements Label test.
Comment #27
swentel CreditAttribution: swentel as a volunteer commentedI'd just add a single line so we know what we're testing.
So we're adding two new elements, but only testing one ? Or am I missing something ?
I'd also add a line here why we're setting this, just for clarity, just like is happening in the rest of this form.
Comment #28
SKAUGHTsee related issue: switching from a fieldset to a container as solution.
Comment #29
swentel CreditAttribution: swentel as a volunteer commented@SKAUGHT hmm, well no, that would only fix it for the install screen, that wouldn't fix it for the other implementations in other screens, core, contrib or custom. If we fix this one, the other one is unnecessary.
Comment #30
heykarthikwithu@swentel, yes we are adding one checkbox and other radio button.
and both fields are been set to
'#title_display' => 'invisible',
And in our test case, we are loading all the span elements where the span has
'visually-hidden'
class.$elements = $this->xpath('//span[contains(@class, \'visually-hidden\')]');
So, in this case we write one test which checks all the span elements in those
'visually-hidden'
class would be existing.Comment #31
swentel CreditAttribution: swentel as a volunteer commented@heykarthikwithu
Well, the assert only tests for the first key, while it contains two elements, I'd check for both keys to be there.
vs
Comment #32
heykarthikwithuyes, @swentel :)
Comment #33
swentel CreditAttribution: swentel as a volunteer commentedThanks :)
Moving back to RTBC to see what committers think about it.
Comment #34
alexpottThis test looks fragile to me I think we should be ensuring the each assert is for the expected form element.
Comment #35
k4v CreditAttribution: k4v as a volunteer commentedComment #36
k4v CreditAttribution: k4v as a volunteer commentedThis test is more specific.
Comment #37
k4v CreditAttribution: k4v as a volunteer commentedComment #39
k4v CreditAttribution: k4v as a volunteer commentedComment #40
swentel CreditAttribution: swentel as a volunteer commentedYes, looks much better, should adress @alexpott's concerns.
RTBC if green (go bot!)
Comment #41
k4v CreditAttribution: k4v as a volunteer commentedhello testbot!
Comment #42
catchMoving to 8.1.x for testing - patch doesn't apply afaict.
Comment #44
naveenvalechaStraight reroll
Comment #45
swentel CreditAttribution: swentel as a volunteer commentedThanks!
Comment #48
catchCommitted/pushed to 8.1.x and 8.0.x, thanks! (separate commits since this won't cherry-pick cleanly).
Comment #49
tduong CreditAttribution: tduong at MD Systems GmbH commentedPHP Notice bug found after this commit. Opened followup: #2665202: PHP Notice bug due to the recent #2595613 commit.
Comment #50
tduong CreditAttribution: tduong at MD Systems GmbH commentedComment #53
xjmReverted this based on #49 after discussing with @tim.plunkett and @berdir.
Comment #54
tim.plunkettComment #55
swentel CreditAttribution: swentel as a volunteer commentedUsing the $variables now.
Thinking how to write a test for the notice that we found - and especially a failing one, since that would mean uploading a test with the original fix, which is kind of silly imo.
Comment #57
swentel CreditAttribution: swentel as a volunteer commentedAnd now with tests. Tests a form in a themeless environment, only returning a fieldset for now, but can be expanded.
To make them fail, I explicitely removed the issets in template_preprocess_fieldset, see the interdiff which contains that + the new test.
Will probably throw a million notices in the test fail patch, but oh well :)
Comment #60
swentel CreditAttribution: swentel as a volunteer commentedOh sweet irony
Comment #62
Wim LeersI ran into this today. The patch still applies cleanly. Manually tested, works perfectly.
Thanks for the test coverage, @swentel!
(Ran into it while working on #2294613: Port CDN to Drupal 8.)
Comment #63
Wim LeersComment #64
alexpottNeeds a reroll.
Comment #65
alexpottAh... does not apply to 8.0.x
Comment #66
alexpottCommitted a08cba3 and pushed to 8.1.x and 8.2.x. Thanks!
Comment #69
tstoecklerSo this did not get re-committed to 8.0.x as far as I can tell.
Comment #70
alexpottIf we want this is 8.0.x we need to reroll it.
Comment #72
royal121 CreditAttribution: royal121 commentedComment #74
tstoeckler8.0 is unsupported now, so going back to fixed.