PopperContent memory leak (#1669)

When a tooltip or popover does not fit in the containing element, this causes the `PopperContent` component to get into an infinite loop of switching between `placement`s in its state. This is because we attempt to update `this.state.placement` everytime popper.js flips the placement to find a better fit and pass it back as the `placement` property to `ReactPopper`.

This is a problem because the `PopperContent` component has two sources of truth for its placement. One coming from its own `placement` property, and one coming from the internal data of `ReactPopper`.

This commit fixes the issue by using `PopperContent`'s `placement` property as the initial placement of the `ReactPopper` component and allowing `ReactPopper` to manage its own updates (i.e. if the popover needs to be flipped because there's not enough room for the specified placement.)

Fixes #1482
Fixes #1488
Fixes #1664

src/PopperContent.js

@@ -51,7 +51,6 @@ class PopperContent extends React.Component {
   constructor(props) {
     super(props);
 
-    this.handlePlacementChange = this.handlePlacementChange.bind(this);
     this.setTargetNode = this.setTargetNode.bind(this);
     this.getTargetNode = this.getTargetNode.bind(this);
     this.getRef = this.getRef.bind(this);
@@ -88,13 +87,6 @@ class PopperContent extends React.Component {
     this._element = ref;
   }
 
-  handlePlacementChange(data) {
-    if (this.state.placement !== data.placement) {
-      this.setState({ placement: data.placement });
-    }
-    return data;
-  }
-
   onClosed() {
     this.props.onClosed();
     this.setState({ isOpen: false });
@@ -120,28 +112,22 @@ class PopperContent extends React.Component {
       onClosed,
       fade,
       transition,
+      placement,
       ...attrs
     } = this.props;
     const arrowClassName = mapToCssModules(classNames(
       'arrow',
       _arrowClassName
     ), cssModule);
-    const placement = this.state.placement || attrs.placement;
-    const placementFirstPart = placement.split('-')[0];
     const popperClassName = mapToCssModules(classNames(
       _popperClassName,
-      placementPrefix ? `${placementPrefix}-${placementFirstPart}` : placementFirstPart
+      placementPrefix ? `${placementPrefix}-auto` : ''
     ), this.props.cssModule);
 
     const extendedModifiers = {
       offset: { offset },
       flip: { enabled: flip, behavior: fallbackPlacement },
       preventOverflow: { boundariesElement },
-      update: {
-        enabled: true,
-        order: 950,
-        fn: this.handlePlacementChange,
-      },
       ...modifiers,
     };
 

src/__tests__/PopperContent.spec.js

@@ -103,10 +103,6 @@ describe('PopperContent', () => {
     expect(wrapper.find(Popper).props().modifiers).toMatchObject({
       // remaining default modifiers
       flip: { enabled: true, behavior: 'flip' },
-      update: {
-        enabled: true,
-        order: 950,
-      },
 
       // additional modifiers
       preventOverflow: { boundariesElement: 'viewport' },
@@ -118,17 +114,19 @@ describe('PopperContent', () => {
     wrapper.unmount();
   });
 
-  it('should have placement class of top by default', () => {
+  it('should have x-placement of auto by default', () => {
     const wrapper = mount(<PopperContent target="target" isOpen container="inline">Yo!</PopperContent>);
 
-    expect(wrapper.find('.auto').exists()).toBe(true);
+    console.log(wrapper.debug());
+
+    expect(wrapper.find('div[x-placement="auto"]').exists()).toBe(true);
   });
 
-  it('should override placement class', () => {
+  it('should override x-placement', () => {
     const wrapper = mount(<PopperContent placement="top" target="target" isOpen container="inline">Yo!</PopperContent>);
 
-    expect(wrapper.find('.auto').exists()).toBe(false);
-    expect(wrapper.find('.top').exists()).toBe(true);
+    expect(wrapper.find('div[x-placement="auto"]').exists()).toBe(false);
+    expect(wrapper.find('div[x-placement="top"]').exists()).toBe(true);
   });
 
   it('should allow for a placement prefix', () => {
@@ -140,53 +138,13 @@ describe('PopperContent', () => {
   it('should allow for a placement prefix with custom placement', () => {
     const wrapper = mount(<PopperContent placementPrefix="dropdown" placement="top" target="target" isOpen container="inline">Yo!</PopperContent>);
 
-    expect(wrapper.find('.dropdown-auto').exists()).toBe(false);
-    expect(wrapper.find('.dropdown-top').exists()).toBe(true);
+    expect(wrapper.find('.dropdown-auto').exists()).toBe(true);
+    expect(wrapper.find('div[x-placement="top"]').exists()).toBe(true);
   });
 
   it('should render custom tag for the popper', () => {
     const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);
 
     expect(wrapper.getDOMNode().tagName.toLowerCase()).toBe('main');
   });
-
-  it('should handle placement changes from popperjs', () => {
-    jest.spyOn(PopperContent.prototype, 'setState');
-    const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);
-
-    const instance = wrapper.instance();
-    const placement = 'top';
-    expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
-    instance.handlePlacementChange({ placement });
-    expect(PopperContent.prototype.setState).toHaveBeenCalled();
-    expect(wrapper.state('placement')).toBe(placement);
-
-    PopperContent.prototype.setState.mockRestore();
-  });
-
-  it('should not update when placement does not change', () => {
-    jest.spyOn(PopperContent.prototype, 'setState');
-    const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);
-
-    const instance = wrapper.instance();
-    const placement = 'top';
-    expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
-    instance.handlePlacementChange({ placement });
-    expect(PopperContent.prototype.setState).toHaveBeenCalled();
-    PopperContent.prototype.setState.mockClear();
-    instance.handlePlacementChange({ placement });
-    expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
-    expect(wrapper.state('placement')).toBe(placement);
-
-    PopperContent.prototype.setState.mockRestore();
-  });
-
-  it('should return data from handle placement changes', () => {
-    const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);
-
-    const instance = wrapper.instance();
-    const data = { placement: 'top' };
-    const result = instance.handlePlacementChange(data);
-    expect(result).toEqual(data);
-  });
 });