Remove trailing space from form element labels and field labels (HTML nbsp)

Tagging as needs review to the cue bot.

Related: #991522: Vertical tabs summaries have stray spaces. This fix would possibly fix the problem over there.

Patch in #4 looks good, applies cleanly, and conforms to coding standards.

+++ b/core/includes/form.incundefined
@@ -4487,7 +4487,7 @@ function theme_form_element_label($variables) {
-  return ' <label' . drupal_attributes($attributes) . '>' . $t('!title !required', array('!title' => $title, '!required' => $required)) . "</label>\n";
+  return ' <label' . drupal_attributes($attributes) . '>' . $t('!title!required', array('!title' => $title, '!required' => $required)) . "</label>\n";

Any reasont to leave this prefix before label?

+++ b/core/modules/field/modules/text/text.jsundefined
@@ -14,6 +14,7 @@ Drupal.behaviors.textSummary = {
+      $summaryLabel.append(' ');

same

+++ b/core/modules/field/modules/text/text.jsundefined
@@ -22,6 +23,7 @@ Drupal.behaviors.textSummary = {
+      $fullLabel.append(' ');

same

#1 adds a useless space befor label id="edit-menu-enabled"> <label

I think #2 & #3 probably makes sense

I think this patch is important. A lot of themes, and automated tests, will rely on the space if it is not removed. The space is, as droplet said, a CSS issue. The space is a bug, and needs fixing.

Marking as a duplicate of #991522: Vertical tabs summaries have stray spaces, you can widen the scope of that issue but it does pretty much the same thing and is 1.5 years older.

Same thing happened to me #1542700: Content type fieldset summaries contain extra spaces a couple months ago :)

This issue may be newer but it has the wider scope already and it has the patch for that, so I reckon this is the one we should keep open :)

Patch at #14 no longer works - line references wrong - i'll reroll.

re-roll of #14 ready for test

Re-rolled. However, due to this issue: http://drupal.org/node/1685140, the changes done to collapse.js in previous patches no longer is relevant (that whole chunk removed.

I'm not sure I'm doing this correctly, so feel free to look over and review. (followed instructions from http://drupal.org/patch/reroll to reroll)

Updating status.

So the test failed at: "Detect a Drupal installation failure"

Details:
10:13:17] Invoking operation [install]...
[10:13:18] Encountered error on [install], details:
array (
'@reason' => 'Installing: failed to arrive at database configuration page using install path http://drupaltestbot664-mysql/checkout/install.php?profile=standard&langcode=en',
)
[10:13:18] Review complete. test_id=335523 result code=7 details=Array
(
[@reason] => Installing: failed to arrive at database configuration page using install path http://drupaltestbot664-mysql/checkout/install.php?profile=standard&langcode=en
)

Is that because of the patch, or is something else going on?

Removing "old mode", "new mode" lines. Don't think that's necessary and hoping that is what was causing the tests to fail.

Third times a charm?

Discovered drupal_attributes call is now replaced by Attribute object. Modified patch and am resubmitting.

Changing status.

I am a novice and found this issue via the novice tag. This is the first core issue I have participated in.
I applied the patch cleanly.
The patch did remove trailing spaces from all labels with the exception of the ones in the login block. There is space between the label text and the "abbr" tag and I am not sure if it should be there.

loose_space.patch queued for re-testing.

Reproduced the issue

Patch 26 applied cleanly and does remove the trailing space inside tags.

Comment #28 noted that spaces still appear in labels of required fields, but this is to separate the text of the label from the asterisk which indicates it is a required field. I'm not sure if this is desired behaviour or not.

However, the leading space before tags still exists after a checkbox or radio button.

<div class="form-item form-type-checkbox form-item-menu-enabled">
 <input type="checkbox" id="edit-menu-enabled" name="menu[enabled]" value="1" class="form-checkbox"> <label class="option" for="edit-menu-enabled">Provide a menu link</label>

</div>

Again, I'm not sure if this is desired behaviour or not.

In both of these cases, I'd vote to keep those spaces in, otherwise we'll lose the visual separation unless a theme knows to style for it. But maybe that's what we want?

Updated issue summary.

Patch looks ok to me.

Regarding the "required" space, I'd suggest a CSS solution. But not the ugly margin-left: 2px workaround, but rather a .form-required:before { content: " "; } However, right now, the space is added in a t() call — although I have no idea why the required marker is being passed to the t() function. Another issue, maybe?

Anyway, I'll add the CSS generated content workaround to the issue summary. It's safe for every browser and IE>8, see http://caniuse.com/#feat=css-gencontent

Patch #26 works fine, but we need to decide if we want to keep spaces between checkboxes and labels, and between label text and the asterisk indicating a required field. suegegambit

This cannot be backported to D7, as it would potentially break hundreds of thousands of themes. ;)

However, we absolutely need to get rid of this.

There's one more incarnation of this, which is even worse: theme_field() outputs an explicit  . Can't get any more ugly. ;) Let's also fix that instance right here, right now.

