Child elements in grouping elements like (meta) details and vertical tabs are invisible to the user when the grouping element is closed or not-focused respectively.
To improve the user experience and accessibility we should indicate when there are errors 'inside'.
Currently it is hard for the user to find the actual problematic field when the (child) field itself is invisible.

Note that only the Seven theme knows the additional concept of using Details elements as meta data elements at the right side of the node edit page.

Before

Meta details open (Seven only, LTR)
details before

After (proposal)

Meta details open (Seven only, LTR)

Meta Details closed (Seven only, LTR)

Meta details open (Seven only, RTL)

Meta Details closed (Seven only, RTL)

Normal Details closed (Seven only, LTR)

Normal Details closed (Bartik, LTR)

Vertical Tabs not focused (Seven, LTR)

Vertical Tabs not focused (Seven, RTL)

Vertical Tabs focused (Seven, LTR)

Vertical Tabs non focused (Bartik, LTR)

Tasks:
- Re-add red right border in LTR vertical tab with error.
- Replace new screenshot.

CommentFileSizeAuthor
#120 2848507-119-grouping-element-child-errors.patch17.84 KBandrewmacpherson
#111 2848507-111-85x.patch17.85 KBdmsmidt
#111 interdiff-2848507-105-111-85x.txt749 bytesdmsmidt
#111 2848507-111-84x.patch17.84 KBdmsmidt
#111 interdiff-2848507-105-111-84x.txt749 bytesdmsmidt
#9 2848507-9-grouping_element_children_errors.patch1.35 KBdmsmidt
#13 00000003.png43.83 KBttamniwdoog
#13 00000004.png153.89 KBttamniwdoog
#13 00000006.png449.13 KBttamniwdoog
#17 drupal-n2848507-17-without.png21.44 KBDamienMcKenna
#17 drupal-n2848507-17-with.png21.86 KBDamienMcKenna
#17 drupal-n2848507-17.patch2.15 KBDamienMcKenna
#17 drupal-n2848507-17.interdiff.txt1.44 KBDamienMcKenna
#19 drupal-n2848507-17-collapsed.png7.95 KBDamienMcKenna
#24 error_message_drupal_styling.png1.97 KBdmsmidt
#25 details-error-mark.png45.98 KByoroy
#32 vert_tabs_child_errors_non_focus_rtl_seven.png47.91 KBdmsmidt
#32 vert_tabs_child_errors_non_focus_ltr_seven.png47.06 KBdmsmidt
#32 vert_tabs_child_errors_focus_ltr_seven.png51.02 KBdmsmidt
#32 meta_details_child_errors_closed_rtl_seven.png11.52 KBdmsmidt
#32 meta_details_child_errors_open_rtl_seven.png29.4 KBdmsmidt
#32 meta_details_child_errors_closed_ltr_seven.png7.59 KBdmsmidt
#32 details_child_errors_closed_ltr_seven.png2.86 KBdmsmidt
#32 vert_tabs_child_errors_non_focus_ltr_bartik.png12.18 KBdmsmidt
#32 details_child_errors_closed_ltr_bartik.png2.83 KBdmsmidt
#32 2848507-32-grouping_element_child_errors.patch5.75 KBdmsmidt
#32 interdiff-2848507-17-32.txt4.55 KBdmsmidt
#34 interdiff-2848507-32-34.txt1.18 KBdmsmidt
#34 2848507-34-grouping_element_child_errors.patch7.17 KBdmsmidt
#38 2848507-38-grouping_element_child_errors.patch7.35 KBpk188
#43 2848507-43-grouping_element_child_errors.patch8.17 KBpk188
#47 jira-error.png11.84 KByoroy
#48 2848507-48-grouping_element_child_errors.patch8.76 KBBarisW
#48 interdiff-43-48.txt1004 bytesBarisW
#50 details_bartik_child_errors_before.png21.45 KBdmsmidt
#50 details_bartik_child_errors.png20.82 KBdmsmidt
#54 2848507-54-grouping_element_child_errors.patch11 KBdmsmidt
#54 interdiff-2848507-48-54.txt13.21 KBdmsmidt
#56 2848507-56-grouping_element_child_errors.patch11.04 KBandrewmacpherson
#61 interdiff-2848507-56-61.txt3.73 KBdmsmidt
#61 2848507-61.patch12.3 KBdmsmidt
#62 interdiff-2848507-61-62.txt8.84 KBdmsmidt
#62 2848507-62-grouping_element_child_errors.patch17.03 KBdmsmidt
#64 interdiff-2848507-62-64.txt1.07 KBdmsmidt
#64 2848507-64-grouping_element_child_errors.patch17.03 KBdmsmidt
#66 2848507-64-grouping_element_child_errors.PATCH_ONLY.patch5.51 KBdmsmidt
#73 2848507-73-grouping_element_child_errors.patch17.11 KBandrewmacpherson
#73 interdiff-2848507-64-73.txt1.41 KBandrewmacpherson
#74 2848507-74.png130.61 KBandrewmacpherson
#79 interdiff-2848507-73-79.txt2 KBdmsmidt
#79 2848507-79-grouping_element_child_errors.patch16.92 KBdmsmidt
#81 interdiff-2848507-79-81.txt2.35 KBdmsmidt
#81 2848507-81-grouping_element_child_errors.patch16.92 KBdmsmidt
#87 interdiff-2848507-81-87.txt2.51 KBandrewmacpherson
#87 2848507-87-grouping_element_child_errors.patch17.76 KBandrewmacpherson
#95 interdiff-2848507-87-95.txt1.93 KBjefuri
#95 2848507-95-grouping_element_child_errors.patch17.76 KBjefuri
#101 interdiff-2848507-87-101.txt1.93 KBandrewmacpherson
#101 2848507-101-grouping_element_child_errors.patch17.76 KBandrewmacpherson
#102 2848507-102-84x.patch17.76 KBmpdonadio
#102 2848507-102-85x.patch17.76 KBmpdonadio
#105 2848507-105-84x.patch17.78 KBdmsmidt
#105 interdiff-2848507-102-105-84x.txt570 bytesdmsmidt
#105 2848507-105-85x.patch17.78 KBdmsmidt
#105 interdiff-2848507-102-105-85x.txt570 bytesdmsmidt
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dmsmidt created an issue. See original summary.

