Problem/Motivation

For short form fields it makes sense to have the #description appear below the form widget; however, in long lists of checkboxes and/or radio buttons it would be nice to be able to have a toggle whether the #description appears above, or below, the form widget.

Although this can be themed with theme_form_element, it is difficult.
Some site builders ask clients to include their instructions for long form widgets in the "label" (#title) instead of the #description so that the instructions appear at the "right" end of the form.

Steps to reproduce

  1. make a module that has a form
  2. notice the description is after
  3. add '#description_display' => 'before' to the module
  4. notice your description in the form is now before

Proposed resolution

Add a form api option "description_display" which has three allowed values: before, after, invisible.

Keep the current behavior the default of the description being after
by adding
+ '#description_display' => 'after',
to core/lib/Drupal/Core/Form/FormBuilder.php
which all forms use.

"invisible" instead of "visually-hidden" is for consistency with the title_display property: https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

This issue does NOT add a UI to core to expose the configurability per field.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions done
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

No changes. (but adds a way for things to change.)

API changes

Change to form api.

Files: 
CommentFileSizeAuthor
#157 314385-nope-2.png155.13 KBLes Lim
#157 314385-nope.png120.33 KBLes Lim
#152 314385-description-configurable-152.patch7.82 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,410 pass(es).
[ View ]
#152 314385-description-configurable-152-tests-only.patch5.02 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,237 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#152 interdiff-314853-149-152.txt705 bytesakalata
#149 interdiff-314385-142-149.txt4.39 KBalimac
#149 314385-description-configurable-149.patch7.82 KBalimac
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,535 pass(es).
[ View ]
#148 form-description-display-set.png28.65 KBalimac
#148 form-description-display-not-set.png29.19 KBalimac
#146 description-field-position.png32.84 KBalimac
#142 314385-description-configurable.intediff.140-142.txt1.89 KBpenyaskito
#142 314385-description-configurable-142.patch7.49 KBpenyaskito
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,491 pass(es).
[ View ]
#140 314385-description-configurable-140.patch5.6 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,490 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#136 314385-description-configurable-136.patch4.65 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,194 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#135 314385-description-configurable-134.patch4.13 KBpenyaskito
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,195 pass(es), 28 fail(s), and 15 exception(s).
[ View ]
#132 description_display-314385-132.patch4.59 KBjday
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,734 pass(es), 28 fail(s), and 15 exception(s).
[ View ]
#126 description_display-314385-126.patch4.64 KBRisse
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,671 pass(es), 30 fail(s), and 15 exception(s).
[ View ]
#123 interdiff.txt4.8 KBjjcarrion
#123 description-display-314385-122.patch4.76 KBjjcarrion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,747 pass(es), 30 fail(s), and 15 exception(s).
[ View ]
#120 description-display-314385-120.patch1.52 KBjjcarrion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,268 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#111 description_display-314385-111.patch17.33 KBjlbellido
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch description_display-314385-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#108 description_display-314385-108.patch17.3 KBjlbellido
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description_display-314385-108.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#108 interdiff.txt1.44 KBjlbellido
#99 description_display-314385-99.patch15.86 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,754 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#96 description_display-314385-96.patch15.64 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 59,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#89 description_display-314385-89.patch14.87 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#89 form_test.module.txt706 bytesmgifford
#76 description_display-314385-76.patch16.11 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description_display-314385-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#76 interdiff_74_76.txt7.64 KBPancho
#76 output.html_.txt2.29 KBPancho
#74 description_display-314385-74.patch10.93 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 57,219 pass(es).
[ View ]
#74 interdiff_55-74.patch4.26 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_55-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 Screen Shot 2013-07-08 at 5.04.16 AM.png14.05 KBmgifford
#57 interdiff-43-55.txt11.46 KBmgifford
#55 description_display-314385-55.patch10.56 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 56,822 pass(es).
[ View ]
#53 description_display-314385-53.patch10.64 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#43 description-position-314385-43.patch4.24 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 53,598 pass(es).
[ View ]
#29 description-position-314385-28.patch6.3 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description-position-314385-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 description-position-314385-10.patch6.29 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]
#10 314385_description_position-10.patch835 bytesstBorchert
Failed to run tests on MySQL 5.0 ISAM: failed during invocation of run-tests.sh.
[ View ]
#10 description_before.png15.41 KBstBorchert
#10 description_after.png15.32 KBstBorchert

Comments

BrightLoudNoise’s picture

Component:usability» forms system
Issue tags:+Needs usability review

Tagging so it's identified to the usability team

Bojhan’s picture

Not sure whether this should be an option in the GUI, this will only occur if the description needs to be read in order to successfully fill out the form. emmajane, could supply more context when you ask your clients to do something like this?

yoroy’s picture

Not convinced either. I'm not really seeing a reason *why* you want to do this.

You are putting more text in front of the user before you let them at the actual task of filling in the field. Drupal already has a (bad) habit of showing help text *before* functionality (see the block admin page for an extreme example), the way I see it you are proposing to replicate that same behaviour on a per-field basis. I don't think that will make for more productive forms.

emmajane’s picture

The help text includes instructions on how to fill out the form. For very long form options (e.g. multiple images or very long lists of checkboxes) it is important to explain what is being filled out before people start filling out the options. A label should be a label. It should not contain instructions on how to complete the form.

For example: some of my clients are "unsophisticated" and do not understand the difference between the visual of a checkbox and a set of radio buttons. In this case the information on how to fill out the form must appear BEFORE they start filling out the form especially when the list of options is longer than the page is tall (making the instructions invisible as they are at the bottom of a long list of options). The same is also true for multiple images. People need to see what images they are meant to select before they start choosing images.

adshill’s picture

I completely agree with emmajane. IF you have a textarea field that is 20 rows high, and you want to supply help text, it's not until you get to the end you actually see what you were "supposed" to put in the field. In addition if you have 10 or 20 check boxes you also have the same problem. I am creating a funding application form for a client in D6 and its imperitive on a number of the items that the user gets the instructions before setting out to answer the question.

These forms will be filled out by the general public and they need the information in a logical way, which to me would be Label, Help text, input.

Just as a side note and would hope for a PM - not to be answered on this thread but emmajane if you had a method to do this in D6 for your clients I'm in dire need of finding one and would really appreciate if you could help me out!!

My 2 pennies :) Adam

