Problem/Motivation

Clicking on a link in the inline form errors summary error on top of a form scrolls a linked problematic field into view (like an anchor).
This however doesn't work for CKeditor enabled fields, because the original field is hidden.

This is a major issue since we have non-functional links.

Proposed resolution

Because CKEditor is only enabled when JavaScript is available, we can use a JavaScript event listener to redirect the hash change.

Remaining tasks

None

User interface changes

None, it just works as expected.

API changes

None

Data model changes

None

Files: 

Comments

dmsmidt created an issue. See original summary.

SKAUGHT’s picture

FileSize
65.06 KB

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.

SKAUGHT’s picture

Issue summary: View changes

adding working image to summary

SKAUGHT’s picture

Component: inline_form_errors.module » ckeditor.module

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

Wim Leers’s picture

Title: CKeditor enabled fields should scroll into view after clicking link to the erroneous field » CKEditor-enabled fields should scroll into view after clicking link to the erroneous field
Component: ckeditor.module » inline_form_errors.module

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.

SKAUGHT’s picture

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.

Wim Leers’s picture

Version: 8.2.x-dev » 8.1.x-dev
Component: inline_form_errors.module » ckeditor.module
Priority: Major » Normal
Status: Active » Needs review
FileSize
898 bytes

Upon further investigation I think you're right :)

Patch attached.

Wim Leers’s picture

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

Wim Leers’s picture

Title: CKEditor-enabled fields should scroll into view after clicking link to the erroneous field » Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea
thpoul’s picture

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

Test body error

thpoul’s picture

FileSize
126.62 KB

Attached image.

thpoul’s picture

Status: Needs review » Reviewed & tested by the community

Test passed. RTBCing

SKAUGHT’s picture

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

Reinmar’s picture

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.

Wim Leers’s picture

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

Thank you very much!

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

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?

SKAUGHT’s picture

Status: Needs review » Needs work

SKAUGHT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

Wim Leers’s picture

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
Issue tags: +DevDaysMilan
dmsmidt’s picture

I found an additional problem with this patch:

It creates a problematic loop. The event callback changes the hash and thus practically is call it self again. Resulting in:Uncaught TypeError: Cannot read property 'container' of null(…)

I fixed it and also the max line length thingy.

I'll work on JS test these coming days.

P.S. @Wim Leers, why did you use "window.addEventListener" when everywhere else in the file we use $(window).on()?

Status: Needs review » Needs work
dmsmidt’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Retry with shorter name.

dmsmidt’s picture

Ok, one() wasn't a good idea since we want to keep the link working for ever.
The JS error is fixed anyhow by checking if we can find an CKEditor instance.

dmsmidt’s picture

And here is the complete thing, with functional javascript test.
Test only patch should fail.

The last submitted patch, 30: 2729663-30-fragment-link-ckeditor-TEST_ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: 2729663-30-fragment-link-ckeditor.patch, failed testing.

Wim Leers’s picture

Issue tags: -Needs tests

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’s picture

Thanks @Wim, fixed all. Although, 'administer nodes' is still needed.

Also removed the wait(), wasn't needed here and only slowed down the test.

The last submitted patch, 34: 2729663-34-fragment-link-ckeditor_TEST_ONLY.patch, failed testing.

Lendude’s picture

Issue summary: View changes

@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.

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Needs work

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.

dmsmidt’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
2.58 KB

Cleanup done!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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

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

Wim Leers’s picture

Issue tags: +JavaScript
dmsmidt’s picture

Thanks you for the quick and elaborative reviews, and of course also a big thanks to @Lendude.

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.

Wim Leers’s picture

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 :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

dmsmidt’s picture

I can move the test, that is the easiest, and should because it depends on an experimental module.
But, the generic case still should be tested: "there is a hash link to a hidden textarea (because of CKEditor), and this link should be redirected to the correct element".
So I can also rewrite the test to not depend on "Inline Form Errors".

Feedback?

dmsmidt’s picture

Offtopic:

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

Currently we just check if the top of the element is in the window vertically. We could make a more generic assertion with some options.
For example it would allows you to assert visibility of [top|right|bottom|left|all] of the element.

Wim Leers’s picture

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

Ok I'll make two tests :-)

dmsmidt’s picture

Status: Needs work » Needs review
FileSize
9.27 KB

New patch with two tests for CKEditor and inline_form_errors, instead of one in CKEditor which depends on inline_form_errors.

There is overlap between the tests, but since inline_form_errors is experimental this should be ok.
Once inline_form_errors is integrated we can remove some of the duplication, and in case it is removed from core there is no problemo. But let's decide that we don't remove it ;-)

I didn't provide an interdiff because you will go bonkers.
Just note that I renamed $javascript to $visibility_check_javascript for readability.


