Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea

it seems that ckeditor plugin itself (not the drupal module) is setting visibility: hidden; display: none; when it becomes active.

this is preventing hashed links from jumping-to the element as expected.

adding working image to summary

changing component. this is not due to IFE, attention should be focused more directly to ckeditor.

Well… when you use CKEditor, you don't want to see the <textarea>. So… inline_form_errors.module should provide a JS API to allow CKEditor to inform it of how IDs are remapped.

Technically, we could do this using form alters already, but we really can't, because we only want this to happen if the user has JS enabled, because only then will CKEditor be enabled.

i would actually think the issue needs to be files against ckeditor itself, somehow. As it is an actively scripted element then needs to respond to a jumpto link. -- if CK's rich-textarea "'knew to grab original elements id/name attr's" would be nice, and solve the issue.

Upon further investigation I think you're right :)

Patch attached.

I pinged mlewand to review #7 from a CKEditor API perspective.

Manual tested and can confirm both the Bug and the Fix. Waiting for patch to turn green to RTBC.

Attached image.

Test passed. RTBCing

i'll confirm manual testing. also works with second rich-textarea field on same node/add page.

I pinged mlewand to review #7 from a CKEditor API perspective.

I confirm that this is the best way to do scroll to an editor in case of a classic editor. But it may not work with an inline one, since the container will point to the editable element and that element may not have an id.

But if you have to deal with classic editors only, then it will be fine.

We don't have jump links in case of in-place editing, so that is then sufficient confirmation.

Thank you very much!