rob5408’s picture

Just piling on here, but I would also like to see this. Also, the 'description' internal name is a bit misleading as it's labeled as 'help' text when creating/editing new fields. Maybe a simple solution to this problem is to have a 'Label' field, a 'Description' field below the label but about the widget and a 'Help' field below?

sun’s picture

Title:#description above form widget» Make position of #description configurable
webchick’s picture

Hm. I'm not sure. I don't like the idea of FAPI embedding presentation logic.

Could you not work around this with #prefix?

yoroy’s picture

I would prefer to see this solved in contrib, not in core. I can see the use case but overall still looks like an edge case. The proposal is to make this a per-field option? That would mean mean an extra checkbox for each field in your content type. Overkill for core imo.

stBorchert’s picture

Status:Active» Needs review
StatusFileSize
new15.32 KB
new15.41 KB
new835 bytes
Failed to run tests on MySQL 5.0 ISAM: failed during invocation of run-tests.sh.
[ View ]

As you can see in this screenshot the description for the set of radios is placed below them instead of between title and first radio button.
What if we make this "hardcoded", but only for radios and checkboxes?

Bojhan’s picture

Let's not :). Although I understand the need for this, Drupal descriptions are not ready yet.

Bojhan’s picture

I am fine if this is hidden in code, or in contrib.

stBorchert’s picture

btw: the code used in my previous patch can be done also in your theme by overriding theme_form_element() (as I do on every of my sites).
Its just a simplification and changing of default behavior.

emmajane’s picture

#prefix won't work (that comes before everything and cannot be edited from the field config screen). If #description were broken into "help above" and "help below" that would work. I'm specifically looking to have optional help text between the Field Label and the input field(s). The solution doesn't need to have a toggle, but I do need to be able to configure the location of the help text per field from the GUI.

Dane Powell’s picture

