[regression] Table Drag handles no longer respond to up/down arrow keys

My bad. Totally forget about keyboards testing

Looks like the new JavascriptTestBase could be used here.

Bug Still exists

Keys up/down, right/left still don't work.

Area tested

• Admin Menu

Tested on simplytest.me with & without patch

on Chrome 55

• in drupal 7.43
• in drupal 8.3.x

Patch doesn't make an item in the menu move with the arrow keys
On 7.43 without the patch, I cannot reproduce the key controls working.

It works after patching (in Chrome 58). Ensure you've clean the caches.

Thanks @techmsi for the thorough issue update and testing steps! Adding issue credit.

@techmsi, there was another contributor you looked at this issue during the sprint; do you know their username?

I tested it on my Mac Chrome 55. Still working. If anyone could capture a GIF or VIDEO that will help.

Note: It won't drag on the single parent. For example in D8 Admin menu, you can't drop default "Administration".

Needed more debug info. Thanks ALL.

@catch, @cilefen, @alexpott, @Cottser, @lauriii and I discussed this issue and agreed that it is major priority due to the impact of the accessibility regression. Thanks @techmsi for helping get this triaged!

Reviewing.

Alright. Tested this in Chrome 58 on macOS. This probably still needs JS tests.

Prior to the patch the keyboard interaction behavior is a strange, as expected.

With the patch applied, the keyboard interaction behavior is what I would expect.

Very nice visualization @drpal how did you make it?

We should include it more often.

@mgifford,

I'm not sure what @drpal used. But on MacOS, you can do it as simple as:
http://osxdaily.com/2013/06/21/mac-virtual-keyboard-os-x/

Windows:
https://support.microsoft.com/en-us/help/10762/windows-use-on-screen-key...
(Windows, only click with mouse has hover effect)

Missing a test :)

Thanks @droplet - no idea how I missed that.. Very useful!

#8 no longer applies. This is a straight reroll. It's not correct, since it doesn't include the change to the tabledrag.es6.js file, so if someone can reroll to include that, that would be awesome. Just posting this in the meantime for anyone wanting to do additional manual testing.

- rerolled from #30 to include the changes in the *.es6.js file.

While doing some manual testing I saw that up, down and right works as expected. Left has a little bit of a problem IMHO, it just goes up one level max.

I went to /admin/structure/menu/manage/admin and focused content. I was able to move it into admin/structure/content-types, but once I tried to move it out again the maximum level back as /admin/structure. Is this another level of the same bug basically, or should this be handled in its own issue?

@dawehner Can you record that behavior? It might be a new bug.

@dawehner,

Can you use Mouse to do the same thing? and compare the behavior with D7 also.

I'm trying to reproduce record the behavior described by @dawehner

I'm reviewing this from the DrupalCon.

Here's my recording of the behavior after the patch 31 on 8.4.

@dawehner (#35) I think this isn't a bug maybe. Because if your element is in the second or third level and the left button is used - if it should go one level up then - it could result in a jump of the screen in long lists, which could confuse the user. Also we then have to assume if the element should go over or under it's previous parent element. The way it's now, the user has the control over it by moving the element up - before the parent element - or down - past the parent element and it's children.

Also, if the selected element is the last one in the parent's children, the left button works as expected, because then we can assume it should be sorted after the parent element.

For me it's in a usable state. What are your thoughts about it?

Patch 31 works properly as @fisherman90 said and has a good usability.

Issues

There are still 2 issues, I would really appreciate being fixed in terms of usability.

Issue#1: Showing lines

There is just one more thing that would be amazing when also be done. So currently there are no lines showing the corresponding elements. As this really helps when moving elements, it would be great to always show them:

So currently thelines are just shown when arrow keys are used once:

For me that does not make sense:

1. for someone using the keyboard, at the very first use there is no lines feedback which is bad
2. for someone switching from drag&drop to keyboard drag, it's confusing (currently shown when parent element is clicked)
3. in general the lines would improve usability

Possible solutions:

1. always show the lines
2. show the lines when tab was used
3. show the lines some element has been selected or some class on the DOM is placed or whatever

Issue#2: Visual feedback (exclamation mark) when move was forbidden

The other issue for keyboard using would be to give visual feedback when something went wront like going left was not possible like you see in that picture. Instead of just doing nothing there needs to be at least something to be happening like showing an information.

@kwoxer
You want to apply * for all child elements of cahnged menu item?

Hi @geophysicist no you get me wrong. I updated the pictures. So it's not about the asterisks. It's just about the lines. :)

I will try to apply some notifications when it's not possible to move items left.

