Fix shouldComponentUpdate interfering with droppability (#1164)

* test(mock): ensure we're importing source files

Otherwise we'll import from build/

* fix(droppable): ensure SCU doesn't short-circuit droppable

We can also do a simpler equality check on activeDrag

Added test to ensure we don't regress this

Fixes #1152 and supersedes #1162

__mocks__/react-grid-layout.js

@@ -0,0 +1,2 @@
+// Used by jest as it requires some example files, which pull from 'react-grid-layout'
+module.exports = require('../index-dev');

examples/example-styles.css

@@ -91,8 +91,9 @@ body {
 }
 .droppable-element {
   width: 150px;
-  background: #ddd;
+  text-align: center;
+  background: #fdd;
   border: 1px solid black;
-  margin-top: 10px;
+  margin: 10px 0;
   padding: 10px;
 }

lib/GridItem.jsx

@@ -182,6 +182,7 @@ export default class GridItem extends React.Component<Props, State> {
     // We can't deeply compare children. If the developer memoizes them, we can
     // use this optimization.
     if (this.props.children !== nextProps.children) return true;
+    if (this.props.droppingPosition !== nextProps.droppingPosition) return true;
     // TODO memoize these calculations so they don't take so long?
     const oldPosition = calcGridItemPosition(
       this.getPositionParams(this.props),
@@ -205,6 +206,10 @@ export default class GridItem extends React.Component<Props, State> {
     );
   }
 
+  componentDidMount() {
+    this.moveDroppingItem({});
+  }
+
   componentDidUpdate(prevProps: Props) {
     this.moveDroppingItem(prevProps);
   }
@@ -213,12 +218,13 @@ export default class GridItem extends React.Component<Props, State> {
   // this element by `x, y` pixels.
   moveDroppingItem(prevProps: Props) {
     const { droppingPosition } = this.props;
-    const prevDroppingPosition = prevProps.droppingPosition;
-    const { dragging } = this.state;
+    if (!droppingPosition) return;
 
-    if (!droppingPosition || !prevDroppingPosition) {
-      return;
-    }
+    const prevDroppingPosition = prevProps.droppingPosition || {
+      left: 0,
+      top: 0
+    };
+    const { dragging } = this.state;
 
     if (!this.currentNode) {
       // eslint-disable-next-line react/no-find-dom-node

lib/ReactGridLayout.jsx

@@ -202,7 +202,8 @@ export default class ReactGridLayout extends React.Component<Props, State> {
       // handle changes properly, performance will increase.
       this.props.children !== nextProps.children ||
       !fastRGLPropsEqual(this.props, nextProps, isEqual) ||
-      !isEqual(this.state.activeDrag, nextState.activeDrag)
+      this.state.activeDrag !== nextState.activeDrag ||
+      this.state.droppingPosition !== nextState.droppingPosition
     );
   }
 

test/examples/15-drag-from-outside.jsx

@@ -9,14 +9,13 @@ export default class DragFromOutsideLayout extends React.Component {
     rowHeight: 30,
     onLayoutChange: function() {},
     cols: { lg: 12, md: 10, sm: 6, xs: 4, xxs: 2 },
-    initialLayout: generateLayout()
   };
 
   state = {
     currentBreakpoint: "lg",
     compactType: "vertical",
     mounted: false,
-    layouts: { lg: this.props.initialLayout }
+    layouts: { lg: generateLayout() }
   };
 
   componentDidMount() {
@@ -98,7 +97,7 @@ export default class DragFromOutsideLayout extends React.Component {
           // @see https://bugzilla.mozilla.org/show_bug.cgi?id=568313
           onDragStart={e => e.dataTransfer.setData("text/plain", "")}
         >
-          Droppable Element
+          Droppable Element (Drag me!)
         </div>
         <ResponsiveReactGridLayout
           {...this.props}
@@ -126,7 +125,7 @@ function generateLayout() {
   return _.map(_.range(0, 25), function(item, i) {
     var y = Math.ceil(Math.random() * 4) + 1;
     return {
-      x: (_.random(0, 5) * 2) % 12,
+      x: Math.round(Math.random() * 5) * 2,
       y: Math.floor(i / 6) * y,
       w: 2,
       h: y,

test/spec/lifecycle-test.js

@@ -1,22 +1,23 @@
 // @flow
 /* eslint-env jest */
 
-import React from 'react';
-import _ from 'lodash';
-import ResponsiveReactGridLayout from '../../lib/ResponsiveReactGridLayout';
-import ReactGridLayout from '../../lib/ReactGridLayout';
-import BasicLayout from '../examples/1-basic';
-import ShowcaseLayout from '../examples/0-showcase';
-import deepFreeze from '../util/deepFreeze';
-import {shallow, mount} from 'enzyme';
-
-describe('Lifecycle tests', function() {
+import React from "react";
+import _ from "lodash";
+import TestUtils from "react-dom/test-utils";
+import ResponsiveReactGridLayout from "../../lib/ResponsiveReactGridLayout";
+import ReactGridLayout from "../../lib/ReactGridLayout";
+import BasicLayout from "../examples/1-basic";
+import ShowcaseLayout from "../examples/0-showcase";
+import DroppableLayout from "../examples/15-drag-from-outside";
+import deepFreeze from "../util/deepFreeze";
+import { shallow, mount } from "enzyme";
 
+describe("Lifecycle tests", function() {
   // Example layouts use randomness
   let randIdx = 0;
   beforeAll(() => {
     const randArr = [0.001, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 0.999];
-    jest.spyOn(global.Math, 'random').mockImplementation(() => {
+    jest.spyOn(global.Math, "random").mockImplementation(() => {
       randIdx = (randIdx + 1) % randArr.length;
       return randArr[randIdx];
     });
@@ -30,46 +31,163 @@ describe('Lifecycle tests', function() {
     global.Math.random.mockRestore();
   });
 
-  describe('<ReactGridLayout>', function() {
-    it('Basic Render', async function() {
+  describe("<ReactGridLayout>", function() {
+    it("Basic Render", async function() {
       const wrapper = mount(<BasicLayout />);
       expect(wrapper).toMatchSnapshot();
     });
-  });
 
-  describe('<ResponsiveReactGridLayout>', function() {
+    describe("Droppability", function() {
+      it("Updates when an item is dropped in", function() {
+        const wrapper = mount(<DroppableLayout />);
+        const gridLayout = wrapper.find("ReactGridLayout");
+        expect(gridLayout).toHaveLength(1);
+
+        // Start: no dropping node.
+        expect(gridLayout.state("droppingDOMNode")).toEqual(null);
+
+        // Find the droppable element and drag it over the grid layout.
+        const droppable = wrapper.find(".droppable-element");
+        TestUtils.Simulate.dragOver(gridLayout.getDOMNode(), {
+          nativeEvent: {
+            target: droppable.getDOMNode(),
+            layerX: 200,
+            layerY: 150
+          }
+        });
+
+        // We should have the position in our state.
+        expect(gridLayout.state("droppingPosition")).toHaveProperty(
+          "left",
+          200
+        );
+        expect(gridLayout.state("droppingPosition")).toHaveProperty("top", 150);
+        // We should now have the placeholder element in our state.
+        expect(gridLayout.state("droppingDOMNode")).toHaveProperty(
+          "type",
+          "div"
+        );
+        expect(gridLayout.state("droppingDOMNode")).toHaveProperty(
+          "key",
+          "__dropping-elem__"
+        );
+
+        // It should also have a layout item assigned to it.
+        let layoutItem = gridLayout
+          .state("layout")
+          .find(item => item.i === "__dropping-elem__");
+        expect(layoutItem).toEqual({
+          i: "__dropping-elem__",
+          h: 1,
+          w: 1,
+          x: 1,
+          y: 4,
+          static: false,
+          isDraggable: true
+        });
+
+        // Let's move it some more.
+        TestUtils.Simulate.dragOver(gridLayout.getDOMNode(), {
+          nativeEvent: {
+            target: droppable.getDOMNode(),
+            layerX: 0,
+            layerY: 300
+          }
+        });
 
-    it('Basic Render', async function() {
+        // State should change.
+        expect(gridLayout.state("droppingPosition")).toHaveProperty("left", 0);
+        expect(gridLayout.state("droppingPosition")).toHaveProperty("top", 300);
+
+        layoutItem = gridLayout
+          .state("layout")
+          .find(item => item.i === "__dropping-elem__");
+        // Using toMatchObject() here as this will inherit some undefined properties from the cloning
+        expect(layoutItem).toMatchObject({
+          i: "__dropping-elem__",
+          h: 1,
+          w: 1,
+          x: 0,
+          y: 9,
+          static: false,
+          isDraggable: true
+        });
+      });
+    });
+  });
+
+  describe("<ResponsiveReactGridLayout>", function() {
+    it("Basic Render", async function() {
       const wrapper = mount(<ShowcaseLayout />);
       expect(wrapper).toMatchSnapshot();
     });
 
-    it('Does not modify layout on movement', async function() {
+    it("Does not modify layout on movement", async function() {
       const layouts = {
         lg: [
-          ..._.times(3, (i) => ({
+          ..._.times(3, i => ({
             i: String(i),
             x: i,
             y: 0,
             w: 1,
-            h: 1,
+            h: 1
           }))
         ]
       };
-      const frozenLayouts = deepFreeze(layouts, {set: true, get: false /* don't crash on unknown gets */});
+      const frozenLayouts = deepFreeze(layouts, {
+        set: true,
+        get: false /* don't crash on unknown gets */
+      });
       // Render the basic Responsive layout.
       const wrapper = mount(
-        <ResponsiveReactGridLayout layouts={frozenLayouts} width={1280} breakpoint="lg">
-          {_.times(3, (i) => <div key={i} />)}
+        <ResponsiveReactGridLayout
+          layouts={frozenLayouts}
+          width={1280}
+          breakpoint="lg"
+        >
+          {_.times(3, i => (
+            <div key={i} />
+          ))}
         </ResponsiveReactGridLayout>
       );
 
       // Set that layout as state and ensure it doesn't change.
-      wrapper.setState({layouts: frozenLayouts});
-      wrapper.setProps({width: 800, breakpoint: 'md'}); // will generate new layout
+      wrapper.setState({ layouts: frozenLayouts });
+      wrapper.setProps({ width: 800, breakpoint: "md" }); // will generate new layout
       wrapper.render();
 
-      expect(frozenLayouts).not.toContain('md');
+      expect(frozenLayouts).not.toContain("md");
     });
   });
-});
+});
+
+function simulateMovementFromTo(node, fromX, fromY, toX, toY) {
+  TestUtils.Simulate.mouseDown(node, { clientX: fromX, clientY: fromX });
+  mouseMove(node, toX, toY);
+  TestUtils.Simulate.mouseUp(node);
+}
+
+function mouseMove(node, x, y) {
+  const doc = node ? node.ownerDocument : document;
+  const evt = doc.createEvent("MouseEvents");
+  // $FlowIgnore get with it, flow
+  evt.initMouseEvent(
+    "mousemove",
+    true,
+    true,
+    window,
+    0,
+    0,
+    0,
+    x,
+    y,
+    false,
+    false,
+    false,
+    false,
+    0,
+    null
+  );
+  doc.dispatchEvent(evt);
+  return evt;
+}

test/util/setupTests.js

@@ -14,3 +14,10 @@ Array.prototype.sort = function(comparator) {
   sort(this, comparator);
   return this;
 };
+
+// Required in drag code, not working in JSDOM
+Object.defineProperty(HTMLElement.prototype, "offsetParent", {
+  get() {
+    return this.parentNode;
+  }
+});