Status:Closed (won't fix)» Needs review

+1... on my site users specifically told me that they were very confused by the descriptions being below form elements, especially text areas, so I themed it such that descriptions always appear above the field (just below the label). This looks very good IMHO as long as a label is present. However, for fields that don't have labels this becomes confusing and looks awkward. A good solution for me would be to place descriptions between the label and the field if a label exists, and below the field if there is no label.

Edit: I achieved this by adding a conditional to my theme's template.php that outputs the description prior to the field if a title is present and after the field otherwise

Bojhan’s picture

Status:Needs review» Closed (won't fix)

Sorry, but as explained above - there are clear reasons why we don't do this in core yet. I would be fine, if our descriptions were short -to the point, and get users to understand what we mean - but a lot of descriptions simply don't. And we won't fix this before release - by far. Lets keep this as an overwriteable theme function, which enables the clients that do request this to get this.

mgifford’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Active

http://www.smashingmagazine.com/2009/09/24/10-useful-usability-findings-...

"A study by UX Matters found that the ideal position for labels in forms is above the fields. On many forms, labels are put to the left of the fields, creating a two-column layout; while this looks good, it’s not the easiest layout to use. Why is that? Because forms are generally vertically oriented; i.e. users fill the form from top to bottom. Users scan the form downwards as they go along. And following the label to the field below is easier than finding the field to the right of the label."

Dane Powell’s picture

Just FYI, this is the solution I used for my site and theme, and it's received unanimously positive reviews: #557162: Position field descriptions more intelligently

I completely understand hesitancies about implementing this in core, but it does seem to me that this is the most logical default and thus I don't think every single module should have to re-implement it like I did

RobW’s picture

Thanks for the multiple patches / snippets to fix this. While I agree an option per each field in core is overkill, descriptions above the input and below the label are essential for most of my client work. Consider a wysiwyg textarea that by default fills the screen (i.e. CKeditor), or an unlimited field, or a field collection (multigroup). In each instance the user will not see the help until they scroll, in last case sometimes for a screen or two. This is compounded when a user is editing content that has already been created in unlimited fields or field collections case.

In my opinion help text that gives a user information essential to completing a field must be visible to the user before they begin to fill out that field. It's a clear usability pass/fail.

Some core descriptions are long, multiple lines including allowed html tags and the like. I can see them being distracting above the field. Possible solutions seem to be:

  • Invite some copy specialists rethink and edit down long core descriptions, then place all descriptions on top.
  • Creating a new, secondary description field and defining the purpose of one as essential help text, one as additional, non essential information (analogous to link or img title text in html).
  • Global option for all field descriptions on the Drupal installation.
  • A more complex module that offers choices by content type or UI area. This might be a good project for D7 to gauge interest in the alteration for D8.

If I had to choose one or the other for my whole installation, I would choose top no questions. Lots of text above the field can be a usability speed bump, but not having access to instructions or an essential piece of help text is a usability failure.

realityloop’s picture

Moved and edited from http://drupal.org/node/1122894

From a usability standpoint I believe that the description text should always be shown between the Field Label and the Field itself.

As it currently stands the text is semantically after the Field which I assume means those using screen readers don't hear the text until after they have encountered the field negating it's use.

This is also relevant for sighted users, as most people don't read ahead when filling out forms.

Either way I believe if it is an option it shouldn't change the semantic placement of the description, but only the visible positioning ie. via CSS.

Reference material:
http://www.slideshare.net/cxpartners/web-and-mobile-forms-design-userfri...
From slide 61

http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H65.html
Suggests use of title element on input's

http://www.smashingmagazine.com/2011/04/06/fundamental-guidelines-of-e-c...
Section 2

yoroy’s picture

Thanks. What would argue against this though, is that too many of our descriptions are still way too long. Increasing distance between label and form field by more than one line would look silly and I assume be counter-productive too.

#17, @mgifford, that quote refers to labels, not descriptions. Drupal form labels are already on top by default.

realityloop’s picture

Yoroy, I don't think we really have an option it we want Drupal to pass wcag recommendations.

And many organizations websites are required by law to meet the wcag, but most of all I'm concerned about the affect on visually impaired users.

mgifford’s picture

Titles & Labels are both valid ways to identify form controls. In Drupal 7 & FAPI we weren't able to get both elements into the API. In Drupal 8 I am hoping that FAPI gets rewritten so that the concerns expressed about including a title element. The invisible label was a work-around.

I'm updating the link here to the final (rather than 2008 draft) of WCAG 2.0 - http://www.w3.org/TR/WCAG-TECHS/H65.html

The usability team has done some great work on encouraging the community to think more closely about the language that we use. So much of the time it isn't clear enough. Having a one line description would usually be sufficient.

I don't think it's a huge issue, but do think it would be great if there were a default setting where one could switch the location of the description to either above or below the field. Heck, maybe even stuff it into a tooltip type of functionality like http://hanshillen.github.com/aegisdemo/

droplet’s picture

Yes. I have same issue. sub first

Everett Zufelt’s picture

Issue tags:+accessibility

IMO, not a WCAG 2.0 requirement, should anyone disagree please provide SC that we are currently not passing.

I agree that it could be useful for fapi, if not field api to provide this option.

In fapi we could add a new property:

$element['#description'] = t('My description');
$element['#description_position'] = 'before' | 'after' // default to 'after' for all elements

This is a theme setting, and might not be appropriate as a fapi property, however, the options are:

1. Do nothing

* Keep things as we have them now and individuals can solve this themselves with theme overrides (sometimes very complicated if you only want this to apply to certain elements).

2. Use #description_before #description_after

* This would require refactoring of every Core / Contrib module that uses #description. It would also not be semantic and would cause some accessibility problems. There is a 1 - 1 relationship between element and description semantically (elements do not have two descriptions), and in accessibility APIs there is a 1 - 1 UI control - description relationship.

3. Allow for a fapi property to indicate if the description should be rendered before or after the element, and modify theme_form_element() to use this. Field API could then possibly leverage this (in core or contrib) as an additional setting when creating a field.

penyaskito’s picture

I attached a patch to #1325488: Add an option to show help text before field and marked as needs review before finding this one. I'm not sure if I should mark it as duplicate, because there have been some comments there too. How should I proceed?

The approach taken is the same one that is described at comment #25 -> option 3, but called #description_display for consistency with #title_display, but #description_position seems to be a better name. Should I change it?

yoroy’s picture

penyaskito: attach your patch here to continue the discussion. I marked the other issue as a duplicate of this. Thanks for checking in here :)

penyaskito’s picture

Status:Active» Needs review
StatusFileSize
new6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 34,020 pass(es).
[ View ]

Patch attached.

penyaskito’s picture

StatusFileSize
new6.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description-position-314385-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

New patch using #description_position instead of #description_display.

Les Lim’s picture

Status:Needs review» Needs work

Testing the patch in #29, and it doesn't work yet for text fields with the text format dropdown selector. The description text for those fields is being output in theme_text_format_wrapper(), not theme_form_element(). Here's the api.drupal.org link:

http://api.drupal.org/api/drupal/core--modules--filter--filter.module/fu...

penyaskito’s picture

Assigned:Unassigned» penyaskito

Thanks for catching it and pointing to me to the right direction, @Lis Lem.

However, if I edit that function the description will be print before the Body label. I'm looking at field_multiple_value_form to see if I find where to put it just after the Body label.

kaare’s picture

Never noticed this issue, but I have a sandbox module for d6 solving this. Ancient code, I know, but I believe FAPI didn't change that much between D6 –> D7, so if you need something immediately, porting this wouldn't be all that difficult.

penyaskito’s picture

Hi kaare. Didn't know about your sandbox. I had a look and the feature is the same than in this patch, but the unsolved problem here is with text with summaries and with formatted text areas, that have changed a lot once fields are included in core since D7.

Thanks anyway for sharing!

spartan300’s picture

Hi emmajane it really helped many thanks

mgifford’s picture

Status:Needs work» Needs review

#29: description-position-314385-28.patch queued for re-testing.

mgifford’s picture

The accessibility issue can be addressed by using WAI-ARIA described-by as per:
http://www.w3.org/TR/wai-aria/states_and_properties

mgifford’s picture

Issue tags:-Usability, -accessibility

#29: description-position-314385-28.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +accessibility

The last submitted patch, description-position-314385-28.patch, failed testing.

jbrauer’s picture

A long time in coming but to answer webchick's question in #8 this can't readily be handled by #prefix because at least in certain cases (i.e. select fields) the #prefix is rendered before the #title/label. While I can see the desire to not embed presentation logic in FAPI it seems it's already doing so.

er... or it's been too long since I had real sleep...

mgifford’s picture

Issue tags:+Needs tests

tagging

mgifford’s picture

penyaskito’s picture

Assigned:penyaskito» Unassigned
mgifford’s picture

StatusFileSize
new4.24 KB
PASSED: [[SimpleTest]]: [MySQL] 53,598 pass(es).
[ View ]

The tests have moved.. So this doesn't include the tests. But it's a re-roll that hopefully applies better.

Albert Volkman’s picture

Status:Needs work» Needs review

Setting status.

Bojhan’s picture

I still think this is a bad idea. This is not part of the 80%, and adding it to core should be about that.

mgifford’s picture

Status:Needs review» Closed (won't fix)

Ok, I'm going to mark this as postponed until someone comes back with a solid usability argument or study which says that the position of the description (not the label) should be above (not below) the input field.

When we've got a study to point to then we can re-open this.

Dave Reid’s picture

I believe the biggest reason for this change is multiple-value fields that take up a lot of space. When you go to enter those fields, the help description text is at the bottom, possibly after you had to scroll past the field label and all the actual fields. For small vertically-limited forms, having the description at the bottom makes sense. But not for fields that use large vertical space, commonly multiple-value fields.

mgifford’s picture

Status:Closed (won't fix)» Active

That's a good reason to make it configurable... Hard to argue with that image.

webchick’s picture

Well, that's a good argument for core to make it possible to move a description to the top, but not a good argument for this being an 80% usage and something that should be part of the core framework.

Is there a reason a custom theme function override couldn't do what you need to do there, Dave?

yoroy’s picture

Yeah, in #12 (and #16) bojhan pointed out that 'make possible' is fine. But lets not provide a UI for it in core and the core default should stay below the form field. I agreed then and still think that's the part that core can provide (and nothing more) now.

Dave Reid’s picture

I think part of the problem is how Field API outputs it's fields since technically it's not possible to put a field description inbetween the label and actual input fields since the label is actually *inside* the multiple-value table.

@webchick: Because I shouldn't have to implement a custom theme function to fix a UX issue on every single site that I have. I can live with API-level support for this in the theme functions, but this not being exposed to the field UI at all. That's perfectly acceptable to me. Fixing the multiple field table may be a separate issue as a follow-up.

Pancho’s picture

Category:feature» task

I like this approach, not only for the presented use case but also for hiding the description while it remains available for screen readers.
In some cases this would be helpful - even in core - where we are having compound widgets that go with a single description for visual usage, but need per-widget descriptions rather than per-widget titles for screen readers.
See cardinality field in field_ui_field_edit_form() resp. a generic select-or-other widget as proposed in #1207060: Interface pattern for custom option as alternative to predefined choices. There might be more use cases.

Besides consistency with #title_display, this is one more reason we should call this attribute #description_display, not just #description_position.

For the accessibility aspects IMHO this is a task, though a normal priority one.

Pancho’s picture

Status:Active» Needs review
StatusFileSize
new10.64 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here's a new patch introducing a #description_display property with the possible values 'before', 'after' and 'invisible'.
Compared to #43, I refactored and simplified quite some code in theme_form_element() and moved the actual implementation to a new theme_form_element_description(). I also partly rewrote the docs which could use some more love.

Worked fine on some preliminary tests, might fail though on ElementsLabelsTest and possibly some more.
Of course we need tests for #description_display, but I wasn't sure whether I should better add some minor tests to ElementsLabelsTest or create extensive tests in a new ElementsDescriptionsTest.
In case of an invisible description, the <div> should possibly also be a <span> instead.

Status:Needs review» Needs work

The last submitted patch, description_display-314385-53.patch, failed testing.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,822 pass(es).
[ View ]

Ah, this is because #1838310: Remove st(), get_t() and $t for good landed the day before. Here's a new patch.
As I said in #53, some tests would need adjustment, so there might be some more fails.

Pancho’s picture

Cool, no fails.
Still needs tests, either in ElementsLabelsTest or a new ElementsDescriptionsTest.

mgifford’s picture

StatusFileSize
new11.46 KB

I'm mostly attaching an interdiff.

This is all stuff that you can control with code, but not through the UI, right? At the moment there's no way to say from admin/structure/types/manage/article/display set where the description sits.

Not a big deal if this is the case, this looks like a good jump ahead.

mgifford’s picture

I didn't even realize there was a module for this already - https://drupal.org/project/label_help

Seems like something that should be in Core though.

Pancho’s picture

I didn't even realize there was a module for this already - https://drupal.org/project/label_help

Neither did I, but that really is another good use case.
Yes, it should be in core, because it's not about some edge cases but improves usability of some core widgets.

mgifford’s picture

Absolutely! So, what is the best way to test this?

We need screenshots at least.

mgifford’s picture

#55: description_display-314385-55.patch queued for re-testing.

mgifford’s picture

StatusFileSize
new14.05 KB

With this patch applied, I'm still not sure how to configure this so that the help text is closer. Same problem described in #47.

Screen Shot 2013-07-08 at 5.04.16 AM.png

penyaskito’s picture

#55: description_display-314385-55.patch queued for re-testing.

Pancho’s picture

Re #62:

$element(
  ...
  '#description_display' => 'before',
  ...
)

Should do it. No?
Or do you rather mean that it's not yet exposed to the UI? That's true.

Pancho’s picture

Status:Needs review» Needs work

So now, we need to identify all possible situations where putting the description right under the title is desirable, or where we want the description not to be displayed at all. Clear cases might possibly be resolved automatically, if that's not a WTF. For the other cases we might need to expose a setting to the UI.

Bojhan’s picture

We do not need to expose this through the UI, this can easily be done in code as you shown.

Given that this seems close from an API standpoint, I'd say we get it into core.

mgifford’s picture

Title:Make position of #description configurable» Make position of #description configurable via the API

@Pancho Yup --> "it's not yet exposed to the UI?" -- That's exactly what I meant.

@Bojhan - Absolutely. Let's get the API in first then open another issue for the UI. If that needs to wait till D9 to get in so be it. Changed the title.

Pancho’s picture

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

OK. So patch #55 is up to be extensively reviewed for:

  • added API feature (Does everything make sense? Would there be a better way? etc.)
  • refactored code (Should be correct, but maybe suggestions?)
  • added theme function (Is it DX-friendly?)
  • rewritten documentation (improvements, clarifications, typofixes, etc.)

Also, it would be cool if someone identified a possible implementation in core to be added rightaway.

Note that the implementation in NodeFormController of course has to go away. Seems like leftover from my own testing.
Also still needs tests. Will fix that all in one go, so for now I'm marking patch #55 as "needs review".

penyaskito’s picture

Quick note:

added theme function (Is it DX-friendly?)

Because of #1757550: Convert core theme functions to Twig templates, we shouldn't include new theme_* functions. Use twig instead.

Sheldon Rampton’s picture

I'm the maintainer of the Label Help module described previously. Getting similar functionality into core would be a good thing. I just want to point out some things to consider:

  1. In Drupal 7, the placement of the description/help text is hard to change, and it seems to me that this difficulty is unnecessary. Developers ought to be able to simply create a tpl.php file to override the normal placement of description, but that isn't possible, which is why I had to write the Label Help module in the first place. I don't know if Twig is going to make this easier to address in Drupal 8, but it would be nice if it could. The Label Help approach simply gives site builders the option of placing help text immediately after field labels, but even that is more limiting than some developers may want. Maybe someone wants to put help text before a label, or somewhere else that we haven't considered. A themeable template would provide the most flexibility for unanticipated use cases.
  2. It has been a challenge to get Label Help to work consistently for all field types. For example, the code for handling this is different for container field types, and is different for simple textfields than for textareas.
  3. I originally set out to build a module that would let me change the placement of the description text so that people could choose whether they wanted it to appear above the form field or below it. It turned out to be easier to simply create a second help field entirely, and I think that approach may give site builders more flexibility.
Pancho’s picture

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

Back to needs work per #69.
Replacing theme_form_element_description() and probably also theme_form_element_label() by a Twig template.

Re #70:
So do I get it right, this patch is on par with what Label Help can do for the admin?
But true, the placement should be overrideable just as the themer wishes. If it isn't, I'd propose this to be a followup, because it should probably be discussed in a larger context.

Sheldon Rampton’s picture

@Pancho: This patch is different from what Label Help does. This patch appears designed to control the placement of the description field, by adding a parameter which specifies where the description should be placed. This patch also does not provide a user interface for controlling that parameter when a site builder is adding a field to an entity. It simply adds the parameter.

From reading over the patch, I don't know if it will actually work for all of Drupal's field types. When I wrote the Label Help module, there were a few code recipes floating around providing ways to move the description text, but they didn't work consistently for all field types, and I couldn't figure out how to make any of them work consistently. To make it work consistently, it seemed that you have to (1) insert the description text in a different place, and (2) prevent Drupal from inserting it in the place where it is accustomed to inserting it. I couldn't figure out how to do that because I think it involves logic inside drupal_render which varies based on a variety of factors. For example, with a textarea field the logic varies depending on whether the field is plain text or formatted text (in which case the logic also inserts an input format selector and input format help text).

After puzzling through that for awhile, I finally decided, "Forget it, I'm just going to create a separate help field entirely from the description field and call it 'Label help.' Then I don't have to mess with all that other stuff."

If the current patch works consistently with all field types, it's probably an improvement over my Label Help approach. Has anyone tested to see how it actually displays the help test across a full range of available field types?

penyaskito’s picture

For the tests that are still needed, maybe the patch at #1325488: Add an option to show help text before field can help.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new4.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_55-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,219 pass(es).
[ View ]

Converted the new theme_form_element_description() to a nice little Twig template, so we're not introducing additional stuff to be converted.
Also some more minor fixes in code and docs.
No UI yet, as this should be left to a followup, covering both title and description.

From all manual tests, this worked nice with different kinds of fields. Not surprising, because it parallels existing #title_display functionality.
Tests still needed, thanks @penyaskito, I will look into the other one. Let's start with the functional code anyway.

Status:Needs review» Needs work

The last submitted patch, interdiff_55-74.patch, failed testing.

Pancho’s picture

Status:Needs work» Needs review
StatusFileSize
new2.29 KB
new7.64 KB
new16.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description_display-314385-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Now, here is a version with some basic tests adapted from #1325488: Add an option to show help text before field, however testing also the @aria-describedby attribute.
I also tweaked the HTML output a bit, so new lines and indentations should be even cleaner than before.
In-line comments improved as well.

Tests passed locally, and additional manual testing suggests the attribute works fine with all kinds of fields. Now even more ready for review.

penyaskito’s picture

Status:Needs review» Needs work

Tests are nice and cover all the possibilities. From my point of view, only #69 is still pending.
Great job @Pancho, looks like we are really close to RTBC a 5-years-old issue.

mgifford’s picture

@penyaskito I'm definitely in favour of converting stuff to twig, but that's beyond the scope here.

There are no new theme functions added here. I'm just following the guide I found in GoogleDocs, but really don't see from reviewing the patch what's been added that needs to be done in twig.

Yes, there are lots of theme_* functions that need to be changed in form.inc, but that should really be done in another issue.

EDIT: Note docs seem to be in https://drupal.org/node/2025313

Pancho’s picture

Status:Needs work» Needs review

Both of you, thanks for reviewing!

Converting the already existing theme functions in form.inc is done in #1898480: [meta] form.inc - Convert theme_ functions to Twig.
So we're just making sure we're not introducing more stuff to convert, so the new one already is a Twig template.

Came up with three more points of discussion:

  • Now the title may be displayed before or after the form element, and if the title is hidden, there even might be only the description. All fine. However IMHO the description should never precede the title if both are displayed. Should we make sure this isn't possible, or should we just leave it to the discretion of the developer.
    If yes, then would it make more sense to refactor the 'description_display' parameter to take either of the values 'after_element' vs. 'following_title', instead of 'after' vs. 'before'? It was chosen to be in line with 'title_display', but does not necessarily have to.
  • Also, the Twig template's doc is incorrectly stating it wouldn't be called if the description is empty. Currently it is just not being called if there is no description at all, in line with theme_form_element_label().
    However, while there is use for calling theme_form_element_label() with an empty label, in order to display the required marker, there is no use for calling template_preprocess_form_element_description() with an empty description.
    Normally it shouldn't do any harm unless $element['#description'] is explicitely set with an empty string, but we could be a bit stricter for descriptions in order to avoid the unnecessary overhead in this case, sacrificing the similarity between the two theme functions.
  • Last question regarding the HTML output in #76: Should the description take its own line even though the label doesn't? Or should both? Or neither? And should the ID go first to allow for identifying it more easily?

So that seem to be the remaining fineprint nitpicks, apart from possible test improvements/expansions. :)

For 'title_display', I also believe we don't need to have both 'invisible' and 'attribute', so the latter could be integrated into the former. If a title is set, why would someone want it not to be displayed in a tooltip, where it's not distracting at all. But this one really is out of scope here. Would like to do a followup though.

Sheldon Rampton’s picture

@Pancho: You wrote: "However IMHO the description should never precede the title if both are displayed. Should we make sure this isn't possible, or should we just leave it to the discretion of the developer."

I'd say, leave it to the discretion of the developer. Drupal should be enabling developers by providing them with good tools and encouraging them to make good design choices, but it should not be forcing design choices on developers. There may be some use case you or I haven't thought of where the developer will have a good reason for doing something even though we don't currently think it is a good idea.

Pancho’s picture

Re #80:
Fine, I even agree, just wanted it to be a conscious decision.
How about the other two points in #79?

I also thought about some actual usage of the new attribute in core. Think that it would be nice to roll in a few uses, and found quite some of it:

  1. On the node edit form, the body textarea and most checkboxes have no description though they could use at least an invisible one. This seems to be a clear case.
  2. Also, more generally, buttons don't have descriptions.
    a) Beyond an invisible one for screenreaders, I'm wondering if a description shouldn't provide more information about what is happening in a tooltip. We might actually implement this or not (probably not within scope of this issue here) but we should probably allow for it, so would need either $element['#description_display'] = 'attribute' or the tooltip functionality rolled into 'invisible' mode.
    See also the very last paragraph in #79.
    Note that the HTML5 specs explicitely allow instructions to be included with the title attribute, see this.
    b) If we'd be adding this as an option, I'd propose to use 'tooltip' which might be better than 'attribute' for both '#title_display' and '#description_display', because 'attribute' doesn't really say what's going to happen. Renaming it might encourage developers to make more use of descriptions, so improving usability. This could be another followup though.
  3. Usage of $element['#description_display'] = 'before' is more complicated, so should be deferred to a followup.