dmsmidt’s picture

We just talked about this in the UX hangout discussion (http://youtu.be/X2LJEiW2BS0).
At least we can color the borders red and make them a bit thicker.

Since #2754977: Enhance formErrorHandler to include children errors on RenderElements is in we now have enough information on the grouping elements to know if there are errors on child elements.

andrewmacpherson’s picture

Issue tags: -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".

andrewmacpherson’s picture

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’s picture

@adrewmacpherson, do we really need to add text?
What about adding the IFE error icon before the labels and adding a thick red border?

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

Working on the PHP part.

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Active » Needs review
Issue tags: +Needs tests

Here is a first stab at things. This will add the 'error' class to tabs with errors.
And the 'error' class plus 'aria-invalid' to details and fieldsets.
Stylers go ahead!

We will need a functional JS test for this.

dmsmidt’s picture

Also, added vertical tabs to the module for easy testing:
https://github.com/dmsmidt/errorstyle

dmsmidt’s picture

Status: Needs review » Needs work

The last submitted patch, 9: 2848507-9-grouping_element_children_errors.patch, failed testing.

andrewmacpherson’s picture

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

andrewmacpherson’s picture

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.
ttamniwdoog’s picture

FileSize
43.83 KB
153.89 KB
449.13 KB

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

markie’s picture

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)
andrewmacpherson’s picture

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.

dmsmidt’s picture

I think it is pretty clear where we stand.

As per #12-2 we now need to "visually" show that there is a child error, some solution are proposed.

