Problem/Motivation

Steps to reproduce:
1. Add new content type at /admin/structure/types/add
2. Make sure some options are selected under "Publishing options"

Actual result:
Summary shows:
"Published , Promoted to front page , Create new revision"
See https://www.drupal.org/files/issues/comma-before-space-bug.png

Expected result:
Summary shows:
"Published, Promoted to front page, Create new revision"
See https://www.drupal.org/files/issues/comma-before-space.png

Proposed resolution

Strip unwanted whitespace before commas

Remaining tasks

- Add JS testing
- Review Patch

User interface changes

- Stripped unwanted commas

API changes

None.

Data model changes

None.

Files: 

Comments

hugovk created an issue. See original summary.

hugovk’s picture

Here's a patch against 8.4.x.

Cause:
core/modules/node/content_types.js is comma joining an array that contains newlines and whitespace:

["↵ ↵↵ Published↵ ", "↵ ↵↵ Promoted to front page↵ ", "↵ ↵↵ Create new revision↵ "]

Fix:
Trim the values before joining:
["Published", "Promoted to front page", "Create new revision"]

See patch or https://github.com/hugovk/drupal/compare/8.4.x...hugovk:8.4.x-no-space-b...

The patch doesn't trim before the other return vals.join(', '); lines in the same file, as it doesn't look like there are multiple values to be shown. But it may be a good idea to include the fix there too.

hugovk’s picture

FileSize
136.29 KB
hugovk’s picture

hugovk’s picture

Issue summary: View changes
hugovk’s picture

Note: this problem doesn't occur for "Language settings".

cilefen’s picture

Status: Active » Needs review
wturrell’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

- I've updated issue summary template
- able to reproduce original problem
- successfully fixed by patch
- the javascript looks absolutely fine and no errors in JS console
- shouldn't require browser testing
- have tried various combinations of settings in all vertical tabs, no regressions
- patch is in scope
- comment makes sense
- no string changes

P.S. should this be moved from 8.4.x to 8.3.x as it's a bug fix?

vaplas’s picture

Status: Reviewed & tested by the community » Needs review

#2 this trick looks nice. But why do we need it? Because we have the ["↵ ↵↵ Published↵ ", "↵ ↵↵ Promoted to front page↵ ", "↵ ↵↵ Create new revision↵ "]? But why do we have this result? Because we collect values as

$(context).find('input[name^="options"]:checked').parent().each(function () {
          vals.push(Drupal.checkPlain($(this).text()));
        });

But why do we do this way? It was appear here. But after we have next change (here):

