Manage fields >> Tags label doesn't match input ID. I'm not sure where the edit-field-container-cardinality comes from but it doesn't match.

 <label for="edit-field-container">Maximum number of values users can enter</label> <span class="field-prefix"><div class="container-inline"></span> <div class="form-item form-type-select form-item-field-container-cardinality">
 <select id="edit-field-container-cardinality" name="field[container][cardinality]" class="form-select"><option value="1">1</option><option value="2">2</option><option value="3"></select>

WAVE-revew.png

CommentFileSizeAuthor
#66 1932074-followup-66.patch857 bytesandypost
#62 cardinality-label-1932074-62.patch989 bytesmgifford
#60 cardinality-label.png145.2 KBmgifford
#60 cardinality-label-1932074.60.patch1.34 KBmgifford
#58 cardinality-label-1932074.58.patch1.83 KBmgifford
#54 cardinality-label-1932074.54.patch1.83 KBmgifford
#50 cardinality-label-1932074.50.patch1.82 KBmgifford
#45 cardinality-label-1932074.45.patch1.82 KBmgifford
#38 cardinality-label-1932074.38.patch2.79 KBlarowlan
#38 Screen Shot 2013-05-13 at 5.42.54 PM.png35.07 KBlarowlan
#33 field-max-vals-1932074.33.patch2.78 KBmgifford
#33 interdiff-22-33.txt1.14 KBmgifford
#33 Fieldset_from_field_ui.png280.19 KBmgifford
#31 field-max-vals-1932074.31.patch2.33 KBmgifford
#31 interdiff-21-31.txt586 bytesmgifford
#31 Screen Shot 2013-04-17 at 1.19.09 PM.png58.29 KBmgifford
#22 field-max-vals-1932074.21.interdiff.txt1.28 KBlarowlan
#22 field-max-vals-1932074.21.patch2.33 KBlarowlan
#22 Screen Shot 2013-04-10 at 8.06.35 AM.png43.98 KBlarowlan
#18 Screen Shot 2013-04-08 at 10.29.49 PM.png92.84 KBmgifford
#17 Screen Shot 2013-04-09 at 8.55.42 AM.png40.35 KBlarowlan
#16 field-max-vals-1932074.16.interdiff.txt1.21 KBnick_schuch
#16 field-max-vals-1932074.16.patch1.87 KBnick_schuch
#12 Screen Shot 2013-04-03 at 11.09.40 PM.png55.34 KBmgifford
#12 Screen Shot 2013-04-03 at 11.10.40 PM.png85.96 KBmgifford
#8 field-max-vals-1932074.7.patch2 KBlarowlan
#7 field-max-vals-1932074.7.interdiff.txt1.93 KBlarowlan
#7 field-max-vals-1932074.7.patch2.38 KBlarowlan
#4 Screen Shot 2013-03-07 at 7.34.49 AM.png30.43 KBlarowlan
#1 field-max-vals-1932074.1.patch849 byteslarowlan
#1 Screen Shot 2013-03-06 at 7.52.04 PM.png31.91 KBlarowlan
#1 Screen Shot 2013-03-06 at 7.52.21 PM.png35.02 KBlarowlan
#1 Screen Shot 2013-03-06 at 7.52.30 PM.png63.5 KBlarowlan
WAVE-revew.png58.55 KBmgifford
MixedUpLabelInputOnTags.png182.22 KBmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

So this is some weird markup where the two fields have been grouped.
Moving the title to the select instead fixes the a11y issues, but does change the look - screenshots attached - adding tag accordingly.

Shot with normal cardinality
Screen Shot 2013-03-06 at 7.52.04 PM.png

Shot with 'more' cardinality aka other
Screen Shot 2013-03-06 at 7.52.21 PM.png

Wave toolbar pass
Screen Shot 2013-03-06 at 7.52.30 PM.png

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me and worked in SimplyTest.me

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

I still think we need UX change sign off here, will try and ping UX folks in irc.

larowlan’s picture

Here's the before shot

Screen Shot 2013-03-07 at 7.34.49 AM.png

Bojhan’s picture

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

I am definitely not sure about this change, @mgifford please give form changes like this a little bit more review.

The reason the labels are on top is for both a11y and ux reasons, changing that doesnt seem like a good idea - I am sure there are other methods to associate after all all our other input elements are associated? The fact that labels are above the forms actually has significant usability reasoning - do we need to rehash that?

larowlan’s picture

Will add a clear-left class to the select

larowlan’s picture

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

This seems hacky but I couldn't find a css way to do it because they share the same .container-inline parent.
The other options are re-factoring the theme_form_element_label function to allow passing in classes for the label.