In general, this patch should remove the offending white-space everywhere, instead of moving it around. If necessary, the CSS should inject an appropriate margin (or padding).

+++ b/core/includes/form.inc
@@ -4541,7 +4541,7 @@ function theme_form_element_label($variables) {
-  $required = !empty($element['#required']) ? theme('form_required_marker', array('element' => $element)) : '';
+  $required = !empty($element['#required']) ? ' ' . theme('form_required_marker', array('element' => $element)) : '';

+++ b/core/modules/field/modules/text/text.js
@@ -39,6 +39,9 @@ Drupal.behaviors.textSummary = {
+      // Add space before the link.
+      $link.prepend(' ');

Both of these should be exclusively handled by CSS.

@stevecowie, any chance to get back to this?

So... erm... the majority of this issue/patch actually happened as part of the patch in #1838114: Change node form’s vertical tabs into details elements already... (I *knew* I touched those very lines already...)

Re-rolling the last patch against HEAD, and incorporating theme_field() leaves us with the attached patch only.

Let's see how it goes.

#35: drupal8.label-whitespace.35.patch queued for re-testing.

Edit: Failed Git checkout.

Yes, we can. But not with a space. CSS is the key. :)

+++ b/core/modules/block/block.jsundefined
@@ -23,7 +23,7 @@ Drupal.behaviors.blockSettingsSummary = {
-      return $.map(vals, $.trim).join(', ');
+      return vals.join(', ');

I don't understand what this change is doing, I read the comments, and didn't see this mentioned.

+++ b/core/modules/field/field.moduleundefined
@@ -1256,7 +1256,7 @@ function theme_field($variables) {
-    $output .= '<div class="field-label"' . $variables['title_attributes'] . '>' . $variables['label'] . ':&nbsp;</div>';
+    $output .= '<div class="field-label"' . $variables['title_attributes'] . '>' . $variables['label'] . '</div>';

+++ b/core/modules/field/theme/field.cssundefined
@@ -9,6 +9,9 @@
+form .field-edit-link {
+  margin: 0 0.3em;
+}

this part makes sense, they removed the space, and added a margin in CSS.

The coding standards look fine.

The change to theme_field() requires manual testing + before/after screenshots for confirmation.

With those, this patch should be RTBC. Any takers? :)

#39: drupal8.label-whitespace.39.patch queued for re-testing.

Can anyone do a quick visual review to confirm the changes of this patch? :)

Alright, one before vs. after screenshot for the removal of whitespace and colon from field labels:

And one additional screenshot for the .field-edit-link margin tweak, affecting the "(Edit summary)" link:

With these, we should be RTBC here.

#39: drupal8.label-whitespace.39.patch queued for re-testing.

if the testbot says green, rtbc from me.
the code was already looked at in #40 and #41, and I looked at it, again, looking good.

The manual testing and screenshots are in #46

We all like pretty HTML. :-) Committed to 8.x. Thanks.

Several issues with the Field part of this :

+form .field-edit-link {
+  margin: 0 0.3em;
+}

.field-edit-*link* ? what does this match ?

- The patch removes a :&nbsp from theme_link() (used for *displayed* entities), and replaces (?) it with some margin on forms ?

- That :&nbsp (the column and the space) was there when the field display is configured with label "inlined". That case is now broken - see screenshot below for field "Some text" :

- If it gets removed from theme_field(), it also needs to be removed from field.tpl.php

