Problem/Motivation

I tried to find out any issue in drupal installation, I got "Update Notifications" repeated in "Configure site" for the following section (image attached)

Update Notification Image

Proposed resolution

I am not sure, Is it necessary? I also checked some others fields for checkboxes type, sometimes I saw they had no #title. I think we can keep the #title of fieldgroup section and we can remove #title for the checkboxes field.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

imrancluster created an issue. See original summary.

imrancluster’s picture

imrancluster’s picture

Status: Active » Needs review
imrancluster’s picture

Sorry for the above app. Today I run git pull command, I got some error when I was trying to install Drupl 8.2.x dev version. Then I run composer install command. Then my composer.lock was modified. With my first patch composer.lock file was added.

Here is a updated patch

royal121’s picture

FileSize
11.99 KB
10.81 KB

The patch applies. Attached before and after screenshots.

himanshugautam’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested , it works fine

alexpott’s picture

Issue summary: View changes

This patch introduces a visual inconsistency since now there is an odd gap between the title and the checkboxes. I'll discuss this with frontend maintainers to see if this is ok.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

For accessibility reasons we should not be removing the title alltogether, but instead use #title_display => invisible. Then screen readers can still make out what the checkboxes are for.

himanshugautam’s picture

changes made as per @tstoeckler described in comment #8

himanshugautam’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: update_notification_title-2691971-01.patch, failed testing.

himanshugautam’s picture

himanshugautam’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: update-notification-title-2691971-12.patch, failed testing.

himanshugautam’s picture

himanshugautam’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Status: Needs review » Needs work

The patch still removes the current #title. That should not be the case. Instead we should just be *adding* the #title_display to the existing form array.

imrancluster’s picture

@tstoeckler Before I deleting #title, May be I found some checkbox related fields in core, they had no #title. Then I tried to use #title_display with previous #title but that was not working with this field.

star-szr’s picture

#title_display invisible seems to work just fine on my end and that does seem like the right thing to use here. From _form_validate() inline comments:

        // Although discouraged, a #title is not mandatory for form elements. In
        // case there is no #title, we cannot set a form error message.
        // Instead of setting no #title, form constructors are encouraged to set
        // #title_display to 'invisible' to improve accessibility.

I think the visual gap is OK (#7) because it's just obeying the form styling of Seven (or whatever theme might be used in an install profile or whatever). If we want to try to adjust that, IMO it could be done in a separate issue.

tim.plunkett’s picture

Issue tags: +a

That comment was rewritten about 4 years ago:
\Drupal\Core\Form\FormValidator::doValidateForm:

        // A #title is not mandatory for form elements, but without it we cannot
        // set a form error message. So when a visible title is undesirable,
        // form constructors are encouraged to set #title anyway, and then set
        // #title_display to 'invisible'. This improves accessibility.

Also this contradicts the intention of #933004: Test that all form elements have a title for accessibility. We want to require #title.

alexpott’s picture

Issue tags: -a +Accessibility

Taking a guess at the tag @tim.plunkett was trying to add :)

imrancluster’s picture

Assigned: imrancluster » Unassigned
star-szr’s picture

Yeah, I may have been accidentally searching a D7 codebase (I blame my head cold). Either way we agree, #title should be there. Thanks @tim.plunkett!

tim.plunkett’s picture

Thanks @alexpott, you guessed correctly :)

alexpott’s picture

So with regards to the accessibility point - won't someone using a screen reader be just as put out out hearing "Update notifications" twice as sighted people are by seeing it?

tstoeckler’s picture

@alexpott: I thought about that too, but as far as I know it is still important to have an element that has a proper label-for attribute for the respective form element. Keep in mind that I don't know very much about accessibility, though, so that might be incorrect.

lluvigne’s picture

Status: Needs work » Needs review
FileSize
39.21 KB
36.71 KB
670 bytes
717 bytes

This works fine to me (attached images). Only adding '#title_display' => 'invisible' seems to work. I don't know why remove the title... Maybe this patch apply to accessibility needs.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Verified locally + Patch looks great = RTBC