larowlan’s picture

Wrong patch

mgifford’s picture

@Bojhan I was just focusing on the label/input issue, totally missed that the position had changed. Sorry.

I was just focusing on the output (which is now good):
<label for="edit-field-container-cardinality">Maximum number of values users can enter</label> <span class="field-prefix"><span class="clearfix">&nbsp;</span></span> <select id="edit-field-container-cardinality" name="field[container][cardinality]" class="form-select"><option value="1" selected="selected">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="-1">Unlimited</option><option value="other">More</option></select>

Lee, your other changes look good.

larowlan’s picture

Just so it is clear - this latest patch adds the following as a field prefix to force the field onto a new line after the label

<span class="clearfix">&nbsp;</span>
mgifford’s picture

#8: field-max-vals-1932074.7.patch queued for re-testing.

mgifford’s picture

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

Just ran this on SimplyTest.me and it looks like the pre shot in #4 to me so marking it RTBC again.

Screen Shot With patch applied:
Screen Shot With patch applied

Screen Shot highlighted by WAVE Toolbar:
Screen Shot highlighted by WAVE Toolbar

xjm’s picture

#8: field-max-vals-1932074.7.patch queued for re-testing.

larowlan’s picture

Component: node system » field_ui.module
Status: Reviewed & tested by the community » Needs work

Spoke with Mike about this on skype.
We should be using a fieldset for the label here
So 'Maximum number of fields...' goes in the legend element.
The other two fields get their own labels and we end up with more semantic markup and no empty span element.

swentel’s picture

#680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) is also changing the form structure to 'redesign' the cardinality settings, but we'll see who gets in first.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
1.21 KB

Here is a patch as per the recommendations in #14.

larowlan’s picture

Assigned: Unassigned » Bojhan
Issue tags: +Needs usability review
FileSize
40.35 KB

Screenshot of the above
Screen Shot 2013-04-09 at 8.55.42 AM.png

Assigning to bojhan for a review of this change.

Before shot is at #4

mgifford’s picture

This is great -> admin/structure/types/manage/article/fields/field_tags/field-settings

Now this is great from an accessibility perspective and way nicer code too.

xjm’s picture

Hmm, can we make sure that the accessibility fixes from this are going into #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield), and fix both issues there? Edit: Since that issue makes some of the needed changes here obsolete.

Bojhan’s picture

Why is this in a fieldset?

larowlan’s picture

Spoke with Bojhan about this in irc.
Fieldset element is fine but we don't want the styling (border).
Investigating hidden styling in views ui as pointed out by Bojhan.

larowlan’s picture

This adds .fieldset-no-border class to field.css to remove the border
Screen Shot 2013-04-10 at 8.06.35 AM.png

Bojhan’s picture

Assigned: Bojhan » Unassigned
Issue tags: -Needs usability review

Looking good, I hope front-enders wont kill us for this :)

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

That's slick. Nice semantic output. Did some testing in VoiceOver and it worked well there too.

swentel’s picture

Status: Reviewed & tested by the community » Needs review

Can we move this fix #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) (that one needs a re-roll) into as outlined by xjm in #19 ?

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

@swentel I'm moving this back to RTBC.

I don't see this as a race, but this was just marked RTBC & although the patches have some overlap, they aren't the same issue and I don't know if we should bundle them together. Least without a good explanation.

#680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) looks like a nice improvement.

Would like to hear other's opinions though. I'm not sure how best to proceed.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Since they are both followups fixing issues with the exact same ten lines or so of code, I agree with @swentel, which is why I asked in #19. That change makes this one obsolete, and currently, it will also revert the fixes being applied here. So let's apply the accessibility fix to that patch, which is already a usability fix for the same form element.

swentel’s picture

Status: Needs review » Needs work

I was going to merge this one in the other (already re-rolled, and should be good), but left it as is for now and set this to needs work for 2 reasons

  1. css needs to go into field_ui.admin.css, not field.css
  2. Can't we do something about the uppercase legend in that form in Seven, because that's really ugly :/ ?
mgifford’s picture

Thanks for the explanation @xjm

swentel, no problem moving the CSS to field_ui.admin.css. Do you want me to roll a new patch with that? Just want to help move this along.

Let's make any decision on upper case a new issue. It could well be really ugly, but that's something that can be changed quickly as it's own issue. Let's not open up the UI if we don't have to.

swentel’s picture

@mgifford yes go ahead - fine for me to split it off in separate issue re: uppercase.

mgifford’s picture

Status: Needs work » Needs review
FileSize
58.29 KB
586 bytes
2.33 KB