@andres (#12-3), I did some research about aria-invalid before I made the patch, but indeed those specs are very vague. I read them as: 'it is ok to add them to elements other than things like , but we may not do anything with it". I'm ok with removing them.

DamienMcKenna’s picture

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:

Without any changes.

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

With the patch applied.

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?

dmsmidt’s picture

First actual visual changes, great!

Indeed, the field error styling should have it's own issue.

Could you also provide a screenshot of how this looks on closed details? The most pressing concern is that it is not clear where errors are if a details is closed or a vertical tab not focussed.

How would the proposal translate to vertical tabs, could you guys come up with something similar?

One thing I thought when looking at the icon in the top right: 'this looks like a red close button'. Although it is perfectly aligned, the location may not be the best. Any thought?

DamienMcKenna’s picture

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

With the patch, when the fieldset is collapsed.

DamienMcKenna’s picture

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

dmsmidt’s picture

Great start. Thanks for the additional screenshot.

In closed form the red icon works better. Maybe we can remove it when the element is open. In combination with IFE the icon would already be present beneath the field.
Maybe a screenshot with IFE enabled and the element open would also help.

One, not belittling ment note: in D8 fieldsets can't be collapsed anymore, they are sort of replaced by details elements. Let's keep the terminology clear to avoid confusion.

Status: Needs review » Needs work

The last submitted patch, 17: drupal-n2848507-17.patch, failed testing.

dmsmidt’s picture

Issue summary: View changes
dmsmidt’s picture

FileSize
1.97 KB

Design feedback / idea from the UX meeting of 15 march (see demo at: https://www.youtube.com/watch?v=lWKo_OK6gHg):

"Reuse the error message styling (thick left border)."
"In open state and IFE enabled the element becomes crowded."

yoroy’s picture

Issue tags: -ux, -user experience +Usability
FileSize
45.98 KB

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?

andrewmacpherson’s picture

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.

andrewmacpherson’s picture

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.

andrewmacpherson’s picture

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

dmsmidt’s picture

Issue tags: +DevDaysSeville
yoroy’s picture

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

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

Working on this @Seville.

dmsmidt’s picture

  • Added error styling for vertical tabs.
  • Added the thick border to the left (with a bit of a rounded corner), corresponding to the error message styling.
  • Styling done per component (separate css files).
  • Position the icon correctly on multi-line titles/summaries.
  • Added the error styling also to Stable to have it show in Bartik. Since this is a bugfix and the styling is on a new class I think we can add this styling to Stable.
  • Also made the details element (with child error) background for Bartik white to have enough contrast.
  • The error styling is disabled when a fieldset is focused or a details element is open, this prevents that the UI becomes too crowded. When the children are shown, we don't need to mark the grouping element anymore.
  • Added RTL styling.

Some after screenshots added to the descriptions according to these changes.

@todo
- Novice: add before screenshots of all after scenarios
- Fix test fail
- Provide error wording for a11y
- Tests (check for the error class and error wording)

Status: Needs review » Needs work

The last submitted patch, 32: 2848507-32-grouping_element_child_errors.patch, failed testing.

dmsmidt’s picture

This adds some missing CSS and fixes the test failure.
We now cover the addition of the error class for details elements in our tests.
Concerning the tests, we now only need a Functional JS test for the error class on Vertical Tabs.

lauriii’s picture

Status: Needs review » Needs work

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

andrewmacpherson’s picture

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]) {
wolffereast’s picture

pk188’s picture

Status: Needs work » Needs review
FileSize
7.35 KB

Re-rolled and fixed #35

Status: Needs review » Needs work

The last submitted patch, 38: 2848507-38-grouping_element_child_errors.patch, failed testing.

andrewmacpherson’s picture

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.

andrewmacpherson’s picture

Issue tags: +Needs reroll
andrewmacpherson’s picture

Issue tags: +sprint
pk188’s picture

pk188’s picture

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

Status: Needs review » Needs work

The last submitted patch, 43: 2848507-43-grouping_element_child_errors.patch, failed testing. View results

BarisW’s picture

Assigned: Unassigned » BarisW

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

yoroy’s picture

FileSize
11.84 KB

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

BarisW’s picture

Assigned: BarisW » Unassigned
Status: Needs work » Needs review
FileSize
8.76 KB
1004 bytes

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.

andrewmacpherson’s picture

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

dmsmidt’s picture

Status: Needs review » Needs work
FileSize
21.45 KB
20.82 KB

@andrewmacpherson and @yoroy, thanks for the nice styling idea. I think it could be implemented in a new theme. The styling introduced by this patch are in line with current styling patterns of errors and proposed during an UX meeting (see #24). Redesigning (error) messages is out of scope for this issue.

@lauriii these styling addition (no changes) are actually major accessibility bug fixes. And as mentioned in #36 the class is used for styling. Could you reconsider?

Bartik details element styling is off, see:
Before

After

yoroy’s picture

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.

lauriii’s picture

  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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
11 KB
13.21 KB

I addressed all @laurii's concerns in this patch, only styling for Bartik and Seven are changed. I also fixed #50 (Bartik details styling regression).

I'll create a follow up issue to make this even more accessible by announcing the amount of child errors if a closed detail/tab gets focus.
This is however a bit out of scope, but the groundwork is now also present (data-child-errors-count).

Should we mark this Major priority btw? Since it is a blocker for Inline Form Errors and a serious non-contained accessibility bug.

Edit: created follow up and working on a fix #2892443: Announce that grouping elements have child element errors for accessibility.

Status: Needs review » Needs work

The last submitted patch, 54: 2848507-54-grouping_element_child_errors.patch, failed testing. View results

andrewmacpherson’s picture

Status: Needs work » Needs review
FileSize
11.04 KB

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.

andrewmacpherson’s picture

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’s picture

Status: Needs review » Needs work

Some valid points by @andrew, so putting this back to needs work.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review

Here is an implementation of what @andrew suggested.
Could you test it?

If it works we should add a functional JS test for the vertical tabs en elaborate on the existing browsertest a bit.

dmsmidt’s picture

dmsmidt’s picture

Woop, first Functional JS test for a core Render element (hooray for a11y tests) :-).
We now have full test coverage for details and vertical tabs concerning the changes of this issue.
I also did some further cleaned of the patch.

For the JS reviewers, note that we now need to use html() instead of text() to support the hidden error indicator span.

Status: Needs review » Needs work

The last submitted patch, 62: 2848507-62-grouping_element_child_errors.patch, failed testing. View results

dmsmidt’s picture

Test fix.

drpal’s picture

@dmsmidt

Can you upload a test only patch for #64?

dmsmidt’s picture

Sure, here it is.

Also bumping the priority, cause it is a IFE blocker and also without IFE an accessibility problem.

Status: Needs review » Needs work
dmsmidt’s picture

Status: Needs work » Needs review

Fails as expected.

drpal’s picture

  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.

dmsmidt’s picture

@drpal

#69.1: yes it is, see #62, last sentence.
#69.2: alright, I'll keep that in mind. I guess there will be a major cleanup when the time comes for Drupal.

yoroy’s picture

andrewmacpherson’s picture

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.
andrewmacpherson’s picture

This patch addresses #72.1 - a minor string change.

andrewmacpherson’s picture

FileSize
130.61 KB

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.

node form with error in authoring information datetime element

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

SKAUGHT’s picture

Status: Needs review » Reviewed & tested by the community

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.

mpdonadio’s picture

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

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

andrewmacpherson’s picture

Status: Needs review » Needs work

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

dmsmidt’s picture

Alright, removed the plural version. This can always be re-added later when the follow up is fixed.
Fingers crossed, I wasn't able to test the rewrite because my system is just doing a dist upgrade.
Testbot will tell..

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

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.

andrewmacpherson’s picture

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 .

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

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?

andrewmacpherson’s picture

Assigned: Unassigned » andrewmacpherson

doing #85 now

andrewmacpherson’s picture

Assigned: andrewmacpherson » Unassigned
Status: Needs work » Needs review
FileSize
2.51 KB
17.76 KB

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.

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

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

dmsmidt’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

Manual tested, still works. All points in #85 are fixed. Back to RTBC, thanks!

lauriii’s picture

Issue tags: +Needs change record

I forgot to add the tag on #85

dmsmidt’s picture

Issue tags: -Needs change record
yoroy’s picture

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)