Thanks!!!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

Actually, I guess we need someone from the accessibility team reviewing this with regards to #25/#26.

tstoeckler’s picture

emallove’s picture

Reviewed and tested.

John Cook’s picture

Status: Needs review » Needs work

With regards to #29, I've had a listen to the page and the title is still heard by the screen reader.

The patch adds the class "visually-hidden" which leaves the title readable.

From https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... :

invisible: #title is rendered as a label element before the form element in the page markup, and is made invisible with the Drupal 7 .element-invisible system CSS class (system.base.css). This makes #title remain available to screen-reader users, but hides it from being displayed visually in the browser.

If the title's class is set to "hidden" then the screen reader does not read the second title.

For reference, I'm testing on a Mac using Apple VoiceOver as the reader, so testing on a proper screen reader is suggested.

andrewmacpherson’s picture

I think it would be better to adjust the form structure instead to eliminate the duplicate title entirely, instead of messing about with #title_display.

The reason we have the repeated title is that we are using the #checkboxes FAPI element when we don't really need to. The #checkboxes element gets rendered as a fieldset + label; normally this is a good thing for a11y, as it groups related options (e.g. red, green, blue) together with a label which describes them collectively.

In this case however, we have the outer fieldset, which only contains a #checkboxes element with the same title, so we have this repeated, nested fieldset + legend. Furthermore, the #options we have here aren't the sort of related options which warrant a #checkboxes. The first checkbox is about whether to enable Update module at all, and the second checkbox governs just one of Update module's settings.

If we replace the #checkboxes FAPI element with two separate #checkbox elements, we won't have the confusing nested fieldset + label, and it should achieve the same visual improvement. The checkboxes will still have their own clearly associated labels.

Removing the novice tag; we'll need to rework the submit handler to use the new keys to save the config, and I expect there might be some tests which need updating too.

andrewmacpherson’s picture

@John Cook - thanks for the Voiceover testing.

For reference, I'm testing on a Mac using Apple VoiceOver as the reader, so testing on a proper screen reader is suggested.

LOL. Voiceover is a proper screenreader, quite a full-featured one in fact ;-)

andrewmacpherson’s picture

New patch, does what I suggest in #33. We no longer get a nested fieldset.

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

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

andrewmacpherson’s picture

Issue tags: +SprintWeekend2017

This would be a good issue to review at the upcoming Global Sprint Weekend 2017. The patch in #35 has test coverage.

Queueing a new test, the previous one in Jun 2016 passed.

mgifford’s picture

Issue tags: +SprintWeekend2017a11y

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ok_lyndsey’s picture

Testing installing 8.3.x using simplytest.me until getting to configuration screen with this message

NDVA screen displays only one heading for Update notification
Screen reader reads: "Update notification grouping" "Check for updates automatically checkbox checked" "Receive notification checkbox checked"

Heading Update notification is only read out once which is intended screen reader behaviour.

Tested to compare against 8.2.5 install via simplytest.me to check previous behaviour - 8.2.5 had duplicate "update notification grouping" title and both read out.

Therefore behaviour of patch from #35 in 8.3.x works as intended for screen reader.

andrewmacpherson’s picture

@ok_lyndsey: That's great, thanks for the screen reader test. I'm in the same room as @JohnCook, and we've being testing here with VoiceOver and ChromeVox.

So that's the manual screen-reader testing done.

Next we need a code review of the changes to (1) the form and (2) the tests.

@JohnCook you okay to look at that side?

John Cook’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
60.5 KB

Also tested with simplytest.me. The output is shown below.

Tested with VoiceOver an the text isn't duplicated.

The code is good and, therefore, ready to be committed.

