Indicate that grouping elements have child element errors for ux and a11y

Tag clean up: "accessibility" is the preferred one. The "a11y" tag doesn't have many issues so I'm moving them all to "accessibility".

For vertical tabs we could iappend an error indication in the tab summary message.

e.g. if there is one error, say "Error: [field label]", or if there are several errors, something like "Contains multiple errors."

It doesn't help us with the details group though.

@dmsmidt Re: comment #5

Yes, an error icon (with fallback text) will do the job, better than my text suggestion.

I was trying to address "At least we can color the borders red and make them a bit thicker" from comment #2. A thick red border alone would not be sufficient to convey an error because:

• Red is not sufficient because (1) users with colour-vision impairments can have difficulty perceiving it, and (2) it may not work effectively in other colour spaces (e.g. Windows' high contrast mode). In other words, can a user distinguish between a tab with a red border, and a tab with a dark-grey border?
• A thick border has no meaning, and can convey anything. It's sometimes used to convey :focus, for instance.

Including the error icon in vertical tabs addresses these, because it uses shape as well as colour, and is a meaningful indication of "error".

Review of the patch in #9:

1. In vertical tabs, the class="error" and aria-invalid="true" attributes are applied to the the <details> element nested inside the tab panel (via PHP) and the error class is applied to the vertical tab itself via JS. I changed the names of these classes to confirm it worked.
2. Adding an error class will not be sufficient on it's own. Presumably the idea is that this class would be used to add an error icon (a red cross, say). If it's just an icon, it will need a text fallback. Ideally this would be an inline image element, but we don't seem to use many of those in core anymore. A CSS background images and/or generated content will not be presented to screen readers, so we should either use an inline image element with an alt attribute, or include a span.visually-hidden the vertical tab somehow, such as a translatable string in vertical-tabs.js. (I would put it after the tab title, but before any vertical tab summary text. This might mean prepending it to the tab summary string.)
3. I don't think it is an appropriate use of aria-invalid to put it on the container element. The WAI-ARIA recs (1.0 or 1.1CR) are a bit vague on this, saying it can be used on "all elements of the base markup". As far as I am aware though, it is only supported when it is applied to a form input element, not a div/details wrapper. In a quick test with Chromevox and the errorstyle module, it was not announced.

Steps to recreate:
1) In Drupal 8.4, create a new article content type here: /node/add/article
2) Scroll down to the URL Path Settings in the vertical tabs: /node/add/article#edit-path-settings
3) Enter in a node title and then enter in a bogus URL that doesn't start with a forward slash("/") as seen here: https://www.drupal.org/files/issues/00000003.png
4) Click "Save and Publish"
5) After saving the page, you should see an error at the top of the page like: https://www.drupal.org/files/issues/00000004.png
6) Also notice the error seen in the URL Path Settings now. Should look like this: https://www.drupal.org/files/issues/00000006.png

As part of an internal accessibility sprint we determined these next steps:

