Spaces shown before commas in publishing options

- 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?

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

Looks like RTBC!

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!

- 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

@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

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

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!

@xjm,

Thanks for the update. Would keep that in mind.

@xjm,

Thanks for the update. Would keep that in mind.

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

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)

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.

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.

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…

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.

@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 :)

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

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.

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

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'

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

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

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 :)

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.

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.

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

Thanks, @hugovk & @vaplas.

the latest patch #75 is fine.

Same patch but with the correct extension.

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.

Thanks @hugovk. Do you enjoy it? haha

Now, we able move to the follow-up issue