Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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)
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.
Comment | File | Size | Author |
---|---|---|---|
#35 | update_notifications_title-2691971-35.patch | 4.29 KB | andrewmacpherson |
#35 | interdiff-2691971-27-35.txt | 3.95 KB | andrewmacpherson |
#35 | update_notifications_title-2691971-AFTER-patch-35.png | 22.45 KB | andrewmacpherson |
Comments
Comment #2
imrancluster CreditAttribution: imrancluster commentedHere is a patch I created for this issue.
Comment #3
imrancluster CreditAttribution: imrancluster commentedComment #4
imrancluster CreditAttribution: imrancluster commentedSorry 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
Comment #5
royal121 CreditAttribution: royal121 commentedThe patch applies. Attached before and after screenshots.
Comment #6
himanshugautam CreditAttribution: himanshugautam commentedReviewed and tested , it works fine
Comment #7
alexpottThis 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.
Comment #8
tstoecklerFor 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.Comment #9
himanshugautam CreditAttribution: himanshugautam commentedchanges made as per @tstoeckler described in comment #8
Comment #10
himanshugautam CreditAttribution: himanshugautam commentedComment #12
himanshugautam CreditAttribution: himanshugautam commentedComment #13
himanshugautam CreditAttribution: himanshugautam commentedComment #15
himanshugautam CreditAttribution: himanshugautam commentedComment #16
himanshugautam CreditAttribution: himanshugautam commentedComment #17
tstoecklerThe 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.
Comment #18
imrancluster CreditAttribution: imrancluster commented@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.
Comment #19
star-szr#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:
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.
Comment #20
tim.plunkettThat comment was rewritten about 4 years ago:
\Drupal\Core\Form\FormValidator::doValidateForm
:Also this contradicts the intention of #933004: Test that all form elements have a title for accessibility. We want to require #title.
Comment #21
alexpottTaking a guess at the tag @tim.plunkett was trying to add :)
Comment #22
imrancluster CreditAttribution: imrancluster commentedComment #23
star-szrYeah, 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!
Comment #24
tim.plunkettThanks @alexpott, you guessed correctly :)
Comment #25
alexpottSo 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?
Comment #26
tstoeckler@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.
Comment #27
lluvigneThis 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.
Comment #28
tstoecklerVerified locally + Patch looks great = RTBC
Thanks!!!
Comment #29
tstoecklerActually, I guess we need someone from the accessibility team reviewing this with regards to #25/#26.
Comment #30
tstoecklerComment #31
emallove CreditAttribution: emallove commentedReviewed and tested.
Comment #32
John Cook CreditAttribution: John Cook commentedWith 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... :
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.
Comment #33
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI 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.
Comment #34
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented@John Cook - thanks for the Voiceover testing.
LOL. Voiceover is a proper screenreader, quite a full-featured one in fact ;-)
Comment #35
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedNew patch, does what I suggest in #33. We no longer get a nested fieldset.
Comment #37
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis 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.
Comment #38
mgiffordComment #40
ok_lyndsey CreditAttribution: ok_lyndsey as a volunteer commentedTesting 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.
Comment #41
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@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?
Comment #42
John Cook CreditAttribution: John Cook commentedAlso 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.
Comment #43
tstoecklerI 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.
Comment #44
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@tstoeckler Thanks! Naturally, I completely failed to think of the impact this might have on Drush :-)
Comment #45
webchickHm. 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.
Comment #46
tstoecklerRe #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):
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.
Comment #47
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe: #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?
Comment #48
tstoecklerYes, I think all questions have been answered.
Comment #49
xjmSomehow the file attachments are in reverse order, so hiding the old ones (including one that has a bad diff).
Comment #50
xjmComment #51
xjmAnd adding an 8.4.x test run.
Comment #52
xjmI'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.
Comment #55
xjmI read @tstoeckler's comment:
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.
Comment #57
pfrenssenI created the followup for Drush in two versions, one for the current 8.x branch and one for the master branch:
Comment #58
pfrenssenI also fixed this in the "CI fork" of
drupal-project
: https://github.com/pfrenssen/drupal-project/pull/1For 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: