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

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new Screenshot 2021-05-28 at 12.25.22.png135.05 KB
new 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 commented
StatusFileSize
new 3216214-3.patch1.64 KB
new interdiff-3216214-2_3.txt783 bytes

Fixed custom command failed. Attached interdiff for same.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
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 commented
StatusFileSize
new 3216214-6.patch1.62 KB
new 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 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
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
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 commented
StatusFileSize
new before patch6.png96.87 KB
new 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 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
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
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.