1. Identify attributes to be added to which element (summary / details)
1. Reuse error class on both.
2. Add class programmatically
   (Drupal\Core\Render\Element\RenderElement::preRenderGroup).
   (/core/misc/vertical-tabs.js (line 68ish)
3. Submit patch for HTML changes.
4. Update CSS to properly show changes.
5. Submit patch for CSS changes.
6. Identify correct ARIA attribute(s) to add.
1. Add “aria-invalid = true”?
2. Do any other CMSes try to solve this issue?
3. Are there any articles that discuss this issue?
4. No ARIA is better than bad ARIA
7. Update tests to confirm changes.
8. Submit patch with test changes.
9. (repeat steps 6 and 7 as needed)

Re: #14 - I think we've covered all of that in the various comments so far.

Identify attributes to be added to which element (summary / details)

Reuse error class on both.

Do you mean adding a .error class to the details and the summary elements? Surely we can style the summary with a selector like details.error summary.

aria-invalid=true doesn't work for anything other than form inputs, so putting it on a <details> is of no benefit.

Here's what a motley crew at Mediacurrent came up with (cehfisher, DamienMcKenna, dbungard, markie, Michelle, samuel.seide, smurrayatwork, ttamniwdoog, Widescreen_bob). This adds a border to the fieldset ("details" tag) in both expanded and collapsed formats, and then adds the error check mark to the summary text.

This is what the error message looks like without the changes:

This is what the error message looks like with the changes:

While reviewing this it was noticed that the field with the error class is not AAA compliant (red text with a reddish background color), maybe something to look at later?

This is what the fieldset looks like when it's collapsed:

We did not get into vertical tabs in the main content area, we just focused on the sidebar fieldset on node forms.

An incomplete proposal:

- How it could look with the red left border (using the wrong color red here)
- Can we do something with a ::before pseudo CSS class to add actual error wording to the container?

Re: #25

The thick red border looks smart! I expect it will work just as well inside a vertical tab header.

Using a thick border alone (comment settings in the screenshot) isn't enough IMO. The fact that it's thicker than the regular border technically means it doesn't rely on colour alone. However the thickness doesn't convey "error" - the red colour provides that. Including the error icon would be better because it's a meaningful symbol.

So far the screenshots have used the special details styling from Seven's node form. We'll need to make it work with Seven's general details style, and Bartik.

- Can we do something with a ::before pseudo CSS class to add actual error wording to the container?

No, CSS-generated text presents a few problems:

• Currently, not all browsers pass the CSS content to host accessibility APIs. Accessible Name and Description: Computation and API Mappings 1.1 will eventually require user agents to take CSS content into account - it's still at the working draft stage. Some browsers already support it, but notably IE11 does not. I don't expect this will ever be fixed in IE, so until we drop support for IE11 we should avoid including text via CSS.
  EDIT: I tried a CSS generated content test case with Chrome 57 + ChromeVox. This combination doesn't read the CSS generated content either.
• The wording needs to be translatable. So far we've only used CSS content: for a few punctuation symbols, not translatable strings. It's possible to use HTML attributes to provide CSS content (e.g. content: attr(data-foo); but I don't see how that's better than just putting it in a HTML element.

The patch in #17 adds the error class to vertical tabs via JS. We can provide a translatable string at the same time. For details elements, providing error wording in HTML would be preferable I think.

Patch from #17:

+.entity-meta details.error,
+.entity-meta details[open].error {

The entity-meta sidebar markup comes from Seven. These selectors belong in core/themes/seven/css/components/entity-meta.css.

Re: #17

While reviewing this it was noticed that the field with the error class is not AAA compliant (red text with a reddish background color), maybe something to look at later?

It has a contrast ratio of 7.02:1 which does indeed pass at level-AAA. In any case our target for the core a11y gate is 4.5:1 to meet level-AA.

I just reviewed the information at Specifying colors and contrast for accessibility - it's a bit misleading because it says we need to meet level-AA, but then mentions the enhanced contrast ratios needed for level-AAA. I should update that page to make it clearer which numbers are for level-AA. Although obviously I'd prefer we met AAA, which we do :-)

Forgot to say thank you to @andrewmacpherson for explaining why and how. Thank you @andrewmacpherson :)

We should not make any changes to Classy or Stable on this issue because they are frozen. We only want to change them on bug fixes that are non-disruptive and major enough. The first assumption should always be that we cannot change them. However, what we can do is create minimalistic styles for core and a bit nicer styles for Bartik and Seven. Then in Drupal 9 those styles will be available in Stable.

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -204,6 +204,11 @@ public static function preRenderGroup($element) {
+      $element['#attributes']['class'][] = 'error';

We should use data attribute instead of classes when the selector is used by JavaScript

Re: #35 - I think this error class will be relevant without JS. Currently we use a polyfill-type of approach for the details/summary elements, but native support is now in Blink and Firefox. Details/summary groups work in these browsers when JS is disabled - this class is used for styling in the patch, combined with the native HTML5 'open' attribute...

+details.error:not([open]) {

Adding related issue

Re-rolled and fixed #35

This needs a re-roll to account for Drupal core now using ES6 for javascript development. That change record describes the new contributor workflow for ES6 javascript. See the sections on "creating (or reviewing) core patches".

The patch in #38 changes core/misc/vertical-tabs.js, but the new ES6 file core/misc/vertical-tabs.es6.js needs to be updated too.

Re rolled.
Fixed #40.

I'll work on this patch as part of the Haarlem accessibility sprint.

Jira puts the icon inside the thick left border, maybe we can do same?

Fixed the error introduced in #43. I would suggest adding these changes to classy, I think this is the most minimal version that needs to be added to Classy.

@yoroy - that Jira style is interesting. If we put the icon inside a thick border for this issue, would we want to change our message boxes to match?

I was trying to address #26, the part where a thick border only was not considered enough. But the current error icon is already added here as well so that's ok then. Agreed that a new design is out of scope here.

1. +++ b/core/misc/vertical-tabs.es6.js
   @@ -60,7 +60,8 @@
   +            children_errors: $that.hasClass('error')
   

   +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -197,13 +197,18 @@ public static function preRenderGroup($element) {
   +      $element['#attributes']['class'][] = 'error';
   

   Could we add a JS specific indicator for the errors? Also, .error isn't the most self-documenting name for this.

   Also according to Classy's BC policy, this change isn't allowed. However, I think if we can make this more specific we could make this change since it wouldn't cause too much of risk anymore and would still be a great improvement.

2. +++ b/core/misc/vertical-tabs.es6.js
   @@ -246,6 +247,11 @@
   +      tab.item.addClass('error');
   

   Could we use the BEM modifier pattern for naming the class for the visual styles?

3. +++ b/core/themes/classy/css/components/details.css
   @@ -21,3 +21,30 @@ summary {
   +details.error:not([open]) summary:before {
   

   Changes to Classy aren't technically allowed. We should move these changes to Seven.

4. +++ b/core/themes/seven/css/components/entity-meta.css
   @@ -57,3 +57,21 @@
   +.entity-meta details.error:not([open]) {
   

   If we can come up with a more specific class name, we could probably make the CSS less specific.

This needed a re-roll following #2880007: Auto-fix ESLint errors and warnings. There was trouble with some offsets, and the first chunk in vertical-tabs.es6.js already had a trailing comma after the ESLint changes in that issue.

-            details: $that
+            details: $that,
+            children_errors: $that.data('children-errors-count')

Sorry, no interdiff file. I got errors when trying to make one, relating to the same chunk. I manually compared patches #54 and #56 prior to uploading it here.

The simplest way to announce that grouping elements have child errors in a screen reader, is for the error icon to have alternative text (see comment #11).

However, the icon is a CSS background image, on a pseudo-element, so we can't associate the text-alternative directly with the icon :-(

The workaround for this is to provide the text alternative in a span.visually-hidden, nested inside the focusable control, so it forms part of the computed accessible name.

e.g.

<details open data-drupal-selector="edit-nested-details-closed" id="edit-nested-details-closed" class="js-form-wrapper form-wrapper form-wrapper--child-error details--child-error" data-children-errors-count="1">
  <summary role="button" aria-controls="edit-nested-details-closed" aria-expanded="false" aria-pressed="false">
    :: before
    Nested details closed
    <span class="visually-hidden child-error-indicator">(contains error)</span>
  </summary>

  ...

</details>

The interesting part is that the icon disappears when the grouping element becomes open. Do we make the text fallback disappear too? I can imagine a confusing scenario like this...

• A user finds a grouping element and hear "contains errors"
• They open the grouping element, then go off browsing the rest off the page, e.g. by navigating headings
• Try to come back to the error, and not be able to find the grouping element that "contains errors".

So let's leave the text-alternative in place (visually-hidden) all the time.

I'd rather deal with the text-alternative in this issue, instead of a follow-up. I might take a crack at it tomorrow.

@dmsmidt

Can you upload a test only patch for #64?

1. +++ b/core/misc/vertical-tabs.es6.js
   @@ -56,8 +56,9 @@
   +            title: $that.find('> summary').html(),
   

   The expected behavior here is that the html contents is returned instead of a glob of all the text nodes?

2. +++ b/core/misc/vertical-tabs.es6.js
   @@ -56,8 +56,9 @@
   +            child_error_count: $that.data('child-error-count'),
   

   This isn't critical, but it is something to think about. The way jQuery is storing data attributes is changing in 3.0, http://jquery.com/upgrade-guide/3.0/#breaking-change-data-names-containing-dashes.

Manual testing of patch #64. I used the dmsmidt/errorstyle test module, and node/article/add.

I like it very much :-) I'm happy to sign off on this approach for the icon's text alternative.

A couple of points about the message text in formatPlural():

1. + '@title <span class="visually-hidden child-error-indicator">(contains an error)</span>',
   Let's shorten this to "contains error". Most native screen reader announcements are quite terse (in English at least) and little words like "an" sound out of place.
2. + '@title <span class="visually-hidden child-error-indicator">(contains @count errors)</span>
   Mixed feelings about this. When a group is closed the error icon doesn't convey the number of child errors, so we're giving a screen reader user information that we're not providing to sighted users. However when a group is open, it's very easy for a sighted user to see that it "contains 3 errors". Overall, I think we can leave this as it is - it will very likely help a user orient themselves when reviewing vertical tab titles.

This patch addresses #72.1 - a minor string change.

Alas, I found a bug.

If the node authoring information has an error, the text alternative says "contains 3 errors". You can experience this by submitting a node with the authoring date component filled in, and the time component left empty. Of course, there is only one error message, and visually only two components are marked as an error (i.e. the date and time parts). So "contains 3 errors" could be confusing for a screen reader user.

I think this is probably an edge case for datatime elements. Since it only affects one element type (and only causes a problem if it is inside a details container) can we treat this as a follow-up issue? Possibly some other compound elements might be affected.

Alternatively, if we change the plural message to "contains several errors", we can mitigate this problem, at least as far as the UI indication goes. (The data-child-error-count="3" attribute would still be wrong, but a user wouldn't experience this directly.)

i have just been locally playing with dmsmidt/errorstyle to add in time to the Datetime field.. and stumbled upon #2875131: Datetime element formats are confusing while trying to find time formatting options.. suffice it to say, the datetime element is confusing, both as a form element (DX) and it's related validation notices (UX)

similarly, the Datelist element suffers from the same oddness when it comes to how which aspects of theses combo fields are in an error'd state.

IMO these are greater issue these two elements themselves, not of this issue 'Indicate that grouping elements in details and vertical tabs... other related issue for these elements and their error notification should be created.

#74, #75, there are one more more interrelated issues regarding that (the format weirdness, the error message(s), no labels when on a non-HTML5 browser, maybe more). Some are listed in datetime.module and some are in forms system. I have a sort-term todo to triage all of these into a mini-plan to improve the overall UX of the datetime / datelist elements, the overall UX of the datetime / daterange / timestamp widgets, and unify the UX (as much as possible) for the datetime / daterange / timestamp widgets.

Looks a lot better for me! Thank you for making the changes. There are still a few issues that show up in the csslint output here.

Chatted with @mgifford about #74 in the #accessibility slack channel.

We'd like to go with "contains several errors" to avoid the confusing count in the text alternative.

Maybe when #2897601: Incorrect #children_errors count from bubbled errors in compound elements is done we can re-introduce the numeric count.

EDIT: but since were basing the single/plural phrase on #children_errors, we'd still have the plural when ther is only one compiund element.

So, compromise to get this over the line. Let's just avoid using the plural format, and always say "contains error".

Still working well.

#79 cleaned up 4 instances of the "float can't be used with display: inline-block" coding standards message.

Of the remaining coding standards messages, none of them are being introduced by lines this patch touches:

• "Using width with border-right can sometimes make elements larger than you expect." - refers to .vertical-tabs__menu-item.is-selected. I think this is fine/intentional. It's the style of the currently selected vertical tab.
• "Outlines shouldn't be hidden unless other visual changes are made." - refers to .vertical-tabs__menu-item a:focus This is definitely a false positive, vertical tabs does have an alternative visual change.
• "Unqualified attribute selectors are known to be slow." - refers to [data-vertical-tabs-panes]. I don't understand this one - we have lots of other [dir="rtl"] attribute selectors which it doesn't complain about.

#81 changed the text alternative for the error indicator, to work around the bug I noticed in #74

Does this issue need a change record for the child-error-count it's introducing? (Sorry, don't know much about JS.)

It has fixes for Bartik and Seven but not Stable/Classy, which aligns with our BC policy.

Yeah, I guess the new data-child-error-count attribute counts as small API addition affecting themers? The idea is that custom theme CSS could present a visible numeric badge with the error count, like an iOS app icon does.

We already discussed (#72, #74, #80) making use of the error count to convey "contains several errors" in the accessible text-alternative for the details error icon, but held back (in core themes) because of #2897601: Incorrect #children_errors count from bubbled errors in compound elements .

Agreed with #83 that we should create a minor change record for the Drupal.theme.verticalTab API addition so developers who use this would be aware of the new key.

Besides that:

1. +++ b/core/misc/vertical-tabs.js
   @@ -39,8 +39,9 @@
   +            child_error_count: $that.data('child-error-count')
   

   This should be camelcase

2. +++ b/core/misc/vertical-tabs.js
   @@ -138,7 +139,12 @@
      Drupal.theme.verticalTab = function (settings) {
   

   Could we add the newly added key to the docblock?

doing #85 now

New patch addresses #85, and it gave me a chance to learn the new es6 yarn watch workflow too.

Could we add the newly added key to the docblock?

I've updated the keys for Drupal.verticalTab and Drupal.theme.verticalTab which were out of sync. Now they both mention the same 3 settings keys.

I forgot to add the tag on #85

Just to confirm that the interaction and visual design are good to go here. There's ways we can make this look nicer, but what this patch does is in line with the currently available visual vocabulary of Seven theme. (See #50)

Really small comment issue fixable on commit:

+++ b/core/misc/vertical-tabs.es6.js
@@ -126,6 +127,8 @@
+   * @param {jQuery} settings.childErrorCount
+   *   The number of form errors contained inside the tab pane.

@@ -249,6 +252,10 @@
+   * @param {jQuery} settings.childErrorCount
+   *   The number of form errors contained inside the tab pane.

@param defines a jQuery object but comment suggests a number (and count($element['#children_errors']) confirms).

Thanks @gambry. I don't think a discrepancy on the data type of a parameter is something a committer should fix on commit, so setting NW for that. Let's make sure we don't have a wrong assumption in the code about the data type.

+++ b/core/misc/vertical-tabs.js
@@ -138,7 +139,12 @@
   };
 })(jQuery, Drupal, drupalSettings);
\ No newline at end of file

This file is also missing its closing newline. Sorry, disregard, that's the auto-generated ES5 file. Still getting used to that.

Sorry to bump this back over small things; I know it's been RTBC a long time without further review. (This is also why @gambry was probably reluctant to NW it.) Unfortunately most of the committers are not as fluent in JS and this makes patches with JS get delayed. I just want to let you know I'm definitely aware of how long this patch has been under review and that it's a major UX/a11y bug that affects IFE.

@dmsmidt already gave me a heads up and begged me to finish this issue :P.

But seriously, the data type comments form the review are fixed and rerolled against 8.5.x.

Happy reviewing!

Thanks for the patch @jefuri.

This is categorized as a major bug, so it's still eligible for 8.4.x. I've re-queued the test against that branch, and will review it when the tests have run.

Review of patch #95:

1. The interdiff for vertical-tabs.es6.js and vertical-tabs.es6.js looks fine. It corrects the last outstanding JS documentation nitpick that @gambry found in #93. (Good, this part is what we needed to be back at RTBC).
2. The patch in #95 applies cleanly to 8.5.x (Good)
3. It no longer applies cleanly to the 8.4.x branch. The seven.libraries.yml file was re-arranged slightly in #2887860: Allow attributes to be passed to admin blocks (admin_block theme hook). The colors.css line was moved. This should be easy to fix.

So what happens next? Do we need to have separate versions of the patch for 8.5.x and 8.4.x?

We should be able to fix this at the DrupalCon Vienna sprints and get it into 8.4.0.

The patch in #95 applies against the 8.5.x branch and is RTBC-worthy IMO.

This patch applies to the 8.4.x branch, please review.

These interdiffs now show basically the same thing, the JS documentation tweak raised by #93.
#87 - #95 (for 8.5.x branch)
#87 - #101 (for 8.4.x branch)

OK, for the sake of sanity, let's name the latest patches like this. Feel free to remove my commit credit as I didn't really do anything; these are just the other patches renamed.

Tested again, and it all works great.

Fieldset collapsed

Fieldset expanded

I also tested RTL and that works as expected as well.

@dmsmidt and I have been discussing and watching this at the DrupalCon Vienna sprints, from the same table. The tests for both branch patches in #105 are green again, we looks like we had some random testbot blips. RTBC!

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Render/Element/VerticalTabsTest.php
@@ -0,0 +1,32 @@
+    $web_assert->elementTextContains('css', '.vertical-tabs__menu-item-title .child-error-indicator', '(contains error)');

I think we might need to wait here so that vertical tab javascript has had time to run. That's why #105 appears to have random errors. I think \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement() might be what we need to use.

Thanks @alexpott for the guidance on the random errors.

The patch as a whole looks good for me now! The CR looks good as well. Thank you @dmsmidt and everyone else!

Are the screenshots in the summary current? The seven LTR screenshot looks like it's missing the right red border around the tab.

Thanks @dmsmidt!

The 8.5.x branch doesn't apply cleanly anymore. It's the vertical tabs JS file, I'll update this.

It was #2915784: 1/3 JS codestyle: camelcase that means patch #111 no longer applies. Should be an easy re-roll.

Here's the re-roll. I've only updated the 8.5.x patch from #111.

I'll have a look at the individual cases, and check screen shots are up to date. Confirm whether the Seven LTR vertical tabs issue is still a problem.

Forgot to upload the patch.

Confirmed that the LTR vertical tab is still a problem. The right border of the vertical tab does not have a red line.

I noticed that the right vertical tab border DOES become red on hover.

The RTL equivalent looks OK.

Working on a re-roll now. Patch no longer applies due to JS format clean-ups, and #2854624: Details and accordion update based on Seven Style Guide

Re-rolled the patch, ran JS prettier.
Going to look into the remaining problem with the LTR vertical tab border from #113.
There's also been some changes committed for the summary/details in Seven, so it's worth another round of manual testing, which could take me a few days.

Fixes automated test failures: deprecated JavascriptTestBase, and a CSS file declared in Seven's library.
Still to do manual tests.

The last test failure was due to switching to WebDriverTestBase. It turns out that HTML5 client-side validation happens when using chromedriver, preventing the form submitting. Since we're checking that server-side error handling produces the .child-error-indicator span, I added a novalidate attribute to FormTestChildErrorVerticalTabsForm.

This is the last blocker of #2915899: [PP-1] Enable the Inline Form Errors module in the Standard install profile.

+++ b/core/misc/vertical-tabs.es6.js
@@ -300,7 +307,7 @@
-          ).text(settings.title)),
+          ).html(settings.title)),

+++ b/core/misc/vertical-tabs.js
@@ -138,7 +139,12 @@
-    tab.item = $('<li class="vertical-tabs__menu-item" tabindex="-1"></li>').append(tab.link = $('<a href="#"></a>').append(tab.title = $('<strong class="vertical-tabs__menu-item-title"></strong>').text(settings.title)).append(tab.summary = $('<span class="vertical-tabs__menu-item-summary"></span>')));
+    tab.item = $('<li class="vertical-tabs__menu-item" tabindex="-1"></li>').append(tab.link = $('<a href="#"></a>').append(tab.title = $('<strong class="vertical-tabs__menu-item-title"></strong>').html(settings.title)).append(tab.summary = $('<span class="vertical-tabs__menu-item-summary"></span>')));

These changes need XSS protection, since any text used in the title is now interpreted as html.

Preferably the title should stay as text and only the error markup is appended as html, so existing implementations remain unchanged.

Thanks @dmsmidt! I'll try to find time for the cross-browser testing soon.

#131:

Although I don't fully agree about the XSS.
The @title is sanitised in Drupal\Core\Render\Element\Details::preRenderDetails()

But does that also count for the change in vertical-tabs.es6.js - the .html(settings.title)?

#134:

Fixed missing vertical tab border red as indicated by @xjm

Awesome, that was the only thing xjm blocked it on in #113.