Problem/Motivation

Follow-up to #3231321: Improve keyboard accessibility in a particular edge case, the form id can be random when there are errors on the form and the form id that is supposed to be #filter-format-edit-form looks like this: id="filter-format-edit-form--caLhw-Q-8ME". It causes a JS error to be triggered in the console when clicking on any vertical-tab title for the plugin config.

Steps to reproduce

  1. Edit a CKE5 text format
  2. Add the Image plugin but do not enable image upload (it will triggers an error on submit).
  3. Try to save the form
  4. Interact with the vertical tab of the cke5 plugin settings
  5. JS error:
    Uncaught TypeError: Cannot read properties of null (reading 'hasAttribute')
    at updateUiStateStorage (ckeditor5.admin.js:404:34)
    at HTMLUListElement.<anonymous> (ckeditor5.admin.js:456:13)

Proposed resolution

Code need to be using form[data-drupal-selector="filter-format-edit-form"] instead of the form ID.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3263601-18.patch4.88 KBwim leers
#18 3263601-18-do-not-test.patch2.25 KBwim leers

Issue fork drupal-3263601

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nod_ created an issue. See original summary.

kostyashupenko made their first commit to this issue’s fork.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

For some reason on this form data-drupal-selector attribute can be also random after submit + when system messages appears after submit. But i believe this attribute should be static no matter what. So for sure it needs to be fixed first and then we can update js.

kostyashupenko opened merge request !1814

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review

Wow, great find!

I think it is probably okay to land this without explicit test coverage. But let's see if others agree with that.

Pinging @nod_ to review this — he's done a lot of work on the "ID to data-drupal-selector" transition :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Related issues: +#2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose

Looks like a duplicate of #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose that we ran into a while ago.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Hm … is it really though?

Isn't the problem that the JS is looking for elements based on id attribute instead of based on data-drupal-selector attribute? 🤔

nod_’s picture

nod_
Primary language French
Location Lille
commented

right not a duplicate per say, related issue is about the backend problem we're seeing that makes the JS patch useless.

While the backend issue is fixed we can use something like [data-drupal-selector^="form-id…"] to make it work. It's contained enough inside the ckeditor5 module that I don't worry we'll run into problems with the selector matching something that it shouldn't. Haven't tested it but it might even be possible to use [data-drupal-selector|="form-id-…"] https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

The only failure here seems unrelated:

There was 1 failure:

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartInstallAndServerCommands
Failed asserting that 'Drupal development server started: http://127.0.0.1:8889>\n
This server is not meant for production use.\n
' contains "127.0.0.1:8889/user/reset/1/".

@nod_ So are you proposing we postpone this on #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose? It's not clear to me from your comment 😅

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

Alright, re-fixed it according to #8

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Nice, thank you.

2 minor points:

  1. could you add a comment explaining why we're doing a prefix match on the selector and add a @todo pointing https://www.drupal.org/i/2897377 so we know when to remove it?
  2. Since the selectors are quite long, I'd put them in a variable (and add the comment above on the variable itself).

After that I'm good to RTBC :)

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

All good, thanks :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Follow-up, fix js error by fixing form id selector » Follow-up, fix JS error by fixing form ID selector
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Does this issue still persist now that #3254328: Update to Drupal 9.3.0 adding --2 suffix to (views-) block-ID's is in?

Yes we shouldn't be using IDs for selection, but do we still need the ^ qualifier?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Yes I can confirm this is still happening with the current 10.x branch.

The issue here is with form ids, not blocks, root cause is in the related issue, the ^ is a band-aid :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Bug Smash Initiative

Is there anyway we can write a test for this?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3263601-18-do-not-test.patch2.25 KB
new 3263601-18.patch4.88 KB

Still a bug, can still reproduce it.

We should definitely be able to write a test for this. Here you go.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Test passed, despite 

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTest.php
@@ -141,6 +141,41 @@ function (ConstraintViolation $v) {
+    $this->failOnJavascriptConsoleErrors = TRUE;

😬

nod_’s picture

nod_
Primary language French
Location Lille
commented

So the original steps to trigger the error is not possible anymore since we added the possibility to use image plugin without upload enabled. In theory this is still an issue but I couldn't find a way to trigger the problem with the core interface anymore.

I still think we should update the code with what's in the MR (needs reroll) even if we don't have a good way to test this.

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.