While a lot more descriptions are missing, 1) should be enough to at least have some implementation rightaway.
But 2a) should be taken into consideration now, in order to have the API cover all possible uses.

Pancho’s picture

Pancho’s picture

Hunting for an actual application really turned out to be a good idea, because I hit some more real cases.
@Sheldon Rampton was right in that a number of elements types need special considerations. This was all too easy to be a complete solution. We'd also need more than these few basic tests.

Let's start with the two container-style types 'fieldset' and 'details' which aren't rendered by theme_form_element() and therefore need to be separately covered.
Container in this case means that everything is contained between the opening and closing tags, both in the HTML source and visually. This has a number of consequences:
Most importantly: 'before' and 'after' is not what we'd usually expect here. Both ways the description would fall out of the container.
What we'd rather need is 'top' and 'bottom', with the former currently being hardwired.
In the 'details' case, the distinction between 'before' and 'top' is even more important, because in the 'before' case, the description would be rolled into the summary, while in the 'top' case it would be not.
And for both container types, the distinction between 'bottom' and 'after' is that in the former case it is rolled into the container, while in the latter case it falls out of it.
In principle, this also holds for the 'checkboxes' and 'radios' element:

title-before
description-before
--------
| description-top
| contents
| description-bottom
--------
title-after
description-after