Oh, and I moved the created user in the CKEditor test to a protected property and gave the class a rather generic name (CKEditorIntegrationTest) so that we can easily add some more tests with the same setUp().

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
dmsmidt’s picture

Woot, here is the test with two additional JSWebAsserts for visibility in the viewport.

You can assert the visibility (or not visibility) of any corner of the element, or the complete element (default).

Note that the assertion messages are formulated the other way around compared to simpletests. It seems that in phpunit the message says what went wrong, instead of what should have happened.

If you like to manually test for example if the new assertions works in a different cases than the given CKEditor test replace:

// Check that the CKEditor-enabled body field is visible in the viewport.
$web_assert->assertVisibleInViewport('css', $ckeditor_id, 'topLeft', 'CKEditor-enabled body field is not visible.');

by

$session->executeScript("document.getElementById('cke_edit-body-0-value').style.height = '80000px';");
$web_assert->assertVisibleInViewport('css', $ckeditor_id, FALSE, 'CKEditor-enabled body field is not visible.');

This will check if the complete node is in view, but also makes the node faaaar to large to be completely in view.

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).

Page before triggering the hash change:
Before

Page after trittering the hash change:
After

Wim Leers’s picture

Assigned: Unassigned » dawehner

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.

dmsmidt’s picture

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).

Did it :-) It's online with some examples: #2755873: Add ability to show the actual viewport on screenshots in JavascriptTestBase.

Wim Leers’s picture

Awesome!

Wim Leers’s picture

Status: Needs review » Needs work

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.

droplet’s picture

+++ 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.

dmsmidt’s picture

@droplet, what do you mean?

Without that margin the CKEditor is in view without changing the hash.
I could replace it by changing the window size for the test, is that preferable?
However then you are testing with an exotic screensize / viewport size.

Wim Leers’s picture

#57++

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Wonderful, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 2729663-59-fragment-link-ckeditor.patch, failed testing.

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated fails, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 2729663-59-fragment-link-ckeditor.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

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.

thpoul’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

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.

Wim Leers’s picture

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!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

thpoul’s picture

Status: Needs work » Needs review
FileSize
461 bytes
16.03 KB

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?

droplet’s picture

+++ 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`

Wim Leers’s picture

Status: Needs review » Needs work

#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’s picture

Assigned: dawehner » dmsmidt

This takes tooo long ;-) Working on it again today.

dmsmidt’s picture

Alright, here we go.

#74, sorry I don't understand exactly what you mean. But your last point is invalid, we want to react on a generic hashchange, we don't define our own hashchange.ckeditor event.

#75:

  1. window.location vs document.location is consistent, double checked.
  2. I can confirm that the back() function is broken after hash changes. However I managed to test it by going back in the history with JavaScript, jay!

Furthermore I removed some code duplication in FormErrorHandlerCKEditorTest::testFragmentLink().

droplet’s picture

@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.

dmsmidt’s picture

@droplet, I see, ok lets name space it then, no problemo.

dmsmidt’s picture

Priority: Normal » Major

Upping the priority of this because it is a blocker for Inline Form Errors, with is under consideration for removal from core.
See: #2504847-57: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2729663-79-ckeditor_fragment_link_redirect.patch, failed testing.

Lendude’s picture

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

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2729663-79-ckeditor_fragment_link_redirect.patch, failed testing.

dmsmidt’s picture

Status: Needs work » Reviewed & tested by the community

Again, unrelated fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2729663-79-ckeditor_fragment_link_redirect.patch, failed testing.

The last submitted patch, 79: 2729663-79-ckeditor_fragment_link_redirect.patch, failed testing.

dmsmidt’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 79: 2729663-79-ckeditor_fragment_link_redirect.patch, failed testing.

dmsmidt’s picture

Status: Needs work » Reviewed & tested by the community

4th time, unrelated fail.

webchick’s picture

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.

webchick’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work

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?

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review
FileSize
16.08 KB

Re-roll against 8.3.x

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll looks great, back to RTBC.

  • catch committed 926f9dc on 8.3.x
    Issue #2729663 by dmsmidt, thpoul, SKAUGHT, Wim Leers, Lendude, droplet...

  • catch committed fa8939c on 8.2.x
    Issue #2729663 by dmsmidt, thpoul, SKAUGHT, Wim Leers, Lendude, droplet...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

Wim Leers’s picture

YAY!

dmsmidt’s picture

Nice, thanks all. Next!

dmsmidt’s picture

For the interested, I explained about the new JS assertions added in this patch here: https://www.triquanta.nl/blog/drupal-8s-functional-javascript-testing-ju...

Wim Leers’s picture

Great blog post, with illuminating screenshots!

Status: Fixed » Closed (fixed)

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