We should put the visual feedback into a seperate issue, so that patch can go to RTBC once the accessibility group was looking over it. @geophysicist ok?

Created Issue for the showing the lines in the table before dragging.

Created as well Issue for showing a hint when a keyboard arrow move was not possible

Patch is tested and working in Drupal 8.4 - MacOS Chrome. Safari and Firefox on MacOS won't focus the arrows unless the accessibility settings in the browser are updated. Example: https://stackoverflow.com/a/11713537

assumed unassigned after months of inactivity.

Also, needs accessibility work according to above feedback.

Wonder if it might make sense to think about a solution that works with #2920006: Research accessibility of drag-and-drop grid interfaces. too.

#2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests might be related.

Re-rolling against 8.5.x. Setting needs review just for the bot, the patch still misses tests.

Back to needs work for the tests.

Wrote tests to cover keyboard accessibility.
As @claudiu pointed out in the related issue #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests there are issue dragging items from a lower row to an upper one, so I also added the root - leaf test to cover that part too. It's missing test coverage regarding grouped items though, as that adds a bit extra complexity due to the grouping itself and I ran out of time for now.

Note: I took the test module code from #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests written by @claudiu and adapted it.

Adding the full patch and the interdiff with #58 just to show that there were no additional changes.

Looking into the failures.

Adding a drupalci.yml to try to debug the failures. Also replaced assertions.

This is really strange. Trying to get the browser output again.

Still trying to get the proper drupalci.yml in.

Ops, the latest patch accidentally included a wrong selector. In the meantime I managed to reproduce the issue locally. The CI runs chromedriver with --headless. If I do the same, the test fails. I'll look into it.

This is a patch against #61.

Adding the changes from #66 and #69.

The 8.6.x version of #80.

Ready for RTBC.

I disagree that testing modules *should* necessary work by their own. However, I added a default set of 5 rows to the table. Tests are overriding with their rows.

Reroll and moved the bug in 8.6.x.

@andrewmacpherson good to add at least one test for the nested children. I took it initially out that part because the nesting functionality has many scenarios to cover.
I am actually not happy about the code I wrote in assertTableRow() line 281-283. When the tests are asserting that nothing is changed, those lines will wait pointlessly. We should try to remove them.

Perfect. I only did a reroll so I guess I can RTBC.

@andrewmacpherson yes those lines. They were added when trying to solve the failures on the tests, which were solved by @claudiu.cristea with the change of mink driver. So yes, they are useless.
Thanks for updating the patch!

+++ b/core/misc/tabledrag.es6.js
@@ -558,10 +558,10 @@
+          let $previousRow = $(self.rowObject.element).prev('tr').eq(0);

Looks like you forgot to run prettier.

@andrewmacpherson Thank you

One more thing after re-reading the patch. We should test also if the "You have unsaved changes." message is show on the page after a row drag.

Also...

+++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
@@ -0,0 +1,346 @@
+    $this->getSession()->wait(500);

This not a correct way to wait. We have to wait until some JS condition is met, not an amount of time.

Tagging for badcamp, thanks

Tackling last remark from @claudiu.cristea.
Added a js library that adds an event to the row handle. Due to the order of inclusion of the js libraries, this event will be executed after the ones added by the tabledrag library.
In the php test a css class is added before moving a row, and then the test will wait until said class is removed by the script.
The tabledrag library allows to add custom handlers to be specified on some events, but we need one that gets executed even when no swapping/dragging happens. This is useful for tests when no changes should happen.
The test js library added only takes care of the "keydown" event, meaning that for now it works only when rows are moved through the keyboard, as this is what the test does. Further support can be added when needed.

Missed the remark about the changed warning. Attaching a patch with an extra test.

1. Re #103
   @sardara while this is clever way for test to wait for the changes I think if we can avoid our test depending on extra Javascript on the page we should

   I have changed assertDraggableTable() keep checking the structure of the table for 2 second until correct. This is similar to \Behat\Mink\Element\Element::waitFor() which is used by \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement() and similar methods.

   Since the checks happen every .1 seconds most runs will only wait .1 seconds but the tests are running slow as we see random failures in DrupalCI the test will still pass.