So while for titles the current approach is fine, for descriptions we'd need four instead of just two possible placements.
Containers should default to the description being on 'top' (like 'fieldset' and 'details' now do) or on 'bottom' (like 'checkboxes' and 'radios' currently do) of contents, but also allow either of the other placements.
Monolithic elements (as most others) however should continue defaulting to 'after', while also allowing 'before', either disregarding or leniently matching the other two placements.

If you agree in principle, I'd roll a new patch following this logic. I'd also add a few more tests covering the whole range of cases, and extend documentation.
So while the patch needs work, the concept is for review.

xjm’s picture

Issue tags:-Needs change record

We don't add change notifications (or the tag) until an issue is committed. Use the API change (if there's a BC break) API addition (otherwise) tags to indicate when something will need a change notification after commit, and maintainers will tag it then.

mgifford’s picture

So just trying to rephrase a bit here.

There are more options for descriptions placements based on the type of input field involved.

  • Containers need the flexibility to change the placement of descriptions and should default to the description being:
    • on 'top' (like 'fieldset' and 'details' now do)
    • on 'bottom' (like 'checkboxes' and 'radios' currently do) of contents
  • Monolithic elements (as most others) however should continue defaulting to 'after', while also allowing 'before', either disregarding or leniently matching the other two placements.

