If a field which can be split into body and summary has multiple label elements (added by a module for whatever reason), additional "Edit/Hide summary" links appear and more appear when these links are clicked. The solution is a tiny change to the jQuery selector to ensure that it only adds the link to one label element.

Steps to reproduce:

  1. Edit text.module. Change line 530 to '#suffix' => '<label>Test</label></div>',
  2. Visit the Article node add form. Verify that there are now two show summary links.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Liam Morland’s picture

The patch above fixes this issue in both D7 and D8.

Liam Morland’s picture

Status: Active » Needs review
Liam Morland’s picture

Version: 7.x-dev » 8.x-dev
FileSize
782 bytes

Updated patch rolled against latest D8.

Status: Needs review » Needs work

The last submitted patch, text-edit-hide-summary-links.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review

#3: text-edit-hide-summary-links.patch queued for re-testing.

Liam Morland’s picture

Updated patch rolled against latest D8.

Liam Morland’s picture

#6: text-edit-hide-summary-links.patch queued for re-testing.

Liam Morland’s picture

#6: text-edit-hide-summary-links.patch queued for re-testing.

swentel’s picture

Issue tags: +JavaScript

Looks good to me, doesn't seem to break anything either. Tagging with Javascript. Seutje or nod_ (or anyone else, could you guys RTBC this then ?

Liam Morland’s picture

Issue tags: -JavaScript

#6: text-edit-hide-summary-links.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript

The last submitted patch, text-edit-hide-summary-links.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
709 bytes

Reroll.

Liam Morland’s picture

Sorry, this is not the right file.

Liam Morland’s picture

Reroll.

swentel’s picture

Component: field system » text.module

Moving to text.module - there might be other text issues in the 'field system' component.

seutje’s picture

Status: Needs review » Reviewed & tested by the community

pretty straight-forward

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0284fcf and pushed to 8.x. Thanks!

Liam Morland’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Liam Morland’s picture

Status: Patch (to be ported) » Needs review
FileSize
762 bytes

D7 version.

Liam Morland’s picture

dcam’s picture

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

This looks like the same basic change that went into 8.x. #19 also works for 7.x. I tested by adding a dummy label element to the summary element suffix in line 530 of text.module:
'#suffix' => '<label>Test</label></div>',
Before applying the patch, show/hide links were added to both labels. Afterward, a link was only added to the first label.

dcam’s picture

Issue summary: View changes

Added the steps to reproduce to the issue summary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: text-edit-hide-summary-links.patch, failed testing.

Status: Needs work » Needs review
Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community

Restoring previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: text-edit-hide-summary-links.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: text-edit-hide-summary-links.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: text-edit-hide-summary-links.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community
droplet’s picture

Version: 7.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
861 bytes

It isn't so Drupal pattern way. but if we do support it, it should be more strictly way.

(and above patches assumed append code to #suffix only, Drupal also has #prefix in form API]

back to D8

droplet’s picture

Version: 8.1.x-dev » 8.0.x-dev
Liam Morland’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

The patch has already been committed to D8. We should get the backport of that patch in #19 committed to D7 before anything else is done.

The patch in #36 appears to be undoing what this issue is about.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 1195358-text-js.patch, failed testing.

droplet’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs work » Needs review

we shouldn't commit a wrong/loose workaround. #36 is a better workaround for this issue.

Liam Morland’s picture

The first() should still be there to ensure that it only selects one element.

droplet’s picture

It should not assume any of one the random label is selected. #36 is limited the target scope. #prefix & #suffix adding the HTML code outside of $widget.find('.text-summary-wrapper [class*="form-item-body-"]'). So that still ONE label is selected.

More ideally, we have to match the ID of each Label but I think it's too much.

(Or if you can provide a patch to describe the bug?)

Liam Morland’s picture

The bug happens anytime the selector returns more than one label element. The first() call is a simple and reliable way to make certain that this never happens.

Liam Morland’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
762 bytes

This issue is currently just about backporting the fix in #17 to Drupal 7. This backport is RTBC since #22 and is just waiting on being committed. Any further changes to this should be done in a follow-up issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: core-multiple_label-1195358-44.patch, failed testing.

Status: Needs work » Needs review
Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to 7.x - thanks!

Tough call, because I definitely understand what @droplet is saying. If your other label appears before the real one (for example if you put it in #prefix) then this patch really doesn't work right. The "hide summary" link is attached to the wrong label.

However, after trying it out a bit I convinced myself that that's still better than the alternative of multiple labels appearing/disappearing all over the place, so I committed this as an interim improvement.

I think this could use a followup to fix this in a more robust way, so back to Drupal 8 for that for now (or could be moved to a separate issue instead).

  • David_Rothstein committed 355446d on 7.x
    Issue #1195358 by Liam Morland, droplet: Fixed Multiple "Edit/Hide...
Liam Morland’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Fixed

Thanks very much, @David_Rothstein.

I really think any follow-up should be a child issue. This issue is already at 50 comments, most of which don't actually apply to the next step for this issue. This issue is about an interim improvement and the child issue would be about improving it further.

Status: Fixed » Closed (fixed)

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