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.

CommentFileSizeAuthor
#72 2595613-72.patch5.56 KBroyal121
#57 2595613-57-fail.patch5.49 KBswentel
#57 2595613-57.patch4.68 KBswentel
#57 2595613-interdiff.txt1.99 KBswentel
Radio button title hiding.jpg37.29 KBkrknth
#3 2595613-1.patch721 byteskrknth
#13 Before-patch.png250.28 KBkrknth
#13 After-patch.png70 KBkrknth
#14 2595613-14.patch1.55 KBaerozeppelin
#14 interdiff-2595613-1-14.txt866 bytesaerozeppelin
#18 2595613-18.patch1.58 KBkrknth
#18 interdiff-2595613-14-18.txt668 byteskrknth
#26 2595613-26.patch3.58 KBheykarthikwithu
#26 interdiff-2595613-18-26.txt1.71 KBheykarthikwithu
#32 2595613-32.patch3.72 KBheykarthikwithu
#32 interdiff-2595613-26-32.txt856 bytesheykarthikwithu
#36 forms-2595613-36.patch3.92 KBk4v
#36 interdiff-2595613-32-36.txt1.39 KBk4v
#41 forms-2595613-36.patch3.92 KBk4v
#44 2595613-44.patch3.93 KBnaveenvalecha
#55 2595613-interdiff.txt550 bytesswentel
#55 2595613-54.patch3.93 KBswentel
#60 2595613-interdiff.txt807 bytesswentel
#60 2595613-60.patch5.32 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

krknth created an issue. See original summary.

krknth’s picture

Issue summary: View changes
krknth’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
721 bytes

Patch attached!

suggestions required

ameymudras’s picture

This issue also existed for the "checkboxes" and the above patch seems to fix it. Looks like RTBC to me

krknth’s picture

Priority: Normal » Major
Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Awesome :) This fixes quite a few issues around core.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2595613-1.patch, failed testing.

krknth’s picture

Status: Needs work » Reviewed & tested by the community

Restarted test & passing. Moving to RTBC as it was moved by BOT

krknth’s picture

Issue summary: View changes
xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for the patch!

I think we probably add some test coverage for this?

krknth’s picture