tstoeckler’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1203,10 +1203,8 @@ protected function installParameters() {
-          'update_status_module' => array(
-            1 => NULL,
-            2 => NULL,
-          ),
+          'enable_update_status_module' => NULL,
+          'enable_update_status_emails' => NULL,

I verified that this does not break Drush installation. Drush will need a small follow-up PR after this, but no error or notice is raised, so this should be good to go.

andrewmacpherson’s picture

@tstoeckler Thanks! Naturally, I completely failed to think of the impact this might have on Drush :-)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. If we adjust the form structure, then I believe that we need to also provide an upgrade path for the newly named settings so that people don't stop getting emails suddenly, no? We could instead do something more like #27, but I believe that still requires a11y team sign-off.

tstoeckler’s picture

Re #45: The module installation logic and the config structure is unchanged as can be seen from the patch context (i.e. there are no +'s or -'s in front of these lines):

+++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
@@ -270,13 +268,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
       $this->moduleInstaller->install(array('file', 'update'), FALSE);
...
         $this->config('update.settings')->set('notification.emails', array($account_values['mail']))->save(TRUE);

So there is no need for an upgrade path. The only thing that needs an update is automated scripts that install Drupal without using the UI, in other words: Drush. I already verified however, that Drush doesn't break with this, we will just need a small follow-up there for correctness sake, as with this change we will be submitting bogus form values via Drush, but that shouldn't really hurt anyone.

andrewmacpherson’s picture

Re: #45/46 @tstoeckler is right, the stored config keys don't change, so this patch only affects the UI of a site installation. Existing sites do not need a config update.

These two checkboxes are slightly unusual in that they don't directly map to config; instead the submit handlers decide what to set.

The first checkbox is about whether to enable the update module at all during site-installation.
The second checkbox is about partial configuration of the Update module. If checked, the site email address is used as the initial value for Update module's list of emails to notifiy (i.e. the checkbox causes a text area to be populated in the main Update module settings page).

In cases where the list of email addresses has been changed after site-install, no update is needed.

For cases where Update module was not enabled at installation time, and is enabled later on, there is no effect.

@tstoeckler - Back to RTBC?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I think all questions have been answered.

xjm’s picture

xjm’s picture

xjm’s picture

And adding an 8.4.x test run.

xjm’s picture

Issue tags: +Needs followup

I've carefully read over the discussion here. Thank you so much @andrewmacpherson, @John Cook, @ok_lyndsey, and others for the very thorough accessibility testing and clear documentation of it. Thank you also for confirming that no upgrade path is needed.

I actually think the updated form is much clearer in general. As is often the case, fixing an accessibility issue also improves it for all users.

This looks ready for commit, but I am waiting for the 8.4.x test run to complete, since it was only tested against 8.2.x. In the meanwhile, saving issue credit.

Tagging "Needs followup" for the Drush PR.

  • xjm committed e4ae1c1 on 8.4.x
    Issue #2691971 by imrancluster, lluvigne, andrewmacpherson, royal121,...

  • xjm committed b97e6a7 on 8.3.x
    Issue #2691971 by imrancluster, lluvigne, andrewmacpherson, royal121,...
xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

I read @tstoeckler's comment:

I already verified however, that Drush doesn't break with this, we will just need a small follow-up there for correctness sake, as with this change we will be submitting bogus form values via Drush, but that shouldn't really hurt anyone.

Given that information, and since this is an accessibility bugfix, I also chose to backport the fix to 8.3.x while we are still in alpha. Thanks everyone! Committed to 8.4.x and 8.3.x

So now we just need someone to file the Drush followup.

Status: Fixed » Closed (fixed)

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

pfrenssen’s picture

I created the followup for Drush in two versions, one for the current 8.x branch and one for the master branch:

pfrenssen’s picture

Issue tags: -Needs followup

I also fixed this in the "CI fork" of drupal-project: https://github.com/pfrenssen/drupal-project/pull/1

For people that are looking here how to install with drush on systems that have no mail transfer agent (e.g. sendmail or smtp) installed, here is the new way to do it:

$ drush site-install standard install_configure_form.enable_update_status_module=NULL install_configure_form.enable_update_status_emails=NULL