On the settings page for an image field, there should be an 'x' between the two max/min resolution fields and 'pixels' after them, but there isn't (see 1st screenshot).

Looking at the code on line 122 of image.field.inc in the core image module, there isn't a function set for '#theme_wrappers' for either the 'x' or 'y' field, so I take it that the function for the parent 'max_resolution' should be used; however, the theme_form_element() function doesn't account for child fields...

The other alternative is to simply add 'form_element' to each field's '#theme_wrappers' setting, but this puts a div around each field and therefore displays the fields on separate lines (see 2nd screenshot)...

CommentFileSizeAuthor
#4 758996-suffix_for_resolution_fields-4.patch2.81 KBstborchert
#4 758996-suffix_for_resolution_fields-4.png33.51 KBstborchert
image_field_suffixes_2.png12.81 KBAnonymous (not verified)
image_field_suffixes.png12.03 KBAnonymous (not verified)

Comments

aaron’s picture

aaron commented

good catch. seems as though (to me) that it should also specify width x height, as that won't be obvious to everyone.

Anonymous’s picture

Anonymous (not verified) commented

I think it mentions 'width' and 'height' in the description below the fields, it just got cut off in my screenshots...

stborchert’s picture

stborchert
he/him
Primary language German
commented
Priority: Normal » Major

While working on #686528: User picture dimensions not validated: Split user picture dimensions into two fields I noticed the same behavior.
Looking into #523478-38: WIDTH x HEIGHT text (the original issue for the dimension fields) I saw that we previously used 

'#type' => 'item',

and floated the fields.
This was changed while getting image.module into core to '#theme_wrappers' => array('form_element'), for the surrounding field and '#theme_wrappers' => array(), for both input fields.

Problem: without a theme function no additional information such as #title, #field_suffix or field_prefix is rendered.

Solution: we need either a new theme function for inline elements or we roll back the code and remove the #theme_wrapper property from these fields.

Thoughts?

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 758996-suffix_for_resolution_fields-4.png33.51 KB
new 758996-suffix_for_resolution_fields-4.patch2.81 KB

Created the patched the rolled back the use of '#theme_wrappers' => array().

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Looks fine.

And it works fine.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Hm. Strange. I wonder why this would've been done to begin with. The new code certainly looks much more sane.

Committed to HEAD. If this breaks anything, let's make sure we get some tests in. :P

Status: Fixed » Closed (fixed)

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