gambry’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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.

jefuri’s picture

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

jefuri’s picture

Status: Needs work » Needs review
jefuri’s picture

Version: 8.4.x-dev » 8.5.x-dev
andrewmacpherson’s picture

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.

andrewmacpherson’s picture

Version: 8.5.x-dev » 8.4.x-dev
andrewmacpherson’s picture

Status: Needs review » Needs work

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.

andrewmacpherson’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
17.76 KB

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)

mpdonadio’s picture

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
Issue tags: +Vienna2017
dmsmidt’s picture

Status: Needs review » Needs work

Bummer, found one nit. Gonna fix it in a bit.

+++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
@@ -467,6 +467,14 @@ form_test.group_vertical_tabs:
+    _title: 'Child element error vertical tabs testing'

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Render/Element/VerticalTabsTest.php
@@ -0,0 +1,32 @@
+ * Tests that sessions don't expire.

Wrong test description.

dmsmidt’s picture

The last submitted patch, 105: 2848507-105-85x.patch, failed testing. View results

BarisW’s picture

Tested again, and it all works great.

Fieldset collapsed

Only local images are allowed.

Fieldset expanded

Only local images are allowed.

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

The last submitted patch, 105: 2848507-105-85x.patch, failed testing. View results

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

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!

xjm’s picture

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

dmsmidt’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

@xjm, good catch. Just checked, and yes they are recent.
Sadly marking this as 'Needs work' again.

xjm’s picture

Thanks @dmsmidt!

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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

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

andrewmacpherson’s picture

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

andrewmacpherson’s picture

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

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.

andrewmacpherson’s picture

Forgot to upload the patch.

andrewmacpherson’s picture

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.