Problem/Motivation

Filing this under Claro because it's an uncaught Claro-only bug. But really it's more of a filter.module bug. I'll let others pick the best component.

Steps to reproduce

  1. Drag in a new CKEditor toolbar button such as the one for <s>:
  2. On Seven, this happens:

    ✅
  3. On Claro, this happens:

    ❌

Root cause analysis:

Seven
Claro

Root cause: Claro changes class attribute values in the admin UI, specifically that of form item descriptions.

OTOH, this was probably only an implicit API?

Proposed resolution

Harden the selector in core/modules/filter/filter.filter_html.admin.es6.js.

Remaining tasks

  • Create patch
  • Review patch
  • Test patch
  • Commit

User interface changes

On Claro, you will now see this:

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#12 after_patch6.png103.89 KBsonam.chaturvedi
#12 before patch6.png96.87 KBsonam.chaturvedi
#6 reroll_diff_3-6.txt2.54 KBawset
#6 3216214-6.patch1.62 KBawset
#3 interdiff-3216214-2_3.txt783 bytesGauravvvv
#3 3216214-3.patch1.64 KBGauravvvv
#2 3216214-2.patch1.61 KBWim Leers
#2 Screenshot 2021-05-28 at 12.25.22.png135.05 KBWim Leers
Screenshot 2021-05-28 at 12.11.42.png450.68 KBWim Leers
Screenshot 2021-05-28 at 12.10.43.png405.26 KBWim Leers
Screenshot 2021-05-28 at 12.04.09.png192.21 KBWim Leers
Screenshot 2021-05-28 at 12.04.37.png174.89 KBWim Leers
Screenshot 2021-05-28 at 12.04.00.png14.78 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Status: Active » Needs review
FileSize
Screenshot 2021-05-28 at 12.25.22.png135.05 KB
3216214-2.patch1.61 KB

Ideal/correct solution is impossible without drastic API changes?

I'd say that the best solution here would be to add a class to target. A js-SOMETHING class. That is AFAICT the best practice.

Unfortunately, template_preprocess_form_element() does not allow us to do that:

  $variables['description'] = NULL;
  if (!empty($element['#description'])) {
    $variables['description_display'] = $element['#description_display'];
    $description_attributes = [];
    if (!empty($element['#id'])) {
      $description_attributes['id'] = $element['#id'] . '--description';
    }
    $variables['description']['attributes'] = new Attribute($description_attributes);
    $variables['description']['content'] = $element['#description'];
  }

We have no way to add things to $description_attributes 😔

So pragmatic solution it is?

Which means AFAICT the only way to fix this is to target the id attribute instead.

So … doing that in this patch. Result:

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented
FileSize
3216214-3.patch1.64 KB
interdiff-3216214-2_3.txt783 bytes

Fixed custom command failed. Attached interdiff for same.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks, @Gauravmahlawat!

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

awset’s picture

awset CreditAttribution: awset at Salsa Digital commented
FileSize
3216214-6.patch1.62 KB
reroll_diff_3-6.txt2.54 KB

The patch #3 by @Gauravmahlawat is working in 9.3 brach, but it can't be applied to 9.4 branch.

I re-rolled the patch from #3 to 9.4 branch.

It needs manual review.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Srijan | A Material+ Company for Drupal India Association commented

Status: Needs review » Needs work

The last submitted patch, 6: 3216214-6.patch, failed testing. View results

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Itty Bitty Byte for Salsa Digital commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative

Unrelated error. Back to needs review.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Itty Bitty Byte for Salsa Digital commented
Issue summary: View changes
Issue tags: +Needs manual testing

Confirmed that patch in #6 applies cleanly to 9.3 and 9.4 and with offsets for 10. Updated issue summary with tasks.

[drupal-10.0.x-dev/10.0.x] [drupal-10.0.x-dev]$ patch -p1 < 3216214-6.patch 
patching file core/modules/filter/filter.filter_html.admin.es6.js
patching file core/modules/filter/filter.filter_html.admin.js
Hunk #1 succeeded at 34 with fuzz 2 (offset 2 lines).

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sonam.chaturvedi’s picture

sonam.chaturvedi CreditAttribution: sonam.chaturvedi at Salsa Digital commented
FileSize
before patch6.png96.87 KB
after_patch6.png103.89 KB

Verified and tested successfully.
Patch applied cleanly with two offsets on 9.5.x-dev.

$ git apply -v 3216214-6.patch
Checking patch core/modules/filter/filter.filter_html.admin.es6.js...
Hunk #1 succeeded at 90 (offset 15 lines).
Checking patch core/modules/filter/filter.filter_html.admin.js...
Hunk #1 succeeded at 56 (offset 24 lines).
Applied patch core/modules/filter/filter.filter_html.admin.es6.js cleanly.
Applied patch core/modules/filter/filter.filter_html.admin.js cleanly.

Test Steps:
1. Goto Configuration > Content authoring > Text formats and editors
2. Edit CKEditor and drag in a new CKEditor toolbar button such as the one for
3. Verify "Based on the text editor configuration, these tags have automatically been added: ." is displayed under filter settings.

Test Result:
"Based on the text editor configuration, these tags have automatically been added: ." is displayed under filter settings.

Before patch:
bef patch6

After patch:
after patch

RTBC +1

ameymudras’s picture

ameymudras CreditAttribution: ameymudras at Salsa Digital commented
Status: Needs review » Reviewed & tested by the community

Verified the patch #6 on 9.5.x and following are my observations:

1. The patch applies cleanly
2. The issue summary is clear
3. Was able to reproduce the issue with "steps to reproduce" & the patch fixes the issue

Based on #12 moving this to RTBC

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Itty Bitty Byte for Salsa Digital commented
Issue tags: -Needs manual testing

Thanks for the testing and review!

Removing testing tag.

  • lauriii committed ac69b04 on 10.1.x 
    Issue #3216214 by Wim Leers, Gauravmahlawat, awset, sonam.chaturvedi,...

  • lauriii committed 26fc8eb on 10.0.x 
    Issue #3216214 by Wim Leers, Gauravmahlawat, awset, sonam.chaturvedi,...

  • lauriii committed 790b8a6 on 9.5.x 
    Issue #3216214 by Wim Leers, Gauravmahlawat, awset, sonam.chaturvedi,...

  • lauriii committed 413fa22 on 9.4.x 
    Issue #3216214 by Wim Leers, Gauravmahlawat, awset, sonam.chaturvedi,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 10.1.x and cherry-picked to 10.0.x, 9.5.x, and 9.4.x. Thank you!

Status: Fixed » Closed (fixed)

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