Cannot swap tabledrag rows by dragging the lower over the upper row in tests

We are experiencing the same thing when testing table drag using Behat. The symptoms are exactly the same, we can drag from up to down, but not from down to up. Behat also uses Mink to interface with the browser.

I also came up against this in #3089151: TableDrag JS :first-of-type issues. If I understand correctly, when being tested with Mink's dragTo, the JS dragRow() is being called once per entire drag action (ie if you drag row 2 over row 1, that only triggers a single call to it). I think this unusual situation causes a bug to manifest where oldY is always initially 0, which means the direction is always down, which means the row ends up one row beneath where it should be when dragging upwards.

I've rerolled the test against latest and removed the initial set of weight assertions from testDragAndDrop() because all the rows start off with a zero weight (but I'm not sure if that's ideal: should we assert the zero weight, or make them have ascending weights?). It looks like a fix might be initialising oldY in dragStart().

Thanks!

Thanks for the review!

I accidentally left the wait in from debugging the test. I didn't actually write any of the test (apart from that wait!), it's just a reroll of the original patch: I wanted to verify the fix got the existing test passing.

Addresses #20 & #21, thanks!

If the message describes the the incorrect order then we should use $strings, not $items

Oh yes, well spotted, fixed! But having done that, I wonder if it makes sense to explicitly display the values at all: here's the output I got when I forced an error with the new patch:

Found strings incorrectly ordered as: 'Row with id 1', 'Row with id 2', 'Row with id 3'
Failed asserting that Array &0 (
    0 => 'Row with id 1'
    1 => 'Row with id 2'
    2 => 'Row with id 3'
) is identical to Array &0 (
    0 => 'Row with id 1'
    1 => 'Row with id 3'
    2 => 'Row with id 2'
).

Would it be better for the message just to be "Found strings incorrectly ordered."?

I also made some small changes to bring the tests more in line with what's already there. I chose not to use \Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest::assertDraggableTable() because I thought it might be more functional (and less testing implementation) to just verify the order and not the actual weight values, dunno what others might think.

Thanks!

"Strings found on the page but incorrectly ordered." ?

👍 Much better, thanks.

We can use here:
$page->pressButton(...);

Done.

Yup, dead code removed, thanks!

1. The issue title needs updating

2. +++ b/core/misc/tabledrag.es6.js
   @@ -760,6 +760,10 @@
   +    // Set the initial y coordinate so the direction can be calculated in
   +    // dragRow().
   +    self.oldY = self.pointerCoords(event).y;
   

   Given that there are runtime changes have we worked out why this bug is not triggered when testing manually.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -44,6 +45,88 @@ protected function setUp() {
      /**
   +   * Tests row weight switch.
   +   */
   +  public function testRowWeightSwitch() {
   

   This is additional unrelated testing as far as I can see? Is there any reason this had to be added here other than for improved test coverage?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/TableDrag/TableDragTest.php
   @@ -200,6 +283,29 @@ public function testTableDragChangedWarning() {
   +  protected function assertOrder(array $items) {
   +    $session = $this->getSession();
   +    $text = $session->getPage()->getHtml();
   +    $strings = [];
   +    foreach ($items as $item) {
   +      if (($pos = strpos($text, $item)) === FALSE) {
   +        throw new ExpectationException("Cannot find '$item' in the page", $session->getDriver());
   +      }
   +      $strings[$pos] = $item;
   +    }
   +    ksort($strings);
   +    $this->assertSame($items, array_values($strings), "Strings found on the page but incorrectly ordered.");
   +  }
   

   This is a nice implementation of this. I think we should consider adding this to \Drupal\Tests\WebAssert so we can do $this->assertSession()->someFunctionNameThatIHaveNotWorkedOutYet(array) in a follow-up.

   One thing that is interesting is should this work against the raw HTML or against the page text. I think it should probably be against the the text rather than the raw HTML. Not sure though. If we do change it then the exception should be ResponseTextException.

Given that there are runtime changes have we worked out why this bug is not triggered when testing manually.

My understanding of what's going on here is that normally the dragRow() handler is called multiple times when being dragged manually:

    $(document).on('touchmove', function (event) {
      return self.dragRow(event.originalEvent.touches[0], self);
    });
    ...
    $(document).on('mousemove pointermove', function (event) {
      return self.dragRow(event, self);
    });

So while it will always think that initially you're moving the row down, that doesn't really matter because there will be another call to dragRow() at which point oldY will be valid and it can correctly work out the direction. It's Mink that creates the unusual behaviour that there's only a single event for the drag, and the bug manifests.

Yep, @claudiu.cristea adding a @todo in this issue and adding a task to search for @todo's to the other issue is a good plan

+1 to keeping the additional test coverage I just wanted to check that I understood.

Thanks @alexpott. I've added a todo to \Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest::assertOrder() and added a task to #2817657: Add methods to assert that a sequence of strings appears on the page in a given order.

It looks like the same issue might affect claro/js/tabledrag.es6.js, should that be a separate issue?

@AndyF great catch. We should definitely fix it here too.

I've had a go at that - turns out that Claro uses a link rather than a button for the Show row weights toggle. I've added a test method ::findWeightsToggle() so it can be overridden in the Claro test, but it did occur to me that we could instead use a looser xpath in the base test, perhaps one of these (untested):

$page->find('xpath', '//button[text()="Show row weights"] | //a[text()="Show row weights"]')->click();
$page->find('xpath', '//*[text()="Show row weights"]')->click();

I erred on the side of caution and currently the base test explicitly tests it's a button and the Claro test tests it's a link. Feedback welcome (:

I also took the opportunity to change the default value of oldY to null rather than 0, I think that makes sense as it should get explicitly set in dragStart() before use.

Thanks!

Thanks @claudiu.cristea

1. Done.
2. Yup, that works, thanks! I stole the lines I was using before from \Drupal\FunctionalJavascriptTests\Theme\ClaroMenuUiJavascriptTest, looking more closely 3 of the existing 4 Claro functional javascript tests do it that way, and \Drupal\FunctionalJavascriptTests\Theme\ClaroViewsUiTest uses $defaultTheme, not sure if there's any meaning to be read into that?

Committed and pushed 5b9cb9b7fb to 9.0.x and 912971406c to 8.9.x. Thanks!

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php
index 922c6b5b3c..c1e4fbe641 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Theme/ClaroTableDragTest.php
@@ -11,7 +11,7 @@
  *
  * @see \Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest
  */
-class ClaroTableDragTest extends TableDragTest  {
+class ClaroTableDragTest extends TableDragTest {
 
   /**
    * {@inheritdoc}

Fixed coding standards on commit.

I think the bugfix here is not disruptive enough to consider for backport to 8.8.x and fixing JS testing for tabledrag interactions seems like a good thing to do.

Will ask other committers once 8.8.x is open again.

Discussed with @lauriii and we agreed that this was not affecting live sites so not worth backporting.