2. #104 looks good except.

   +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -241,6 +241,28 @@
   +    $this->moveRowWithKeyboard($this->findRowById(5), 'down');
   +    $this->assertSession()->pageTextNotContains('You have unsaved changes.');
   

   I think will still need to wait for this text to appear to avoid random test failures on DrupalCI

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,402 @@
   +   * @return \Behat\Mink\Element\NodeElement|mixed|null
   +   *   The row element.
   

   This can't return a null if we assert it is not. Also because we passing it to moveRowWithKeyboard() so it will always be NodeElement

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,402 @@
   +    if (!$row) {
   +      throw new ElementNotFoundException($this->getSession(), 'row');
   +    }
   

   I am not sure why we are throwing exceptions vs just asserting that $row is not empty.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,402 @@
   +    if (!in_array($arrow, ['left', 'right', 'up', 'down'])) {
   +      throw new \InvalidArgumentException('The arrow parameter must be one of "left", "right", "up" or "down".');
   +    }
   

   I think it would simpler just throw an exception if $keys[$arrow] is not set.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,402 @@
   +    if (!$row) {
   +      throw new ElementNotFoundException($this->getSession(), 'row');
   +    }
   ...
   +      throw new \InvalidArgumentException('The arrow parameter must be one of "left", "right", "up" or "down".');
   

   I am not sure why we are throwing exceptions vs just asserting that $row is not empty.

Replying to #105 remarks:

1. Using an arbitrary amount of time is exactly what was asked by @claudiu.cristea to be avoided in #101. This is especially true because we have some parts of the test that verify that some rows cannot be moved. By depending on a real condition (the class being removed by JS) we are sure that the row wasn't moved.
2. Yes it should wait indeed as it happens on blur of the handle, which is after moving the row.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
@@ -0,0 +1,402 @@
+    $this->assertEmpty($assert_session->waitForElement('css', '.messages--warning:contains("You have unsaved changes.")', 1000));