I didn't get 4 options though. From the UI, I can't see how there would be more than 2 for any given item.

I'd like to see a patch though following this line of thought.

mgifford’s picture

mgifford’s picture

#76: description_display-314385-76.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +accessibility, +API addition, +Twig

The last submitted patch, description_display-314385-76.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new706 bytes
new14.87 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just a re-roll, other than the rejected $items['form_test/form-descriptions'] piece which I've included as an attachment. I don't know if the form_test.module still needs this or if this needs to be modified and added somewhere else.

Status:Needs review» Needs work

The last submitted patch, description_display-314385-89.patch, failed testing.

mgifford’s picture

I had trouble installing this on STM too. Applies no problem locally.

Sheldon Rampton’s picture

I hate to throw a curve this far into the discussion of this ticket, but I'm starting to wonder if the problem as stated in the title of this ticket should be reconsidered: "Make position of #description configurable via the API." The configuration options that are currently proposed are: before, after, and hidden. Does this really capture the range of options that developers might want or need for positioning the #description? Consider, for example, a WYSIWYG text field. It contains the following components:

  1. The field label
  2. The textarea, with its appearance and behavior modified by some Javascript which enables the WYSIWYG.
  3. The input format selector
  4. The filter tips associated with the selected input format (a type of help text).
  5. The #description.

Site builders might actually want to place the #description in several places, including:

  • Above the field label
  • Below the field label, but above the textarea.
  • Below the input format selector, but above the filter tips.
  • Below the filter tips (where it appears presently by default).

This suggests that the options of "before/after/hidden" may not be adequate for all field types or all design use cases. Offering these options would provide greater flexibility than currently exists, but does it make sense to put development effort into adding a couple of additional options that still don't provide enough flexibility?

Perhaps instead the focus should be on making it easier to change the positioning and behavior of the #description using theme_form_element, which currently I find difficult to use.

mgifford’s picture

There's also dealing with how to style for mobile content.... Would having a hidden-on-mobile be useful?