Now that we have an API for JavaScript Testing in core, is it possible to add a test for this? (See also: https://www.chapterthree.com/blog/javascript-testing-comes-to-drupal-8)

Setting NR for feedback on that.

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,17 @@
+  function redirectTextareaFragmentToCKEditorInstance() {

Should this have a docblock?

added a docblock

reattaching

Wow, you hit Drupal CI at a very unstable time :P :D Sorry you had to go through that!

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,21 @@
+   * Pull the ID from the original textarea, that gets hidden. Specifically for jump-to
+   * links used with Inline Form Errors.

This must fit on one line and within 80 characters.

Most importantly, this still needs a JS test of course.

AWESOME!!!!! EXCITING!!!!!

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +class CKEditorInlineErrorsTest extends JavascriptTestBase {
   

   WOOOOOOOOOOOOT!!!!!!!!!!!!!!!!!!!!!!!

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +    // Create a text format and associate a CKEditor.
   

   s/a CKEditor/CKEditor/

3. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +      'filters' => array(
   ...
   +    EntityFormDisplay::create(array(
   

   Nit: s/array()/[]/

4. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +        'filter_html' => array(
   +          'status' => 1,
   +          'settings' => array(
   +            'allowed_html' => '<h2 id> <h3> <h4> <h5> <h6> <p> <br> <strong> <a href hreflang>',
   +          ),
   +        ),
   

   We don't even need this filter. :)

5. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +      'administer site configuration',
   +      'administer nodes',
   

   Do we really need these two permissions?

6. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,138 @@
   +    // Check that the CKEditor enabled body field is currently not visible in
   ...
   +    $this->assertFalse($session->wait(500, $javascript), 'CKEditor enabled body field is not visible.');
   ...
   +    $this->assertJsCondition($javascript, 1500, 'CKEditor enabled body field is visible: ' . $session->evaluateScript($javascript));
   

   s/CKEditor enabled/CKEditor-enabled/

Looks like there is a bug in the JS test code:

One or more errors were raised in the Javascript code on the page.
            If you don't care about these errors, you can ignore them by
            setting js_errors: false in your Poltergeist configuration (see documentation for details).
TypeError: null is not an object (evaluating 'document.getElementById('edit-title-0-value').style')
TypeError: null is not an object (evaluating 'document.getElementById('edit-title-0-value').style')

@dmsmidt looking good, nice work! I already live-reviewed this at Dev Days, so anybody with more knowledge of the actual issue gets a +1 from me to RTBC this.

Only minor things left, then this is RTBC! Thanks so much! :)

1. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -273,6 +273,23 @@
   +   * Pull the ID from the original textarea, that gets hidden. Specifically for
   +   * jump-to links used with Inline Form Errors.
   

   From #24:

   This must fit on one line and within 80 characters.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,130 @@
   + * Tests the inline errors fragment link to a CKEditor enabled textarea.
   

   Nit: s/CKEditor enabled/CKEditor-enabled/

3. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,130 @@
   +    // Create a body field instance for the page node type.
   

   Nit: s/page/'page'/

4. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,130 @@
   +    // Create a user and login.
   

   Nit: s/login/log in/

   (Or: just remove this — it's quite obvious. The comment is unnecessary.)

5. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
   @@ -0,0 +1,130 @@
   +    // Go to the add node page.
   ...
   +    // Submit the form.
   

   These comments feels unnecessary.

Awesome! This is an awesome contribution — thanks @dmsmidt!

Given your newfound experience with JS tests … would you be interested in writing more of those? :)

Yes, but my primary focus now is keeping inline form errors in core.

Awesome! Let me know if you need reviews in areas that I can review (see MAINTAINERS.txt). Would love to keep you unblocked, this issue was a very productive collaboration with you :)

Given that inline_form_errors is experimental I think this test should be in that module and not ckeditor - less cleanup to do if it is removed.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorInlineErrorsTest.php
@@ -0,0 +1,127 @@
+    // Javascript to test if top of the CKEditor-enabled body field is visible
+    // in the viewport. The NodeElement::isVisible() method doesn't take the
+    // viewport into account.
+    $javascript = <<<JS
+    (function(){
+        var w = window,
+        d = document,
+        e = d.documentElement,
+        el = d.getElementById('cke_edit-body-0-value'),
+        r = el.getBoundingClientRect();
+
+        return (
+          r.top >= 0 &&
+          r.top < (w.innerHeight || e.clientHeight)
+        );
+    }());
+JS;
+
+    // Check that the CKEditor-enabled body field is currently not visible in
+    // the viewport.
+    $this->assertFalse($session->evaluateScript($javascript), 'CKEditor-enabled body field is not visible.');

This looks super useful, and therefore worth generalising to JSWebAssert and making it an assertion.

Given that inline_form_errors is experimental I think this test should be in that module and not ckeditor - less cleanup to do if it is removed.

I agree that the test should be moved there.

However, I think it'd be valuable to have a similar test inside the ckeditor module that does NOT use inline_form_errors. Because this needs to work correctly for use cases other than inline_form_errors too.

#45: oh hah, seems like we are thinking the exact same thing :)

#46 + @alexpott's remark that that looks super useful: closely related: https://github.com/WICG/IntersectionObserver/blob/gh-pages/explainer.md.

I also wrote a helper function to get a screenshot with the viewport drawn on top of it (I may write a patch for Drupal as well to incorporate this helper).

That looks extremely helpful!

s/or it's specific corner/or its specific corner/

Assigning to @dawehner for review since this is adding new JS function testing assertion methods.

Awesome!

Went for a review anyway. Found a bunch of nits.

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -84,42 +84,26 @@
   +    $web_assert = $this->assertSession();
   

   I found this particularly strange, but apparently it's what several other functional JS tests do. So that's consistent then.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +   * Test that a node, or it's specific corner, is visible in the viewport.
   ...
   +   * Test that a node, or it's specific corner, is not visible in the viewport.
   ...
   +   * Check the visibility of a node, or it's specific corner.
   

   s/it's/its/

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +   *   The element selector type (css, xpath).
   

   s/css/CSS/
   s/xpath/XPath/

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +   *   (Optional) The corner to test:
   +   *   topLeft, topRight, bottomRight, bottomLeft.
   

   Can we put this on the same line?

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +   *   Or FALSE to check the complete element (default behavior).
   

   s/behavior//

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +    // Check if the node is visible on the page, which is a perquisite of being
   

   I think you meant prerequisite, although this also exists:

   perquisite |ˈpərkwəzit|
   noun formal
   another term for perk2.
   • a thing regarded as a special right or privilege enjoyed as a result of one's position: the wife of a president has all the perquisites of stardom.
   • historical a thing that has served its primary use and is then given to a subordinate or employee as a customary right.

   :D

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
   @@ -28,4 +32,201 @@ public function assertWaitOnAjaxRequest($timeout = 10000, $message = 'Unable to
   +        ¶
   

   Trailing whitespace.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -0,0 +1,109 @@
+    // Add a bottom margin to the title field to be sure the body field is not
+    // visible. PhantomJS runs with a resolution of 1024x768px.
+    $session->executeScript("document.getElementById('edit-title-0-value').style.marginBottom = '800px';");

It should be avoided to change the testing factor if we can.

#57++

Wonderful, thanks!

Unrelated fails, back to RTBC

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,20 @@
+    var hash = location.hash.substr(1);
...
+        document.location.hash = '#' + id;

Silly JS newbie question, but why document.location in the setter, but only location in the getter?

#65: They're completely equivalent. Do this in your browser console:

> document.location === location
true

They're even identical (i.e. the same exact object), as you can see. Feel free to change either one on commit for consistency.

We have no other code in HEAD that uses document.location. We have a bunch of code that uses bare location and some that uses window.location.

#67 Here is the change to be more consistent with HEAD

Thanks @thpoul!

IMVHO it's a whole new level of nitpicking though. If eslint doesn't catch it, we don't need to care about it IMO.

The nitpick was because I'm not knowledgeable enough in JS to know if location, document.location, and window.location are always the same object, or if there are some conditions that cause them to differ (e.g., in the presence of iframes, etc.).

I manually tested #68, but found a problem with Back button integration. In HEAD, when I click on the fragment link from the inline form error (per issue summary screenshot), it doesn't scroll me to the body field (hence, this issue), but the browser Back button works: clicking it once takes me back to the URL without the fragment, clicking it again takes me to the state of the form prior to the submission that caused the error, clicking it again takes me back to the prior page I was on before this form, etc. With #68, the scroll problem is fixed, but then clicking Back repeatedly keeps me exactly where I am. I'm guessing because the back button keeps retriggering the hashchange event? This is on Chrome.

Let's add test coverage for using the Back button. $session->back() should do the trick per http://mink.behat.org/en/latest/guides/session.html?highlight=back#using....

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,20 @@
+        location.hash = '#' + id;

This may need to use history.replaceState().

EDIT: great find btw, @effulgentsia!

This should fix the back button functionality. I tried extending the test with $session->back(); but for some reason phantomjs hangs when the url has a fragment (tried without a fragment) while $session->reload(); works just fine. Any ideas why?

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,20 @@
+      var editor = CKEDITOR.dom.element.get(element).getEditor();

it can be an error I think. e.g.

CKEDITOR.dom.element.get('#random').getEditor()

** be sure the JS test also catch it.

We knew where the CKEditor has been loaded. Another workaround, we maintain a cached map from CKeditor when it's initiated. (& destroy)

e.g.

drupalSetting.ckeditor.locationHashMapping = ['#textareaIdA', '#textareaIdB', '#textareaIdC'] // may be we have editor IDs stored in somewhere already

function redirectTextareaFragmentToCKEditorInstance() {
   if(CKeditorLocationHashMapping.indexOf('location.hash') !== -1) {
    // CKEDITOR.dom.element.get(element).getEditor()
   // ....etc
   }
}

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -273,6 +273,20 @@
+  $(window).on('hashchange', redirectTextareaFragmentToCKEditorInstance);

to `hashchange.ckeditor`

#73:

1. you still need to make the patch use window.location vs document.location consistently.
2. RE: the PhantomJS problem: https://github.com/ariya/phantomjs/issues/13586 or https://github.com/ariya/phantomjs/issues/11738 could be related. It seems like PhantomJS has had problems with using back() in the past. I propose we don't test the "back" button then.

@dmsmidt,

Many way to trigger hash changes and the URL could contain example.com/#random-non-ckeditor-instances.

hashchange.ckeditor
This is namespaced. Not actually a custom event. let's say I don't like the code above. I can unload this function event handler only without affected all my other `hashchange` event in JS.

#76: I agree :(

#77:

However I managed to test it by going back in the history with JavaScript, jay!

Nice! :)

This looks great. AFAICT all feedback was addressed.

Unrelated fail, see #2828045: Tests failing with unrelated „Translation: The French translation is not available”, waiting for that to go away before RTBCing again

Back to RTBC

Ok, looks like this is solving a major bug, unblocks the inline form errors initiative, and has been reviewed by at least one JavaScript maintainer. Great tests, too! Let's do it! :)

Credit.

Shoot, this patch doesn't apply cleanly to 8.3.x. :( Could we get a re-roll for that branch as well so they can be committed simultaneously?

Re-roll looks great, back to RTBC.

Committed/pushed to 8.2.x, thanks!

YAY!

Great blog post, with illuminating screenshots!