Default field.tpl.php prints a clearfix class theme_field doesn't [#1388496]

Comment	File	Size	Author
#14	1388496-field-template-always-prints-clearfix-class_d7.patch	1 KB	barraponto
#7	1388496-field-template-always-prints-clearfix-class-7.patch	1.03 KB	barraponto
#3	1388496-field-template-always-prints-clearfix-class_d7.patch	1 KB	barraponto
#2	1388496-field-template-always-prints-clearfix-class.patch	1.03 KB	barraponto
#2	1388496-field-template-always-prints-clearfix-class_d7.patch	0 bytes	barraponto

Comment #1

johnalbin

they/他

English

Taipei, Taiwan

commented 29 December 2011 at 16:04

Title:	Default field.tpl.php prints a clearfix class, theme_field doesn't	» Default field.tpl.php prints a clearfix class theme_field doesn't
Status:	Active	» Postponed (maintainer needs more info)

theme_field() does get a "clearfix" class, but it is conditionally added. See http://api.drupal.org/api/drupal/modules--field--field.module/function/t...

It looks as if the field.tpl.php gets it unconditionally since its hard-coded in the template file. Actually, if the label is inline, I think it might print 2 clearfix classes. Capi, can you confirm that?

Log in or register to post comments

Comment #2

barraponto

he / him

commented 2 January 2012 at 01:05

Version:	7.x-dev	» 8.x-dev
Status:	Postponed (maintainer needs more info)	» Needs review

Status	File	Size
new	1388496-field-template-always-prints-clearfix-class_d7.patch	0 bytes
new	1388496-field-template-always-prints-clearfix-class.patch	1.03 KB

@JohnAlbin yes it does print clearfix twice. I believe it means field.tpl.php shouldn't print it unconditionally. I guess the straightforward patch for Drupal 8 is just removing the clearfix class from field.tpl.php (preprocess logic will insert it if needed).

For Drupal 7 I think we can follow the same strategy, since the template file is not used by default unless it has been copied to the theme folder — then changing the file will have no effect.

Attached are patches for both versions.

Log in or register to post comments

Comment #3

barraponto

he / him

commented 2 January 2012 at 01:06

Status	File	Size
new	1388496-field-template-always-prints-clearfix-class_d7.patch	1 KB

D7 patch is broken.

Log in or register to post comments

Comment #4

njwrigley commented 10 January 2012 at 21:28

Version:

8.x-dev

» 7.10

I'm using the Zen theme and this is causing text to fail to wrap around images which are floated.

If I set the fields to display 'above' then the issue goes away, so the clearfix applied if the field is 'inline' (field-label-inline) appears to be the root cause.

Log in or register to post comments

Comment #5

barraponto

he / him

commented 28 February 2012 at 14:25

Version:

7.10

» 8.x-dev

@njwrigley thanks for the feedback. Please don't change the version since this has been tested and affects the development version of both Drupal and Drupal 8.

Try upgrading Drupal 7 and applying the provided D7 patch, and post feedback on whether it works for you. It would speed up fixing this issue. See http://drupal.org/patch/apply for instructions.

Log in or register to post comments

Comment #6

28 February 2012 at 15:03

Status:

Needs review

» Needs work

The last submitted patch, 1388496-field-template-always-prints-clearfix-class_d7.patch, failed testing.

Log in or register to post comments

Comment #7

barraponto

he / him

commented 1 March 2012 at 01:38

Status	File	Size
new	1388496-field-template-always-prints-clearfix-class-7.patch	1.03 KB

Re-roll, trying to make it more explicit. There is some logic for printing 'clearfix' classes in template_preprocess_field:

  // Add a "clearfix" class to the wrapper since we float the label and the
  // field items in field.css if the label is inline.
  if ($element['#label_display'] == 'inline') {
    $variables['classes_array'][] = 'clearfix';
  }

Theming fields usually call the function implementation in field module. There is also a tpl.php implementation that is never called, it's only there to make designer's lives easier. It will only be used if it is copied to the theme folder. It has a bug though: a hardcoded clearfix class that always prints, and when the condition in template_preprocess_field() applies, it gets printed twice.

<div class="<?php print $classes; ?> clearfix"<?php print $attributes; ?>>

My patch just removes that hardcoded class. Anyone currently affected by this bug won't benefit from the patch, since they have already copied the tpl.php file over to their theme (and probably tweaked it further). But this bug will stop biting new users.

Log in or register to post comments

Comment #8

barraponto

he / him

commented 1 March 2012 at 01:45

Status:

Needs work

» Needs review

Go bot!

Log in or register to post comments

Comment #9

xjm

she/her

English

commented 1 March 2012 at 02:02

Status:	Needs review	» Needs work
Issue tags:		+Needs manual testing

So my concern here (as @barraponto and I discussed on IRC) is that the clearfix is only being added to the classes in the preprocess when #label_display is 'inline', but the template hardcodes it and prints it always.

The question is, should the clearfix also be added if the label is not inline?

If it should be, we should also remove the condition from around
```
$variables['classes_array'][] = 'clearfix';
```
in the template, and we can possibly backport without any concern. Edit: Hmm, maybe not. Someone might be overriding all of $classes but relying on that clearfix.
If it shouldn't be added to fieldsets with labels that aren't inline, then the patch would be fine as is, but not for backport (since we'd potentially be stripping a clearfix class out of fields all over production websites.)

It might also be good to get some manual testing for this in core themes for some fields with hidden, inline, and block labels.

Edit: Oops, crosspost. Testbot will still do the job, though. :)

Edit: Note also that this bug is very easy to work around in a custom theme. You just override the template. So I'm not seeing a strong case for backporting this, even though it's definitely a good fix.

Log in or register to post comments

Comment #10

johnalbin

they/他

English

Taipei, Taiwan

commented 1 March 2012 at 04:58

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

The question is, should the clearfix also be added if the label is not inline?

No, it shouldn't. The clearfix class is added there because Drupal has some completely whacky styling for fields with inline labels. It floats the label to the left and makes all the field content shift to the right of the label. IMO, that whole styling should be removed because it's not the styling that 99.9% of sites want for inline labels. They want the label to appear inline with the field content flowing around it.

However, what I just described is a feature request. Anyone care to open one up? It shouldn't stop progress on this legitimate bug.

If it shouldn't be added to fieldsets with labels that aren't inline, then the patch would be fine as is, but not for backport (since we'd potentially be stripping a clearfix class out of fields all over production websites.)

So, yes, the patch for D8 is just fine. But I disagree with your assessment for the D7 patch; it should be backported. The field.tpl.php file is not even used; core always uses the theme_field() function. The only time field.tpl.php is used is when a themer copies it to their theme in order to override core's theme_field() function. That means it is only a copy of field.tpl.php that is being used on 1000s of production sites. Fixing the original won't break any sites.