I would like to see some UI element, but can see how you might want to have even more control than before, after & hidden.

We might also want to set some more sensible defaults. Dave Reid's suggestion in #47 should definitely be fixed for everyone.

I'm not sure how to approach implementing your suggestion though. Ideally we'd be able to do more with theme_form_element() - more easily - and then simply have a UI that works with it.

In the mean time we've got a patch that helps give D8 a more flexible #description element...

Sheldon Rampton’s picture

@mgifford I'm not against getting this patch into D8. I think it's at least some kind of step forward.

Bojhan’s picture

Yhea, I think the wish list of "other positions" than above/below is endless. Lets get this in first.

mgifford’s picture

StatusFileSize
new15.64 KB
FAILED: [[SimpleTest]]: [MySQL] 59,509 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Ahh, finally had time to look at the install error again:
Unable to find template "core/modules/system/templates/form-element-description.html.twig" (looked into: /home/s772c3c09b7cb761/www).

Then search to determine that form-element-description.html.twig wasn't included in the last patch.

mgifford’s picture

Status:Needs work» Needs review

Go bot.

Status:Needs review» Needs work
Issue tags:-Usability, -accessibility, -API addition, -Twig

The last submitted patch, description_display-314385-96.patch, failed testing.

penyaskito’s picture

Assigned:Pancho» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new15.86 KB
FAILED: [[SimpleTest]]: [MySQL] 59,754 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Rerolled. form_build_form moved to FormBuilder::doBuildForm().

Status:Needs review» Needs work

The last submitted patch, 99: description_display-314385-99.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 99: description_display-314385-99.patch, failed testing.

jlbellido’s picture

Issue tags:+SprintWeekend2013

I'll work on this.

jlbellido’s picture

Assigned:Unassigned» jlbellido
mgifford’s picture

Thanks. This is especially important for #47.

jlbellido’s picture

Issue tags:-SprintWeekend2013+#SprintWeekend2014, +#D8SVQ

Updated tags

jlbellido’s picture

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
new17.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch description_display-314385-108.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Test failed because path '/form_test/form-descriptions' didn't exists, added it with @joe_carvajal!!

jlbellido’s picture

Issue tags:-#SprintWeekend2014, -#D8SVQ+SprintWeekend2014, +D8SVQ

Status:Needs review» Needs work

The last submitted patch, 108: description_display-314385-108.patch, failed testing.

jlbellido’s picture

Status:Needs work» Needs review
StatusFileSize
new17.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch description_display-314385-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled #108 and try again.

mgifford’s picture

Thanks @jlbellido - great to see this test finally pass!

For testing it is a matter of writing a form (via a module or the template.php) to use FAPI to position the description above or below as per #92.

I'll try to test that in the coming week. I just applied this in SimplyTest.me but there are no UI changes.

rootwork’s picture

Assigned:jlbellido» Unassigned

@mgifford did you get a chance to test this? I'd love to see this get in.

I unassigned @jlbellido since it seemed like his work was done.

mgifford’s picture

mgifford’s picture

Issue tags:+Needs reroll

@rootwork - I haven't had time sadly. I did quickly check though and this needs a re-roll.

Status:Needs review» Needs work

The last submitted patch, 111: description_display-314385-111.patch, failed testing.

jjcarrion’s picture

Assigned:Unassigned» jjcarrion

I'm going to try the re-roll

jjcarrion’s picture

Issue tags:+DrupalCampSpain
LewisNyman’s picture

Looking at this patch, it looks like it's now affecting a lot more than just the position of the description, can we clarify this is within the scope of this patch and update the issue title and description?

jjcarrion’s picture

Assigned:jjcarrion» Unassigned
Status:Needs work» Needs review
Issue tags:-DrupalCampSpain
StatusFileSize
new1.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,268 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Finally, I haven't make the reroll, ther was many changes around the initial change.

I have just add a new $variables['description_display'] with the value 'before', with that variable in the twig file we can render the description before or after de element.

penyaskito’s picture

I would move the definition from template_preprocess_form_element to FormBuilder::doBuildForm(), so we still have the option of defining where it would be rendered from Form API.

Please take the tests from last patches, they should work or at least provide a good start.

This would need an issue summary update as Lewis pointed out.

Thanks!

Status:Needs review» Needs work

The last submitted patch, 120: description-display-314385-120.patch, failed testing.

jjcarrion’s picture

Status:Needs work» Needs review
StatusFileSize
new4.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,747 pass(es), 30 fail(s), and 15 exception(s).
[ View ]
new4.8 KB

I have changed the patch with the modifications that penyaskito proposed. Here is the patch and the interdiff.

Status:Needs review» Needs work

The last submitted patch, 123: description-display-314385-122.patch, failed testing.

Risse’s picture

Assigned:Unassigned» Risse
Issue tags:+drupalcampfi

I'll take a look at this reroll.

Risse’s picture

Status:Needs work» Needs review
StatusFileSize
new4.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,671 pass(es), 30 fail(s), and 15 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 126: description_display-314385-126.patch, failed testing.

LewisNyman’s picture

Issue tags:+frontend
JacobSanford’s picture

Issue tags:-Needs reroll

Patch applies cleanly, removing 'Needs reroll' tag

JacobSanford’s picture

Issue tags:+Needs reroll

My mistake, I was squelching file-not-found errors. Resetting tag.

jday’s picture

Hi, new contributor here, working on re-rolling this patch.

jday’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new4.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,734 pass(es), 28 fail(s), and 15 exception(s).
[ View ]

Reroll of patch description_display-314385

Status:Needs review» Needs work

The last submitted patch, 132: description_display-314385-132.patch, failed testing.

mgifford’s picture

Issue tags:+Needs reroll
penyaskito’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new4.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,195 pass(es), 28 fail(s), and 15 exception(s).
[ View ]

Rerolled.

penyaskito’s picture

StatusFileSize
new4.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,194 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new531 bytes

Now it should get back to green.

The last submitted patch, 135: 314385-description-configurable-134.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 136: 314385-description-configurable-136.patch, failed testing.

penyaskito’s picture

