WIDTH x HEIGHT text

Nitpicking really, but why not lowercasing those, while you are at it?

The better solution is probably to use two textfields? Like [ 640 ] x [ 480 ] px or something where [ ] is a textfield?

Here's a patch with two textfields.

The last one don't include upload.admin.css.

when trying to apply the last patch it said like 62 was unknown line type, so I remade the patch and looks like a tab was messing things up, change the tab into spaces and made a new patch, tried to apply it and it works

(I didn't change anything to Bojhan's patch aside from the tab)

NOTE: this solution will cause screen readers not to read anything, as the "Maximum resolution ...." label isn't set as the label for any of the 2 fields, so even though this might be a usability improvement, it certainly isn't an accessibility improvement

It will be impossible to label two input fields with one label, as the for attribute of a label accepts only one element ID.

Recommendations:
1. use two labels, one for each input field.

2. If necessary hide one of the labels by using style="height: 0; overflow: hidden;", so that it does not display visually, but is available to screen-reader users. This will mean that there is less of a clickable area for the second input field.

Attempting to define a system class to make content invisible in #473396: Defining System-Wide Approaches to Remove, Make Invisible & Push Content Off-screen with CSS

Here's another on with floated textfields and a label for each textfield.

I like this solution, but I'm wondering a few things:

do we rly need to t() the 'x' and 'px' and are there any reusable classes we can utilize for the floating?

do we rly need to t() the 'x' and 'px'
Hm. Maybe someone like to translate it. Or append "pixels" instead of "px". But it wouldn't bother me if we don't wrap it in t().

are there any reusable classes we can utilize for the floating
I scanned current head for classes that uses float: left; only and found

.progress-disabled {
  float: left; /* LTR */
}
.ahah-progress {
  float: left; /* LTR */
}
.password-strength-title {
  float: left; /* LTR */
}

in system.css. Nothing to re-use in this issue.

I like where this is going. This patch will make the selection of max width/height for images much more intuitive and user friendly.

Couple of simple code styling fixes.

+ margin-left: .4em;

I think this should be 0.4em

+ if (($form_state['values']['upload_max_resolution_x'] != '0')) {

and

+ if (($form_state['values']['upload_max_resolution_y'] != '0')) {

These both have an extra set of parentheses that are not necessary.

Can we change the form_set_error() messages so that they are more specific. Something like, "The maximum allowed image height should be entered as a numeric value. Set to 0 for no restriction.", and the other one to "width".

+ '#field_suffix' => '<kbd>' . t('x') . '</kbd>'

Should have a comma at the end, we always add a trailing comma to large arrays.

Ok, updated the patch according to eojthebrave's notes and removed the useless check if the value not equals '0' around is_numeric().

Removed <kbd>.
Good enough for RTBC? :-)

The div with upload-max-resolution-y-wrapper seems to be unused? No one declares the class upload-max-resolution-y-wrapper.

Also, could we use #attributes to set the needed class, instead of adding yet another wrapper div with #prefix and #suffix? It's better to stay away from those as much as possible.

Looks like it needs one more re-roll. :)

@Dries: upload-max-resolution-y-wrapper is generated through forms api and is used in CSS.
In fact the CSS rule didn't had the effect I thought so I modified it a little.

@Damien: '#type' => 'item' did not have a '#attributes' key. Therefore we couldn't add these classes via '#attributes' => array('class' => 'form-item-wrapper form-item-resolution'),. I tried (in fact forms api is doing its magic even if its not documented) but the classes aren't added to the element.

So this has only very minor changes in upload.admin.css.

@stBorchert: why not just use 'markup'? because an 'item' with a description just generates an unbound label, and markup allows the use of attributes

$form['settings_general']['upload_max_resolution'] = array(
  '#value' => t('The maximum allowed image size (e.g. 640x480). Set to 0x0 for no restriction. If an <a href="!image-toolkit-link">image toolkit</a> is installed, files exceeding this value will be scaled down to fit.', array('!image-toolkit-link' => url('admin/settings/image-toolkit'))),
  '#attributes' => array('class' => 'form-item-wrapper form-item-resolution')
);

perhaps?

@seutje: 'markup' isn't wrapped into tags so we can't position it and we aren't able to place the fields beneath it.
And (additionally) we wouldn't have a title for this "field group".
Well, the label is unbound but I know no better way to build a structure like the one the last patch is providing.

Layout didn't change since comment #20 --> screenshot

Ok bot, here's a new one. Especially for you.

Committed to CVS HEAD. Thanks.

A small follow up issue --> #556138: Error 404 for image toolkit in the File uploads setting. :-)