Isn't this subject to random failures on the CI anyway? It could take more than 1 second to show.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
@@ -0,0 +1,402 @@
+    do {

This won't do. We have assertions where we check that rows are not moved under some conditions. This code still doesn't ensure that the table operations are over and it will return true over the first check. In those cases we would be relying on 2 seconds being enough time for JS to finish.

Replying to #106 remarks:

1. Yes indeed the function doc makes no sense, good catch.
2. Why use an assertEmpty() instead of throwing a proper exception of a specific type? I think using ElementNotFoundException is more correct and it's usually thrown in these scenarios. If we want to improve that, we should use \Behat\Mink\WebAssert::elementExists() which returns also the node given the xpath.
3. Agree.
4. I think this is duplicate of the first point.

This is especially true because we have some parts of the test that verify that some rows cannot be moved.

I do see why my way won't work yet for this case.

The problem I mainly see with #104 is that our test covering Javascript code now depends on Javascript code that itself is not tested. If we can find a way to avoid that we should but maybe we can't.

Reviewing #104 locally again it is actually failing randomly for me on \Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest::assertTableRow()
But not on the same line.

For now here is a version of 104 that will run only this test 50 times. To see if these random fails happen on DrupalCI.

I am not sure what was happening locally for me. #108 seems to show no random test errors.

my comment

The problem I mainly see with #104 is that our test covering Javascript code now depends on Javascript code that itself is not tested. If we can find a way to avoid that we should but maybe we can't.

So maybe we can't

Switching back to @sardara's patch at #104 with just a couple of changes.

from my review on #106
1. fixed
2. If you search for "throw new ElementNotFoundException(" in core you there is only 1 instance outside of \Drupal\FunctionalJavascriptTests\JSWebAssert or \Drupal\Tests\WebAssert. If you search for $this->assertNotEmpty( you will find many instances where it is being used to check if expected element is not empty.

I think we should change just to make this more readable alongside other core tests.

fixed
3. fixed

@tedbow which errors are you getting? I only had issues if I touch the browser window while tests are running (as focus gets shifted around).

Regarding your comment

The problem I mainly see with #104 is that our test covering Javascript code now depends on Javascript code that itself is not tested. If we can find a way to avoid that we should but maybe we can't.

This test itself tests the extra JavaScript, because if the class added in the php code is not removed by the JS code the test will fail. So I think we are safe.

Regarding point 2 of #109, it's ok for me too to change it.

By the way, did the test run really 50 times? I see only 3.

1. @tedbow which errors are you getting? I only had issues if I touch the browser window while tests are running (as focus gets shifted around).

   This may have been the problem. I only noticed when the test browser was in the background.

2. By the way, did the test run really 50 times? I see only 3.

   +++ b/core/drupalci.yml
   @@ -3,48 +3,15 @@
   -        testgroups: '--all'
   +        testgroups: '--class "Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest"'
   ...
   +        repeat: 50
   

   Run only this test 50x

   Yeah the report page I don't think counts duplicate runs. this test class has 3 test methods so that is where it comes from. You have to click through to
   View results on dispatcher -> Console Output

   That will get you here: https://dispatcher.drupalci.org/job/drupal_patches/79372/console
   where you can see the test running and passing 50x.

- JavaScript is 👍, these changes are minor and the test coverage appears to be robust.
- @andrewmacpherson, it looks like you've provided a number of reviews on the accessibility side of this patch.
- The two additional feature requests from #43 are part of a series of followups, #2912732: Table Drag always showing lines and #2912737: Table Drag showing visual feedback when move was forbidden

That will get you here: https://dispatcher.drupalci.org/job/drupal_patches/79372/console
where you can see the test running and passing 50x.

Oh yeah makes sense, thanks for the explanation!

Sorry to change a test that is already passing but here are couple test changes that I think make the test more readable and more certain it is not passing just because the structure of the test.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,413 @@
   +    $this->assertDraggableTable([
   +      ['id' => 1, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +      ['id' => 2, 'weight' => -10, 'parent' => 1, 'indentation' => 1, 'changed' => TRUE],
   +      ['id' => 3, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +      ['id' => 4, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +      ['id' => 5, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +    ]);
   

   Only 1 row in the table has changed here.
   But for anyone to really review this test assertion you have to actually confirm not only that row index 1 has changed you also have to confirm that all other rows have not changed.

   We could make this test more understandable by assigning the table structure to $expected_table at the beginning of the method and before every assert updating only the rows in $expected_table that actually should have been changed. This will help make sure the test is not working because of copy/paste error in any case.

   Changed in subsequent calls.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -0,0 +1,413 @@
   +    // Rows marked as root cannot be moved as children of another row.
   +    $this->moveRowWithKeyboard($this->findRowById(5), 'right');
   +    $this->assertDraggableTable([
   +      ['id' => 1, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +      ['id' => 2, 'weight' => 0, 'parent' => 1, 'indentation' => 1, 'changed' => FALSE],
   +      ['id' => 3, 'weight' => 0, 'parent' => 1, 'indentation' => 1, 'changed' => FALSE],
   +      ['id' => 4, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +      ['id' => 5, 'weight' => 0, 'parent' => '', 'indentation' => 0, 'changed' => FALSE],
   +    ]);
   

   Here we are asserting the table structure has not changed. But to review the test and make sure it is not passing because of problem in the test itself you actually have to make sure array passed to assertDraggableTable() is exactly the same as the previous call.

   Again we can fix this by using a variable $expected_table that we know hasn't changed(because it is not passed by reference).

I'm fine with the change even if I fail to see the issue as we are reviewing the patch so it's not hard to verify that the lines are correct.
Anyway let's go this way, but I would like to focus on getting this patch committed as the bug is quite severe, breaking accessibility to tables.

Just adding an interdiff from 109 which is the last it was RTBC. It was just pushed back for test change not for the actual JS fix.

a lot of the diff is the test module that @sardara put in in #103 so that we can confirm that something did NOT move if it should not have. I pushed back on this but I am not convinced we need it as per #109

Two nits and I can RTBC it

1. +++ b/core/modules/system/tests/modules/tabledrag_test/js/tabledrag_test.es6.js
   @@ -0,0 +1,21 @@
   + * Testing behaviours for tabledrag library.
   

   Change "behaviours" to "behaviors" to match Drupal's naming

2. +++ b/core/modules/system/tests/modules/tabledrag_test/js/tabledrag_test.es6.js
   @@ -0,0 +1,21 @@
   +   *   Removes a test class from the handle elements to allow verifying that dragging operatios have been executed.
   

   Typo with "operatios"

Setting back to RTBC because random unrelated fail in MediaStandardProfileTest. I checked and there doesn't to be anything related to a table drag there.

If test fails again it will automatically get kicked back.

@lauriii asked me to look at this issue from a backport perspective.

• The change is all about changing (‘tr:first-of-type’) to (‘tr’).eq(0) - this is a minimal change and is not API
• There is excellent test coverage and the new test fails when run against the old code.
• This is the only usage of the :first-of-type pseudo selector in core.

Therefore for +1 to commit and +1 to backporting to 8.6.x

I ran the new test against 8.6.x locally and it passes.

I think we should be safe backporting this since this only affects keyboard navigation code which prior to this change is completely broken. This is due to the fact that the selectors used before are simply broken. Using :first-of-type in .next() doesn't make sense since it has been designed to match the first of type inside their parent. This selector will simply never match because .next() can only match elements that are not the first element inside the parent element.

Thank you also for adding extensive test coverage for tabledrag. This will significantly improve the maintainability of this component.

Committed 735aaca and pushed to 8.7.x, as well as cherry-picked to 8.6.x. Thanks!

Fixed commit credit

@andrewmacpherson I think requiring tests for keyboard behavior is a good idea. We should open a new issue to add it to the Drupal core gates.