Assigned:Risse» penyaskito

Part of the tests were missed after #111.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new5.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,490 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new4.39 KB

Now it is green for real.

Status:Needs review» Needs work

The last submitted patch, 140: 314385-description-configurable-140.patch, failed testing.

penyaskito’s picture

Status:Needs work» Needs review
StatusFileSize
new7.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,491 pass(es).
[ View ]
new1.89 KB

(facepalm) Forgot to add a class.

alimac’s picture

Issue tags:+TCDrupal 2014

@YesCT and I are going to review this patch later (but if anyone wants to jump in, please do).

We were wondering why the setting is called "invisible" instead of "visually-hidden" like the class that's used to make it visually hidden?

penyaskito’s picture

"invisible" instead of "visually-hidden" is for consistency with the title_display property: https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h....

If we consider renaming, we should do that on both, it is an API change, and I guess it is out of scope of this issue.

YesCT’s picture

Issue summary:View changes

1. Thanks, that makes a lot of sense.

2. but I didn't see where the new key and possible values for it was documented...

   * @return array
   */
  public function doBuildForm($form_id, &$element, FormStateInterface &$form_state);

does not document the element array and the possible values at all anyway.
ah, this is the whole form api documented in the big table thing.
We can have a separate issue to document this new #description_display array key and it's possible values.
Or find the issue to document form api stuff for drupal 8, and make sure that issue also documents this new key.

3.
still looking at this code and also working on an issue summary update with @alimac

alimac’s picture

Issue summary:View changes
StatusFileSize
new32.84 KB

Added steps to reproduce. Will add an "after" screenshot and steps in a bit.

YesCT’s picture

Re #145 2. created #2318105: Add documentation for new #description_display key for form api documentation to document this new property.

I think the steps to reproduce would really be like:
make a module that has a form, notice the description is after,
add this line to the module, notice your description in the form moved... or something.

We can probably use the test module for the before and after screenshots.

alimac’s picture

Issue summary:View changes
StatusFileSize
new29.19 KB
new28.65 KB

Sorry, I misunderstood. I updated the steps to reproduce (based on @YesCT's comment in #147) and added before and after screenshots from an example module.

alimac’s picture

StatusFileSize
new7.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,535 pass(es).
[ View ]
new4.39 KB

I updated the comments to use third person tense in the summary of classes (https://www.drupal.org/coding-standards/docs), and more accurately describe what the code is doing. There was also an indentation issue that I fixed.

The current options make visually hiding the description mutually exclusive from changing the positioning of the description -- so the option to have the description appear before a form element is not available to screen readers. We are going to use a screenreader test whether or not it should be mutually exclusive.

YesCT’s picture

Issue summary:View changes

issue summary updated.

YesCT’s picture

turning on the accessibility screenreader on my mac, the description is not read for me. hm.

oh! it does, if I pause long enough in the field.

And, it's using aria-describedby, in which the pattern is to read it "the help tag is ...."
so, there is no such thing as visually hidden before or visually hidden after.

OK.

oh, spotted one small nit. Having someone make the new patch right now.

akalata’s picture

StatusFileSize
new705 bytes
new5.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,237 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new7.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,410 pass(es).
[ View ]

Working w/ YesCT @ TCDrupal2014!

Nitfix: Documentation wrapping beyond 80 characters. See https://www.drupal.org/node/1354#drupal ("Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over").

Also added a test-only patch for the tests that were added.

YesCT’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

nice fixes. issue summary updated. and I read the whole patch.

The last submitted patch, 152: 314385-description-configurable-152-tests-only.patch, failed testing.

penyaskito’s picture

Awesome, thanks!

alimac’s picture

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

@Les Lim and I found that this patch didn't work for multiple value form elements (the description appeared below the fields in those cases). Going to write a test first, then work on a fix.

Les Lim’s picture

StatusFileSize
new120.33 KB
new155.13 KB

Elements with text format selectors are also a problem. Here are screenshots from our testing.

description placement problemsmore description placement problems

alimac’s picture

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

Changing it back to RTBC.

In #51 Dave Reid recommended creating a follow-up issue to this, for multiple value field widgets, so I just created it #2318757: Make position of #description configurable via the API for form field widgets.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed b366d7d and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Form/ElementsLabelsTest.php b/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
index 98721bf..dbf13d2 100644
--- a/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
+++ b/core/modules/system/src/Tests/Form/ElementsLabelsTest.php
@@ -99,7 +99,7 @@ function testFormLabels() {
    * Tests different display options for form element descriptions.
    */
   function testFormDescriptions() {
-    $test = $this->drupalGet('form_test/form-descriptions');
+    $this->drupalGet('form_test/form-descriptions');

     // Check #description placement with #description_display='after'.
     $field_id = 'edit-form-textfield-test-description-after';
diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestDescriptionForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestDescriptionForm.php
index 1b500bb..aeb1196 100644
--- a/core/modules/system/tests/modules/form_test/src/Form/FormTestDescriptionForm.php
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestDescriptionForm.php
@@ -9,7 +9,6 @@

use Drupal\Core\Form\FormBase;
use Drupal\Core\Form\FormStateInterface;
-use Symfony\Component\HttpFoundation\JsonResponse;

/**
  * Defines a form for testing form element description display options.

  • alexpott committed b366d7d on 8.0.x
    Issue #314385 by penyaskito, Pancho, mgifford, jjcarrion, jlbellido,...
penyaskito’s picture

Fabianx’s picture

Change notice on how to use the new API?

penyaskito’s picture

@Fabianx, you are right, thanks! Created https://www.drupal.org/node/2324025

Status:Fixed» Closed (fixed)

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

penyaskito’s picture

vitalie’s picture

Arla’s picture

abibbings’s picture

I'm very glad this issue has been addressed for Drupal 8.

People with low vision may use the site at an extreme zoom level, in which case the issue of the help text being below the field is exacerbated because they only see a very small portion of the screen at one time. Imagine the image in comment #47 but at 500% zoom. That's a TON of scrolling. ARIA doesn't really help users in this scenario.