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:
'#suffix' => '<label>Test</label></div>',| Comment | File | Size | Author |
|---|---|---|---|
| #44 | core-multiple_label-1195358-44.patch | 762 bytes | liam morland |
| #6 | text-edit-hide-summary-links.patch | 765 bytes | liam morland |
| #3 | text-edit-hide-summary-links.patch | 782 bytes | liam morland |
| text-edit-hide-summary-links.patch | 762 bytes | liam morland |
Comments
Comment #1
liam morlandThe patch above fixes this issue in both D7 and D8.
Comment #2
liam morlandComment #3
liam morlandUpdated patch rolled against latest D8.
Comment #5
liam morland#3: text-edit-hide-summary-links.patch queued for re-testing.
Comment #6
liam morlandUpdated patch rolled against latest D8.
Comment #7
liam morland#6: text-edit-hide-summary-links.patch queued for re-testing.
Comment #8
liam morland#6: text-edit-hide-summary-links.patch queued for re-testing.
Comment #9
swentel commentedLooks 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 ?
Comment #10
liam morland#6: text-edit-hide-summary-links.patch queued for re-testing.
Comment #12
liam morlandReroll.
Comment #13
liam morlandSorry, this is not the right file.
Comment #14
liam morlandReroll.
Comment #15
swentel commentedMoving to text.module - there might be other text issues in the 'field system' component.
Comment #16
seutje commentedpretty straight-forward
Comment #17
alexpottCommitted 0284fcf and pushed to 8.x. Thanks!
Comment #18
liam morlandComment #19
liam morlandD7 version.
Comment #20
liam morland19: text-edit-hide-summary-links.patch queued for re-testing.
Comment #22
dcam commentedThis 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.
Comment #23
dcam commentedAdded the steps to reproduce to the issue summary.
Comment #26
liam morlandRestoring previous status.
Comment #29
dcam commentedComment #31
liam morlandComment #35
dcam commentedComment #36
droplet commentedIt 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
Comment #37
droplet commentedComment #38
liam morlandThe 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.
Comment #40
droplet commentedwe shouldn't commit a wrong/loose workaround. #36 is a better workaround for this issue.
Comment #41
liam morlandThe first() should still be there to ensure that it only selects one element.
Comment #42
droplet commentedIt 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?)
Comment #43
liam morlandThe 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.
Comment #44
liam morlandThis 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.
Comment #47
liam morlandComment #48
David_Rothstein commentedCommitted 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).
Comment #50
liam morlandThanks 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.