I don't think it works in field_ui.admin.css. I think the patch in #22 is still better.

Here's what I did though.
Screen Shot 2013-04-17 at 1.19.09 PM.png

swentel’s picture

You will have to add the css where this form is loaded in PHP.
field.css is for field templates, not for Field UI (which will be come entity ui soon)

mgifford’s picture

Ok, this is working now.

Fieldset with field_ui.css file

mgifford’s picture

Issue tags: -Accessibility

#33: field-max-vals-1932074.33.patch queued for re-testing.

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

The last submitted patch, field-max-vals-1932074.33.patch, failed testing.

mgifford’s picture

Did field_ui.admin.inc get completely re-written? 3 out of 3 hunks FAILED to apply. Couldn't even find pieces of them when grepping for pieces of each of those 3 chunks of code.

larowlan’s picture

Yeah would have gone with the FormInterface cleanup, will see if I can get a patch with some resetting on a local branch in git.

larowlan’s picture

Status: Needs work » Needs review
FileSize
35.07 KB
2.79 KB

Here's a re-roll to allow for the move of the field settings form to form interface.

Screenshot
Screen Shot 2013-05-13 at 5.42.54 PM.png
Looks identical to existing form but now with 111.2% more accessibility*.

Also added

fieldset.fieldset-no-border legend {
  text-transform: none;
}

So the text is lower case.

*Made that figure up

Bojhan’s picture

RTBC?

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. .fieldset-no-border is going to be required for other elements too.

mgifford’s picture

Status: Reviewed & tested by the community » Needs work

I just want clarification that core/modules/field_ui/field_ui.admin.css is the right place for this as if we want to use this CSS more generally this might be a problem.

mgifford’s picture

Status: Needs work » Needs review

#38: cardinality-label-1932074.38.patch queued for re-testing.

mgifford’s picture

Issue tags: +fieldset

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

The last submitted patch, cardinality-label-1932074.38.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

I think this piece can't be applied, so yanked it. Not sure if there is somewhere else for this code, but took it out of the patch and re-rolled it so it could be applied via git:

-      $form['field']['container']['#description'] = t('%unlimited will provide an %add-more button so users can add as many values as they like.', array(
-        '%unlimited' => t('Unlimited'),
-        '%add-more' => t('Add another item'),
-      ));
+      $form['field']['description'] = array(
+        '#type' => 'item',
+        '#description' => t('%unlimited will provide an %add-more button so users can add as many values as they like.', array(
+          '%unlimited' => t('Unlimited'),
+          '%add-more' => t('Add another item'),
+        ))
+      );
mgifford’s picture

#45: cardinality-label-1932074.45.patch queued for re-testing.

mgifford’s picture

Issue tags: +fieldset

tagging.

mgifford’s picture

Issue tags: -Accessibility, -fieldset

#45: cardinality-label-1932074.45.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Accessibility, +fieldset

The last submitted patch, cardinality-label-1932074.45.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.82 KB

re-roll

mgifford’s picture

Issue tags: -Accessibility, -fieldset

#50: cardinality-label-1932074.50.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Accessibility, +fieldset

The last submitted patch, cardinality-label-1932074.50.patch, failed testing.

falcon03’s picture

When I opened #2002336: Introduce a CSS class to hide borders of fieldset elements I was thinking to this kind of use-case :-)

I think that the styling should be provided by the system module so that it can be used across core without any problem since we will need something like that for other issues as well! :-)

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

Re-roll.

The last submitted patch, cardinality-label-1932074.54.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 54: cardinality-label-1932074.54.patch, failed testing.

mgifford’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.83 KB

reroll

mgifford’s picture

Patch still applies. Easy to test with SimplyTest.me by going to /admin/structure/types/manage/article/fields/node.article.field_tags/field

I think we should be using #2002336: Introduce a CSS class to hide borders of fieldset elements rather than adding more css to core/modules/field_ui/field_ui.admin.css

Which would mean adding the class fieldgroup rather than fieldset-no-border

However, this still doesn't deal with the Legend being all caps...

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 60: cardinality-label-1932074.60.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
989 bytes

Hmm.. Let's try that again.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Back at RTBC.

  • Commit a43b612 on 8.x by catch:
    Issue #1932074 by mgifford, larowlan, nick_schuch: Tags Includes Label...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

andypost’s picture

Status: Fixed » Needs review
FileSize
857 bytes

There's duplicated '#type' definition now

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Woops.. Correction looks good, applies nicely, and works.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit bb6730d on 8.x by webchick:
    Issue #1932074 follow-up by andypost: Remove duplicated #type...

Status: Fixed » Closed (fixed)

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