No idea how to write tests for these :( .

alexpott’s picture

Issue tags: +Needs screenshots

It would be also great to have a screenshot of a site with the patch applied.

krknth’s picture

FileSize
250.28 KB
70 KB

Attached 2 images. Before patch & after patch for install page.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
866 bytes

Added test for the screenshots uploaded in #13. Just wondering, do we need more tests?

malcomio’s picture

Status: Needs review » Reviewed & tested by the community

The patch applies cleanly, and fixes the issue on the three places mentioned.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/form.inc
@@ -206,6 +206,11 @@ function template_preprocess_fieldset(&$variables) {
   $variables['legend_span']['attributes'] = new Attribute();
 
+  // Add 'visually-hidden' class to legend span.
+  if ($element['#title_display'] == 'invisible') {
+    $variables['legend_span']['attributes'] = new Attribute(array('class' => 'visually-hidden'));
+  }
+

This means we create the Attribute instance once, then immediately overwrite it. Why not the empty Attribute in an else?

krknth’s picture

Assigned: Unassigned » krknth

I'm working on

krknth’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
668 bytes

@catch : done changes.

added patch & interdiff files

bojanz’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs screenshots

Tested locally, still works fine. Time to get it in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2595613-18.patch, failed testing.

swentel’s picture

Status: Needs work » Reviewed & tested by the community

Testbot random failure

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The 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

swentel’s picture

Also, getting confused, the issue title talks about radios, but the patch is actually adding code to the fieldset preprocess function ..

heykarthikwithu’s picture

Assigned: krknth » heykarthikwithu

working on this.

tstoeckler’s picture

Re #23: Radios (and checkboxes, ...) are wrapped in a fieldset for accessibility. See CompositeFormElementTrait.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
3.58 KB
1.71 KB

As per #22, added a test case at Elements Label test.

swentel’s picture

  1. +++ b/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
    @@ -93,6 +93,9 @@ function testFormLabels() {
    +    $elements = $this->xpath('//span[contains(@class, \'visually-hidden\')]');
    +    $this->assertTrue(isset($elements[0]), "Title/Label not displayed when 'visually-hidden' attribute is set");
       }
    

    I'd just add a single line so we know what we're testing.

  2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php
    @@ -109,6 +109,27 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +    $form['form_checkboxes_title_invisible'] = array(
    +      '#type' => 'checkboxes',
    +      '#title' => 'Checkboxes test invisible',
    +      '#title_display' => 'invisible',
    +      '#options' => array(
    +        'first-checkbox' => 'First checkbox',
    +        'second-checkbox' => 'Second checkbox',
    +      ),
    +      '#required' => TRUE,
    +    );
    +    $form['form_radios_title_invisible'] = array(
    +      '#type' => 'radios',
    

    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.

SKAUGHT’s picture

see related issue: switching from a fieldset to a container as solution.

swentel’s picture

@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.

heykarthikwithu’s picture

So we're adding two new elements, but only testing one ? Or am I missing something ?

@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.

swentel’s picture

@heykarthikwithu

Well, the assert only tests for the first key, while it contains two elements, I'd check for both keys to be there.

    $this->assertTrue(isset($elements[0]), "Title/Label not displayed when 'visually-hidden' attribute is set");

vs

Array
(
    [0] => SimpleXMLElement Object
        (
            [@attributes] => Array
                (
                    [class] => visually-hidden fieldset-legend js-form-required form-required
                )

        )

    [1] => SimpleXMLElement Object
        (
            [@attributes] => Array
                (
                    [class] => visually-hidden fieldset-legend js-form-required form-required
                )

        )

)
heykarthikwithu’s picture

yes, @swentel :)

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Thanks :)

Moving back to RTBC to see what committers think about it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
@@ -93,6 +93,10 @@ function testFormLabels() {
+    $elements = $this->xpath('//span[contains(@class, \'visually-hidden\')]');
+    $this->assertTrue(isset($elements[0]), "Title/Label not displayed when 'visually-hidden' attribute is set in checkboxes.");
+    $this->assertTrue(isset($elements[1]), "Title/Label not displayed when 'visually-hidden' attribute is set in radios.");

This test looks fragile to me I think we should be ensuring the each assert is for the expected form element.

k4v’s picture

Assigned: Unassigned » k4v
Issue tags: +SprintWeekendBerlin, +SprintWeekend2016
k4v’s picture

This test is more specific.

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: forms-2595613-36.patch, failed testing.

k4v’s picture

Status: Needs work » Needs review
swentel’s picture

Status: Needs review » Reviewed & tested by the community

Yes, looks much better, should adress @alexpott's concerns.
RTBC if green (go bot!)

k4v’s picture

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

Moving to 8.1.x for testing - patch doesn't apply afaict.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: forms-2595613-36.patch, failed testing.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
3.93 KB

Straight reroll

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

  • catch committed 5016b9c on
    Issue #2595613 by krknth, heykarthikwithu, k4v, aerozeppelin,...

  • catch committed 62d6ff4 on
    Issue #2595613 by krknth, heykarthikwithu, k4v, aerozeppelin,...
catch’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and 8.0.x, thanks! (separate commits since this won't cherry-pick cleanly).

tduong’s picture

PHP Notice bug found after this commit. Opened followup: #2665202: PHP Notice bug due to the recent #2595613 commit.

tduong’s picture

  • xjm committed 3cd78b1 on
    Revert "Issue #2595613 by krknth, heykarthikwithu, k4v, aerozeppelin,...

  • xjm committed f459072 on
    Revert "Issue #2595613 by krknth, heykarthikwithu, k4v, aerozeppelin,...
xjm’s picture

Status: Fixed » Needs work

Reverted this based on #49 after discussing with @tim.plunkett and @berdir.

tim.plunkett’s picture

Title: Forms - '#title_display' => 'invisible' is not working for radio buttons ? » '#title_display' => 'invisible' does not work for composite form elements
Issue tags: +Needs tests
swentel’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
3.93 KB
550 bytes

Using 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.

The last submitted patch, 55: 2595613-54.patch, failed testing.

swentel’s picture

And 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 :)

The last submitted patch, 57: 2595613-57-fail.patch, failed testing.

The last submitted patch, 57: 2595613-57.patch, failed testing.

swentel’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I 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.)

Wim Leers’s picture

alexpott’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

alexpott’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Reviewed & tested by the community

Ah... does not apply to 8.0.x

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Committed a08cba3 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 229b1a5 on 8.2.x
    Issue #2595613 by swentel, krknth, heykarthikwithu, k4v, aerozeppelin,...

  • alexpott committed a08cba3 on 8.1.x
    Issue #2595613 by swentel, krknth, heykarthikwithu, k4v, aerozeppelin,...
tstoeckler’s picture

Version: 8.2.x-dev » 8.0.x-dev

So this did not get re-committed to 8.0.x as far as I can tell.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

If we want this is 8.0.x we need to reroll it.

The last submitted patch, 60: 2595613-60.patch, failed testing.

royal121’s picture

Status: Needs work » Needs review
FileSize
5.56 KB

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Status: Needs review » Fixed

8.0 is unsupported now, so going back to fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.