(also, note from the screenshot that bartik's bartik_field__taxonomy_term_reference() still prints the column for taxonomy field labels)

The quoted CSS is for the "Edit summary" link of text_summary field widgets in a form. (see #46)

Thanks, looks like we overlooked inline field labels :) For these, we should add a class and corresponding CSS rule along the lines of this:

.field-label--inline::after {
  content: ': ';
}

Updated issue summary.

I'm working on a patch and screenshots. We're handling this issue in Drupal Con Austin in the front end/twig sprint. Will submit shortly.

Submitting a patch for this which removes the   and replaces it with CSS. Tested in Stark and Bartik with LTR and RTL languages. I'm attaching some screenshots:

Bartik before patch:

Bartik after patch:

Stark before patch:

Stark after patch:

Fixed the test as well and re-submitting my previous patch. I also uploaded an interdiff for easier review.

1. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFormatterTest.php
   @@ -178,7 +178,7 @@ public function testEntityFormatter() {
   -      <div class="field-label">Body:&nbsp;</div>
   +      <div class="field-label">Body:</div>
   

   You'd need to remove the ':' as well because that is no longer part of the markup.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -636,6 +636,9 @@ table tr.error {
   +  content: ': ';
   

   This is not equivilent to a   but considering it's in a div, that's probably fine but just wanted to point that out.

Thanks Joel! I delayed to re-submit the patch because I was trying to reproduce the fail on my local environment and couldn't. So I'm resubmitting the patch with the same correction you are suggesting. I'm also submitting the interdiff.txt. Lets cross our fingers :-).

yw @cesarmiquel and thanks for working on this!

@sun you can do the honors?

RTBC++

Looks good to me, just want to note for the commit message that @zetagraph worked on this at the sprint also and should get a commit mention.

Could be better but something like:

Issue #1623574 followup by cesarmiquel, zetagraph: Fixed Remove trailing space from form element labels and field labels (HTML nbsp).

As Scott said, Andrei (a.k.a. @zetagraph) also contributed significantly to getting this done. Glad to have helped and hoping to contribute more on other issues :-).

I was able to reproduce the extant issues as I understand them in Stark in RTL and LTR on Chrome and Firefox 30.0 on OS X 10.9.3. They are:

• Unwanted space after inline field labels.
• Excess spacing after "Body" in "Body (Edit summary)"

The patch applied cleanly and fixed those issues.

But in Bartik there is an extra colon after the taxonomy label.

This patch removes the extra colon.

After applying patch from #66 there is no colon at all:
Before patch:

After patch:

In Bartik and Stark:

#65 produces two colons after the taxonomy label when the label is inline and one when the label is above.

#66 produces one colon after the taxonomy label when the label is inline and none when the label is above.

@cilefen: why to remove colon when label is above?

@valthebald True. Is it as simple as this?

#70 looks fine, should moved to Need Review status. As interdiff-66-70.patch filename was wrong, it should be interdiff-66-70.txt. Rename and uploaded interdiff-66-70.txt file and moved this to Need Review status.

Why does the latest patch add a colon to all field labels now?

Only inline field labels should be adjusted, as in the previous patches up till #66.

+++ b/core/modules/system/css/system.theme.css
@@ -636,7 +636,7 @@
-.field-label-inline .field-label::after {
+.field-label::after {
   content: ':';
 }

Re-roll of 66

Inline

Above

Thanks for re-rolling this patch! It looks good just one question:

+++ b/core/modules/system/css/system.theme.css
@@ -588,18 +588,18 @@ table tr.error {
-form .field-edit-link {
-  margin: 0 0.3em;
-}

Was this accounted for? Could you take a screenshot of this? It was taken in #46 for the summary field I believe.

The margin on .field-edit-link was removed in patch-65 and looks alright:
Without margin:

With margin:

Looks good to me. Unfortunately this needs a reroll.

reroll

Screenshots clearly show the difference, thanks @mbroere
Patch looks good as well

1. +++ b/core/modules/system/css/system.theme.css
   @@ -596,18 +596,18 @@ table tr.error {
    .field-label-inline .field-label,
    .field-label-inline .field-items {
      float:left; /*LTR*/
   -  padding-right: 0.5em;
   +  margin-right: 0.5em;
    }
   

   Should have /*LTR*/ comment on margin-right too.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -596,18 +596,18 @@ table tr.error {
    [dir="rtl"] .field-label-inline .field-label,
    [dir="rtl"] .field-label-inline .field-items {
      float: right;
   -  padding-left: 0.5em;
   +  margin-right: 0.5em;
   +}
   

   don't you mean margin-left and don't we have to reset margin-right here?

EDIT: Improved code context

@alexpott has absolutely right. I've attached improved patch, interdiff and some screens how it looks now.

LTR without margin:

LTR with margin:

RTL without margin reset:

RTL with margin reset:

Awesome, thanks for the screenshots. This looks like it should do the trick and addresses #85

Committed f04c960 and pushed to 8.0.x. Thanks!