-      $('input:checked', context).parent().each(function() {
+      $('input:checked', context).next('label').each(function() {

Can we use this approach here?

Also we have several different code in one small file:

$context.find('#edit-submission').drupalSetSummary(function (context) {
$('#edit-language', context).drupalSetSummary(function (context) {

$('input:checked', context)
$editContext.find('input:checked')

Maybe try to bring to a common code mean?

vaplas’s picture

this is what I meant in the local and global scope

hugovk’s picture

I had a quick test of those two patches in Drupal 8.2.5 and they both worked.

Dinesh18’s picture

I have tested both the patch and it is working as expected.

th_tushar’s picture

Status: Needs review » Reviewed & tested by the community

Looks like RTBC!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs tests, +JavaScript, +needs JavaScript review, +Needs issue summary update

Thanks for your contributions on this!

@wturrell, thanks for the step-by-step documentation of your review!

@Dinesh18, to receive credit for your review and testing, please document how you documented the patch and what is working as expected. Simply saying it is "working as expected" does not confirm any fix and does not help the issue get in. One way to demonstrate your testing would be to post before screenshots (without the patch) and after screenshots (with the patch). Also write out what you did to test it.

For this issue, we should also be able to add JavaScript tests to prove the fix I think.

There are two patches attached in #10. One of them is a one-line fix, and the other seems like way too much code to change for extra spaces. Let's pick one and document why in the IS. Maybe a JS maintainer can give feedback and confirm the correct fix. Tagging as a JS issue also so JS maintainers can find the issue if they like.

Finally, I believe there is a duplicate of this issue somewhere in the queue, possibly from many years ago. Can someone search and try to find duplicate issues in the queue to see what other work might be in progress or already fixed?

Thanks everyone!

droplet’s picture

- one line patch looks fine to me.
- I didn't align it line-by-line. The huge patch looks like a refactoring rather than bug fixing. It should split into a new issue. Thanks.

Addition:
- It should be only ONE "#edit-title-label". No join is required

vaplas’s picture

Assigned: Unassigned » vaplas

I'll do the JS test now.

vaplas’s picture

Assigned: vaplas » Unassigned
Status: Needs work » Needs review
FileSize
3.21 KB
3.85 KB

I'm sorry for the delay. The patch checks the appearance of labels and separated by commas. But not testing this:

if (!$(context).find('#edit-options-status').is(':checked')) {
          vals.unshift(Drupal.t('Not published'));
        }

...

if (!$editContext.find('#edit-display-submitted').is(':checked')) {
          vals.unshift(Drupal.t("Don't display post information"));
        }

We need testing it?

+++ b/core/modules/node/content_types.js
@@ -20,13 +20,11 @@
-        var vals = [];
-        vals.push(Drupal.checkPlain($(context).find('#edit-title-label').val()) || Drupal.t('Requires a title'));
-        return vals.join(', ');
+        return Drupal.checkPlain($(context).find('#edit-title-label').val()) || Drupal.t('Requires a title');

Looks like a refactoring too, no?

vaplas’s picture

Only one found by search "space|spaces|comma|commas" in "javascript|node system".

The last submitted patch, 17: 2849100-17-test-only.patch, failed testing.

hugovk’s picture

Thank you vaplas!

So this bug has been around since at least Drupal 7 and 2010 (!) when it was reported in https://www.drupal.org/node/991522. That was then closed as a duplicate of another, but that other one was committed in 2014 and didn't fix this problem, and then was reopened 10 months ago and has sat dormant since.

It'd be great if we could get one of these patches in for D8 and then consider it for D7 too :) What's needed for that?

Dinesh18’s picture

@xjm : I have applied the patch mentioned in #10. Here are the screengrab requested before and after the patch.
PFA
1) Edit Article content type Drupal8_after patch applied.png
2) Edit Article content type Drupal8_ before applying the patch.png

chiranjeeb2410’s picture

Status: Needs review » Reviewed & tested by the community

JS test patch applies cleanly. Changes are good to go with. Changing to RTBC.

xjm’s picture

Issue tags: -Needs JS testing

Great work on the screenshots and adding JS tests! We might need to make some small additional improvements to the patch. Hopefully I or another committer can follow up about that soon. I just wanted to say though that it's great to see the progress here in the meanwhile.

Thanks also to @chiranjeeb2410. Just a note, in the future, be sure to specify once you tested and the reasons that you concluded that a patch is ready to go. Thank you for reviewing the JavaScript change!

vaplas’s picture

Thanks for reviews! Unfortunately, my patch looks bit doubtful. Therefore, we need to stay on the NR. For example:

  • +++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
    @@ -0,0 +1,97 @@
    +  private function checkZone($page, $zone) {
    ...
    +  private function checkedAllCheckboxes($page, $selector) {
    ...
    +  private function waitSummaryUpdateAfterChanges($page, $items, $selector) {
    

    private or protected or not use additional functions?

  • one test, instead of dividing into several logic tests (test updates, test commas)?
  • not cover the full content_types.js logic ('Published/Not published', 'Title/Requires a title')
  • my not-oxford comments :)

Attached patch testing only commas, but it is not equivalent too :(

chiranjeeb2410’s picture

@xjm,

Thanks for the update. Would keep that in mind.

chiranjeeb2410’s picture

@xjm,

Thanks for the update. Would keep that in mind.

The last submitted patch, 24: 2849100-24-test-only.patch, failed testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Setting back to NR as suggested by @vaplas in #24, for review of the updated patch. Thanks!

vaplas’s picture

-    $is_spaces_between_comma = $page->waitFor(10, function () use ($page) {
-      $summaries = $page->findAll('css', '.vertical-tabs__menu-item-summary');
-      foreach ($summaries as $summary) {
-        if (strpos($summary->getText(), ' , ') !== FALSE) {
-          return TRUE;
-        }
-      }
-      return FALSE;
+    // Wait while summaries text will changed.
+    $page->waitFor(10, function () use ($page, $init_summaries_text) {
+      $summaries_text = $page->findAll('css', '.vertical-tabs__menu')[0]->getText();
+      return $init_summaries_text !== $summaries_text;
     });

Bit improvement, now test more clearly.

droplet’s picture

private or protected or not use additional functions?

"protected", allowed to override :)

one test, instead of dividing into several logic tests (test updates, test commas)?

If you could, a standard testing for SUMMARY text updates would be great! BUT for this issue, we only need test commas.

A standard test, for example, clicking 2 options in Publishing Options:

expected string: "Published, Promoted to front page"
(this is also testing commas)

The last submitted patch, 29: 2849100-29-test-only.patch, failed testing.

vaplas’s picture

@droplet, thanks for help with my questions! I can add needs tests without problem. Which procedure is best for this?

  1. Fix this.
  2. Create and fix issue about full cover of content_types.js.
  3. Create and fix issue about refactoring content_types.js.
  4. Create issue about refactoring tests after refactoring of content_types.js.

Right?

I would also add a couple of thoughts to the #29 patch.
$page->findAll('css', '.vertical-tabs__menu')[0]->getText();
It looks rude, because captures all region with tabs like one text. But this is also its advantage as prevent regression in the case with adding new tab. Also we cann't testing only one tab, because content_types.js implements a collection of elements for each tab by different code.

vals.push(Drupal.checkPlain($(context).find('#edit-title-label').val()) || Drupal.t('Requires a title'));
return vals.join(', ');
...
$(context).find('input[name^="options"]:checked').next('label').each(function () {
return vals.join(', ');
...
vals.push($('.js-form-item-language-configuration-langcode select option:selected', context).text());
return vals.join(', ');
...
$editContext.find('input:checked').next('label').each(function () {
return vals.join(', ');

But maybe we can simplify this test after refactoring of js.

droplet’s picture

Maybe:
- Fix this, write a test for Publishing
- Full test cover of content_types.js (we can do it together, just clone it 4 times, nothing more)
- Refactoring content_types.js

But this is also its advantage as prevent regression in the case with adding new tab.

I'd suggested just to test what we have now.

1. Perform a click to cancel one default setting.
2. Compare expected result to jQuery('[href="#edit-workflow"]').find('.vertical-tabs__menu-item-summary').text()

Tests should not affect by a simple refactoring like Patch #10.

The last submitted patch, 34: 2849100-34-test-only.patch, failed testing.

droplet’s picture

  1. +++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
    @@ -0,0 +1,51 @@
    +    $page->findField('options[status]')->check();
    +    $page->findField('options[promote]')->check();
    

    can we uncheck one or also check Sticky, because these 2 are default option. (even the script is broken, we will see correct Summary Text)

  2. +++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
    @@ -0,0 +1,51 @@
    +    $page->waitFor(10, function () use ($page, $selector, $summary_text) {
    +      return $summary_text !== $page->findAll('css', $selector)[0]->getText();
    +    });
    

    We don't need this check I think, or merged with code below as condition.

wturrell’s picture

I've just reviewed this again too…

- still able to reproduce original problem
- still fixed by patch
- still no JS errors
- coding style of the test looks perfect, no errors
- note: the waitFor time is in milliseconds
- as @droplet says, we've only got one test combination here. That doesn't bother me especially, but you could try turning Published off (which'll change it to "Not published") and maybe enable "Sticky" and disable "Create new revision" or something - i.e. do it in such a way it's had to change the entire sentence. And actually, if you're going to do that, might as well do another test with them all unchecked and make sure there are no commas…

+1 for RTBC from me though otherwise…

vaplas’s picture

@droplet, @wturrell thanks for reviews!

I like idea about 'uncheck status' and testing 'Not published', but we cann't do only it, because it set manual (without right space). Hence comma between "Not published" and "Promoted to front page" will correct always:

vals.unshift(Drupal.t('Not published'));
...
# result 'Not published, Promoted to front page , ...'

Also we need waiting after check()/uncheck(), because the script does not always have time to execute. We can wrapping wait by assertTrue, but the error message is not very informative: false is not true (see all_patch).

even the script is broken, we will see correct Summary Text

If content_type.js not run, we have empty Summary Text. Or I misunderstood this phrase? Therefore, can we rely on the default settings and check the script immediately (see mini_patch)?

Bit clean-up in mini_patch:

-  public static $modules = ['language', 'node'];
+  public static $modules = ['node'];

Because we not testing "Language settings" now. (edit: opps, i'm do it for all patch, but not mini patch)

- $page->findAll('css', $selector)[0]->getText();
+ $page->find('css', $locator)->getText();
  • $selector to $locator, because public function find($selector, $locator),
  • findAll(...)[0] to just find(...).

What about names and comments:

 * Tests the JavaScript updating of summaries on content type form.
class TestSettingSummariesContentType
   * Test a vertical tab 'Workflow' summary.
public function testWorkflowSummary()

The last submitted patch, 38: mini_2849100-38-test-only.patch, failed testing.

The last submitted patch, 38: all_2849100-38-test-only.patch, failed testing.

Truptti’s picture

FileSize
24.87 KB
23.14 KB

Verified the patch 'mini_2849100-38.patch' in #38 on Drupal 8.4.x site, below are the observations
1.Before applying the patch space was displayed before comma on Publising options
2.After applying patch 'mini_2849100-38.patch' on Drupal 8.4.x, the patch applied smoothly and the space before comma at Publishing options is removed. Attaching snapshot beforepatch.png and afterpatch.png for reference.

This can be marked as RTBC.

droplet’s picture

@vaplas,

You are right.

I preferred all.patch, even the error almost never happen in Drupal Core :) But I think tests are designed for unexpected. We able to make the script does not work after the first execution.

+++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
@@ -0,0 +1,47 @@
+    $this->assertTrue($page->waitFor(10, function () use ($page) {
+      $summary = $page->find('css', '[href=\#edit-workflow] .vertical-tabs__menu-item-summary')->getText();
+      return $summary === 'Not published, Promoted to front page, Sticky at top of lists, Create new revision';
+    }));

what if drop the waitFor also?

\#, escape? is it required?

href="#edit-workflow", please :)

droplet’s picture

Priority: Minor » Normal
Status: Needs review » Needs work
vaplas’s picture

href="#edit-workflow", please :)

Thanks for hint!

what if drop the waitFor also?

This code works:

$page->findField('options[status]')->uncheck();
$page->findField('options[promote]')->check();
$this->assertContains('Not published', $summary);

But this code not works:

$page->findField('options[status]')->uncheck();
$this->assertContains('Not published', $summary);

The last submitted patch, 44: 2849100-44.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: 2849100-44-fail.patch, failed testing.

vaplas’s picture

Strangely. Trick from 2849100-44.patch works on my machine. In any case, I think it only further confirms the need to wait the script.

droplet’s picture

Sorry @vaplas, I thought that works...So to prevent random failure. Let's keep waitFor.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
821 bytes
2.26 KB

The test fail looks like it was expecting a string : 'Not published, Promoted to front page, Sticky at top of lists, Create new revision'
But the output text was: 'Published, Promoted to front page, Create new revision'
So I changed the test file to check for the latter text.

vaplas’s picture

droplet’s picture

Status: Needs work » Needs review
vaplas’s picture

The last submitted patch, 50: 2849100-50-test-only.patch, failed testing.

The last submitted patch, 52: 2849100-51-test-only.patch, failed testing.

hugovk’s picture

droplet, tameeshb: any comments on vaplas's latest patches?

NikitaJain’s picture

Verified the patch '2849100-51.patch' in #52 on Drupal 8.4.x version.
- Able to reproduce the issue before applying the patch.
- Patch applied successfully without any errors.
- Before applying the patch:
a) The space was appearing before commas for all publishing options (Published, Promoted to front page, Sticky at top of lists, Create new revision).
b) For 'Not published' status, the space was not there before commas. So in this case the issue is already resolved.

- After applying the patch:
a) The space before comma has been removed for all other publishing options (Published, Promoted to front page, Sticky at top of lists, Create new revision).
b) Also after unchecked status and checked sticky its working fine
(Not published, Promoted to front page, Sticky at top of lists, Create new revision)
c) The above patch works fine in all other conditions.

Screen-shots attached

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/node/content_types.js
    @@ -26,7 +26,7 @@
    +        $(context).find('input[name^="options"]:checked').next('label').each(function () {
    

    Should we add a class for the label so to allow overriding the label with another element. This also documents the dependency from JS to this element.

  2. +++ b/core/modules/node/tests/src/FunctionalJavascript/TestSettingSummariesContentType.php
    @@ -0,0 +1,49 @@
    +   * Test a vertical tab 'Workflow' summary.
    

    The vertical tab should be called 'Publishing options'

gaurav.kapoor’s picture

Assigned: Unassigned » gaurav.kapoor
gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
608 bytes
gaurav.kapoor’s picture

Assigned: gaurav.kapoor » Unassigned
vaplas’s picture

I'm sorry for the delay.
#55: @hugovk thank you for good question!)
#56: @NikitaJain thank you for detailed review!
#57: @lauriii thank you for review and good ideas!
#59: @gaurav.kapoor thank you for help with 57.2!

added 57.1 point.

NikitaJain’s picture

Verified the patch '2849100-61.patch' in #61 on Drupal 8.4.x version.
- Able to reproduce the issue before applying the patch.
- Patch applied successfully without any errors.
- Before applying the patch:
- The space was appearing before commas for all publishing options (Published, Promoted to front page, Sticky at top of lists, Create new revision).
- After applying the patch:
a) The space before commas has been removed for all other publishing options (Published, Promoted to front page, Sticky at top of lists, Create new revision).
b) Checked for all other conditions, its working fine.
Screen-shots attached

vaplas’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.39 KB
1.26 KB

@NikitaJain, thank you for nice user review again! But I think we should wait for the code review too.

For example, I added a class '.js-option__label'. And __ (double underline) is used in the css for the child pointer. But label is not child of input. Hence, maybe better use '-'. Added patch with it (repeated user review for this patch is not necessary, only the code).

vaplas’s picture

FileSize
3.39 KB

opps, #63 contains correct interdiff, but old patch. New patch here.

droplet’s picture

Being more complicated over time. If committers agreed, we should fix them all with other vtabs in a new refactoring.

lauriii’s picture

Status: Needs review » Needs work

I discussed with @nod_ and we agreed that even though core has plenty of js- prefixed classes, all additions should be done using data attributes. So instead of what I proposed on #57.1, we should use data attributes as a selector when using JS since that is even less likely to be messed up by anyone. Sorry for giving slightly off directions earlier :)

vaplas’s picture

Being more complicated over time.

@droplet, yeah. You know, I was offered a just check the entire '.vertical-tabs__menu' region (#29) and go home to full refactoring issue :)

@lauriii, no problem.

Tag input already have data-drupal-selector with 'edit-options-*' values. Which is better to choose for the label:

  1. label
  2. option-label (current choose)
  3. edit-options-label
  4. edit-options-*name*-label
  5. *your variant here*

Which selector is best: input[data... or just [data....

droplet’s picture

Assigned: Unassigned » lauriii

there's a way to guard the accessibility also. use [for] attr.

If we do #67 way, IMO, we should not use jQuery next that allowed more freedom custom coding.

hugovk’s picture

Being more complicated over time.

Yep, nearly 70 comments! So where are we up to? It'd be great to get something merged and fix this 7-year-old bug, and create a new issue is further refactoring is still warranted.

Thanks everyone for all your work so far on this!

vaplas’s picture

I tried to create an issue about refactoring and move everything I know about this. See #2871619: Refactoring content_type.js.

tstoeckler’s picture

Marked #2871214: Remove a space in the Publishing options vertical tab as duplicate as the patch there did not have test coverage. The fix over there seemed to go in a slightly different direction maybe someone from there can comment here.

droplet’s picture

Patch like #51 is good enough at the moment and really fit into this issue scope.

vaplas’s picture

@droplet: you mean #50 or #52?)

hugovk’s picture

Status: Needs review » Reviewed & tested by the community

Patch like #51 is good enough at the moment and really fit into this issue scope.

There is no patch in #51, but I assume @droplet meant #52 which has been reviewed, so putting this back into RTBC :)

vaplas’s picture

#74 +1.

Re-upload #52, because afaik the RTBC status should apply to the last patch.

Edit: Opps, looks like @droplet really pointed to #52. The fact is that I just incorrectly addd postfix to this patch, hence a small confusion, sorry!

vaplas’s picture

Status: Reviewed & tested by the community » Needs review

Changed the status to run tests. Also all points from @droplet and @lauriii have big sense, but this is really bit out of scope from issue about annoying space. Welcome to the #2871619: Refactoring content_type.js, or we need change title and summary here, so as not to mislead anyone :)

droplet’s picture

Thanks, @hugovk & @vaplas.

the latest patch #75 is fine.

adriancid’s picture

Same patch but with the correct extension.

Status: Needs review » Needs work

The last submitted patch, 78: 2849100-75-test-only.patch, failed testing.

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

random fail.

hugovk’s picture

Looks good:

  • 2849100-75-test-only.patch is meant to fail -- it's a test without a fix.
  • 2849100-75.patch is the test and the fix, and this passes.

The last submitted patch, 78: 2849100-75.patch, failed testing.

The last submitted patch, 78: 2849100-75.patch, failed testing.

The last submitted patch, 78: 2849100-75.patch, failed testing.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

After reconsideration and @droplet pointing out that this can be used to safeguard accessibility, I think it makes sense to target the label element directly.

Thank you for adding the JavaScript test coverage.

Committed e31abcc and pushed to 8.4.x. Thanks!

I wouldn't backport this to 8.3.x because this has a minor change in how the text in summary is built.

  • lauriii committed e31abcc on 8.4.x
    Issue #2849100 by vaplas, hugovk, tameeshb, droplet, NikitaJain,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed
hugovk’s picture

Thanks everyone who helped out, this was my first ever Drupal core bug report!

droplet’s picture

Thanks @hugovk. Do you enjoy it? haha

Now, we able move to the follow-up issue

hugovk’s picture

@droplet Yes, it's great to contribute, but I must say I'm looking forward to the change to something modern like GitHub or GitLab :) Creating/viewing/applying/re-rolling patches is somewhat tedious.

By the way, I think https://www.drupal.org/node/991522 can now be closed or marked as duplicate of this one. I can't edit it, please could someone check it? Thanks!

Status: Fixed » Closed (fixed)

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