Fragment link pointing to <textarea> should be redirected to CKEditor 5 instance when CKEditor 5 replaced that textarea [#3238257]

Problem/Motivation

See #2729663: Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea. Same problem with CKEditor 5.

Steps to reproduce

Trigger a validation error on a text field using CKEditor 5 while Inline Form Errors is enabled.

Proposed resolution

Implement a solution similar to #2729663: Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea.

When suggested to implement this upstream, the CKEditor developers reacted surprised: they've never seen this requested before. Since it's simple enough to implement and maintain on our side, the above seems fine.

Remaining tasks

~~Port test coverage from \Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testFragmentLink()~~.
~~Port test coverage from \Drupal\Tests\inline_form_errors\FunctionalJavascript\FormErrorHandlerCKEditorTest().~~
~~Add fix.~~

User interface changes

Fragment link navigation pointing to text fields using CKEditor 5 now works.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#18	3238257-18-D9.patch	10.55 KB	Wim Leers
#18	9.4.x: PHP 8.0 & MySQL 5.7 29,347 pass 9.3.x: PHP 8.0 & MySQL 5.7 29,236 pass
#18	3238257-18-D10.patch	10.42 KB	Wim Leers
#18	10.0.x: PHP 8.1 & MySQL 5.7 29,269 pass
#8	interdiff_6-8.txt	2.06 KB	ranjith_kumar_k_u
#8	3238257-8.patch	6.78 KB	ranjith_kumar_k_u
#8	9.4.x: PHP 8.0 & MySQL 5.7 29,300 pass, 1 fail

Issue fork drupal-3238257

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

1 hidden branch

3238257-fragment-link-pointing

changes, plain diff MR !1631

Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

21 September 2021 at 11:12

Wim Leers created an issue. See original summary.

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 12 November 2021 at 14:52

Project:	CKEditor 5	» Drupal core
Version:	1.0.x-dev	» 9.3.x-dev
Component:	Code	» ckeditor5.module
Issue tags:		+Needs tests

Comment #3

12 November 2021 at 14:52

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.

Comment #4

7 January 2022 at 22:41

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

Comment #5

10 January 2022 at 23:29

hooroomoo opened merge request !1631

Comment #6

hooroomoo

they/them

CreditAttribution: hooroomoo at Acquia commented 11 January 2022 at 15:16

Test is currently expected to fail because it is using the wrong $ckeditor_id but not sure why it is failing on the "assertNotVisibleInViewport" assertion on line 104.

Comment #7

hooroomoo

they/them

CreditAttribution: hooroomoo at Acquia commented 11 January 2022 at 19:07

Status:

Active

» Needs review

Works when manually tested but still unsure why line 107 which has the "assertNotVisibleInViewport" assertion in the CKEditor5FragmentLinkTest.php file keeps failing.

Could also use a review before I add another test for this in inline-error form.

Comment #8

ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commented 12 January 2022 at 05:07

File	Size
3238257-8.patch	6.78 KB
9.4.x: PHP 8.0 & MySQL 5.7 29,300 pass, 1 fail
interdiff_6-8.txt	2.06 KB

Fixed CS errors.

Comment #9

12 January 2022 at 06:09

Status:

Needs review

» Needs work

The last submitted patch, 8: 3238257-8.patch, failed testing. View results

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 12 January 2022 at 17:00

@ranjith_kumar_k_u Can you please commit your change to the merge request instead? By uploading a patch, you introduced a parallel path 😅

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 13 January 2022 at 16:22

Issue tags:

-Needs tests

This is definitely on the right track 😊

Let's first try to get tests to pass — I posted a suggestion that I think will fix this.

Then let's try to tackle that "unique URL on every redirect" problem.

Comment #12

hooroomoo

they/them

CreditAttribution: hooroomoo at Acquia commented 13 January 2022 at 19:58

Status:

Needs work

» Needs review

Comment #13

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett as a volunteer and at Acquia commented 13 January 2022 at 21:28

2 files were hidden/shown/deleted

File	Size
3238257-8.patch	6.78 KB
9.4.x: PHP 8.0 & MySQL 5.7 29,300 pass, 1 fail
interdiff_6-8.txt	2.06 KB

Fixing credit

Comment #14

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 14 January 2022 at 13:08

Priority:	Normal	» Major
Issue summary:	View changes
Status:	Needs review	» Needs work
Issue tags:		+Accessibility, +JavaScript

Almost there!

Issue summary updated. After my feedback is addressed, this will be RTBC :) Also bumping this to Major because without this, CKEditor 5 will regress compared to CKEditor 4 when using Inline Form Errors for improved accessibility. Plus, the equivalent CKEditor 4 issue was Major too: #2729663: Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea.

Comment #15

hooroomoo

they/them

CreditAttribution: hooroomoo at Acquia commented 14 January 2022 at 18:10

Status:

Needs work

» Needs review

Comment #16

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 January 2022 at 14:06

Status:

Needs review

» Reviewed & tested by the community

Manually tested one last time; works great!

All my concerns have been addressed.

Thanks, @hooroomoo!

Comment #17

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii at Acquia commented 17 January 2022 at 15:52

Status:

Reviewed & tested by the community

» Needs work

This needs a Drupal 10 patch 😇

Comment #18

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 17 January 2022 at 17:26

Status:

Needs work

» Reviewed & tested by the community

File	Size
3238257-18-D10.patch	10.42 KB
10.0.x: PHP 8.1 & MySQL 5.7 29,269 pass
3238257-18-D9.patch	10.55 KB
9.4.x: PHP 8.0 & MySQL 5.7 29,347 pass 9.3.x: PHP 8.0 & MySQL 5.7 29,236 pass

Uploading both D9 and D10 patches extracted directly from the latest MR (the only difference being that I ran cd core && yarn install && yarn build:js again for the D10 patch), which makes it easy to compare what's different between the two. Basically: var vs const, due to #3253148: Remove IE from core's browserlist, remove non-essential CSS importing and recompile assets.

P.S.: due to #3253148: Remove IE from core's browserlist, remove non-essential CSS importing and recompile assets we're going to have to create D10 patches for any JS change that affects Drupal-owned JS for the CKEditor 5 module 😬

Comment #19

18 January 2022 at 08:32

lauriii committed 241794b on 10.0.x

Issue #3238257 by hooroomoo, Wim Leers: Fragment link pointing to...

Comment #20

18 January 2022 at 08:33

lauriii committed 365cb3a on 9.4.x

Issue #3238257 by hooroomoo, Wim Leers: Fragment link pointing to...

Comment #21

18 January 2022 at 08:34

lauriii committed 6c0fe61 on 9.3.x

Issue #3238257 by hooroomoo, Wim Leers: Fragment link pointing to...

Comment #22

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii at Acquia commented 18 January 2022 at 08:34

Version:	9.4.x-dev	» 9.3.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed 241794b and pushed to 10.0.x. Committed the Drupal 9 patch to 9.4.x and cherry-picked to 9.3.x because CKEditor 5 is still experimental. Thanks!