[change] Class names being added to body during render of non-opened modals #436

fuller · 2017-06-23T16:27:37Z

Changes proposed:

Modal reference tracking

There is an issue where class names are being added to document.body during render of non-opened modals. With only one type of bodyOpenClassName it was "working", but if you added a custom bodyOpenClassName it was causing that value to be added regardless of isOpen status.

See the tests to replicate the behavior against current codebase.

This PR creates a new data structure for the modal references. Instead of an array of modals, we need to know which open modals are associated with each bodyOpenClassName.

Proposed structure is:

modals = {
    "bodyClassName1": [ modal1Ref, modal3Ref ],
    "bodyClassName2": [ modal2Ref ],
    "bodyClassName3": [ modal4Ref ],
}

Additionally fixed an issue where aria-hidden was being removed from the appElement before all open modals were actually closed.
Also refactored some of the demo app so I could manually see this functionality working

I'm not a huge fan of the side-effects when manipulating the modals object, but it's consistent with the current code, and it works 💅

Acceptance Checklist:

All commits have been squashed to one.
The commit message follows the guidelines in CONTRIBUTING.md.
Documentation (README.md) and examples have been updated as needed.
If this is a code change, a spec testing the functionality has been added.
If the commit message has [changed] or [removed], there is an upgrade path above.

coveralls · 2017-06-23T17:13:21Z

Coverage increased (+0.6%) to 85.593% when pulling 8f30171 on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

diasbruno · 2017-06-23T19:50:12Z

@ajfuller This PR does a lot of stuff, maybe we can separate the fix from this new structure. It's better to integrate small PRs.

diasbruno · 2017-06-23T19:58:02Z

In order to avoid adding too much complexity on react-modal, I think we need to add some rules to use this property since it can be really problematic.

fuller · 2017-06-23T20:33:49Z

@diasbruno I can remove the examples/basic/app.js changes for now. I left them in because the changes helped validate the broken scenarios were actually working now.

Are there any specific changes you're concerned about? The majority of changes are consolidating the instance tracking and associated tests. I'm more than happy to change the implementation, but it was the only way I could find to make it pass the tests.

diasbruno · 2017-06-23T23:20:10Z

We can simplify the structure by changing the value to be a counter. This way we can just increment and decrement the counter for each class name.

<Modal isOpen={true} bodyOpenClassName={A} />
<Modal isOpen={false} bodyOpenClassName={A} />
<Modal isOpen={false} bodyOpenClassName={B} />

{ "A": 1, "B": 0 }

The refCount api can be updated to something like:

// register/increment
add(bodyOpenClassName);

// unregister/decrement
remove(bodyOpenClassName);

// check if a class name must be in `document.body`
active(bodyOpenClassName) -> Boolean

It seems better to handle toggle the class name and aria in the Modal class, otherwise it'll be hidden inside the refCount. Probably we need to improve both methods renderPortal and removePortal.

We also could use componentWillReceiveProps to throw an exception if bodyOpenClassName changes. Perhaps, it would be better to treat this as a violation.

It seems the way to go since we can't no longer rely on the reference of the instance neither the value of bodyOpenClassName (if it can change).

fuller · 2017-06-24T20:35:01Z

@diasbruno good call, on the counter. To make that work I've changed the code to be in theModalPortal component instead. That way we're only running refCount.add() when the modal is actually opening and refCount.remove() when the modal is closing, not just whenever renderPortal is being called.

coveralls · 2017-06-24T20:35:19Z

Coverage decreased (-0.1%) to 84.814% when pulling a78049b on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

coveralls · 2017-06-24T20:35:20Z

Coverage decreased (-0.1%) to 84.814% when pulling a78049b on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

diasbruno · 2017-06-24T23:24:52Z

This is really nice. Letting Modal be just a proxy class.

diasbruno · 2017-06-24T23:26:17Z

specs/Modal.spec.js

@@ -166,6 +166,7 @@ describe('State', () => {
    expect(
      moverlay(modal).className.includes('myOverlayClass')
    ).toBeTruthy();
+    unmountModal();


unmountModal shouldn't be necessary. afterEach should deal with clean up. I'll have a look on this.

Yeah I'll remove, it was failing during one of my initial attempts, but it's passing now with the latest updates 👍

diasbruno · 2017-06-24T23:27:55Z

specs/Modal.spec.js

+    renderModal({ isOpen: true });
+    expect(isBodyWithReactModalOpenClass()).toBeTruthy();
+    renderModal({ isOpen: false, bodyOpenClassName: 'testBodyClass' });
+    expect(isBodyWithReactModalOpenClass('testBodyClass')).toBeFalsy();


Expectation should be always True.

expect(!isBodyWithReactModalOpenClass('testBodyClass')).toBeTruthy();

diasbruno · 2017-06-24T23:28:31Z

specs/Modal.spec.js

+    expect(isBodyWithReactModalOpenClass()).toBeTruthy();
+    renderModal({ isOpen: false, bodyOpenClassName: 'testBodyClass' });
+    renderModal({ isOpen: false });
+    expect(isBodyWithReactModalOpenClass('testBodyClass')).toBeFalsy();


expect(!isBodyWithReactModalOpenClass('testBodyClass')).toBeTruthy();

diasbruno · 2017-06-24T23:29:10Z

specs/Modal.spec.js

    expect(isBodyWithReactModalOpenClass()).toBeTruthy();
    unmountModal();
    expect(isBodyWithReactModalOpenClass()).toBeTruthy();
    unmountModal();
-    expect(!isBodyWithReactModalOpenClass()).toBeTruthy();
+    expect(isBodyWithReactModalOpenClass()).toBeFalsy();


expect(!isBodyWithReactModalOpenClass()).toBeTruthy();

diasbruno · 2017-06-24T23:36:23Z

src/helpers/refCount.js

+
+export function totalCount() {
+  const count = Object.keys(modals)
+    .reduce((acc, curr) => acc + modals[curr], 0);


This can be inlined with return.

diasbruno · 2017-06-24T23:37:30Z

src/components/ModalPortal.js

    // Focus only needs to be set once when the modal is being opened
    if (!this.props.isOpen && newProps.isOpen) {
      this.setFocusAfterRender(true);
+      this.addModalInstance();


Perhaps, this.addModalInstance(); should be called from this.open();.

diasbruno · 2017-06-24T23:37:49Z

src/components/ModalPortal.js

      this.open();
    } else if (this.props.isOpen && !newProps.isOpen) {
+      this.removeModalInstance();


Perhaps, this.removeModalInstance(); should be called from this.close();.

diasbruno · 2017-06-24T23:38:33Z

src/components/ModalPortal.js

@@ -84,6 +102,7 @@ export default class ModalPortal extends Component {
  }

  componentWillUnmount() {
+    this.removeModalInstance();


Better let this.close() call this.removeModalInstance();.

I've moved the other ones, but in componentWillUnmount we aren't calling the close method at all, so the tests will fail.

We could call close method here, but I feel safer calling removeModalInstance directly so we don't run into potential issues if a closeTimeoutMS prop is set.

diasbruno · 2017-06-24T23:39:23Z

src/components/ModalPortal.js

@@ -62,16 +69,27 @@ export default class ModalPortal extends Component {
    // Focus needs to be set when mounting and already open
    if (this.props.isOpen) {
      this.setFocusAfterRender(true);
+      this.addModalInstance();


Better let this.open() call this.addModalInstance();.

diasbruno · 2017-06-24T23:43:34Z

src/components/Modal.js

-    if (props.ariaHideApp) {
-      ariaAppHider.toggle(props.isOpen, props.appElement);
-    }
-
    this.portal = renderSubtreeIntoContainer(this, (


This is something that bugs me. Not sure what happens with this.portal when a modal is open and close too many times. I'll make some tests.
When Modal is unmounted, can this property leaks?

diasbruno · 2017-06-25T15:17:25Z

This is going to bump minor version since we are going to introduce this rule for this class name, but it's backwards-compatible, so it will be released as v2.1.x.

coveralls · 2017-06-25T18:41:32Z

Coverage decreased (-0.01%) to 84.943% when pulling 480e6dd on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

coveralls · 2017-06-25T18:46:06Z

Coverage decreased (-0.01%) to 84.943% when pulling 7873156 on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

coveralls · 2017-06-25T18:46:06Z

Coverage decreased (-0.01%) to 84.943% when pulling 7873156 on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

diasbruno · 2017-06-25T19:57:00Z

src/components/ModalPortal.js

+    }
+  }
+
+  removeModalInstance() {


removeModalInstance can be beforeClose.

diasbruno · 2017-06-25T19:57:20Z

src/components/ModalPortal.js

@@ -99,12 +116,37 @@ export default class ModalPortal extends Component {
    this.content = content;
  }

+  addModalInstance() {


We can rename addModalInstance to beforeOpen.

Changed 👍

coveralls · 2017-06-25T20:19:16Z

Coverage decreased (-0.01%) to 84.943% when pulling 0ff3caf on ajfuller:fix/bodyOpenClassName-adding-classes into 8f3898a on reactjs:master.

diasbruno · 2017-06-25T20:26:11Z

I'm going to make the last release for v2.0 after merging #441. And later release this patch for version v2.1.

coveralls · 2017-06-26T13:51:48Z

Coverage increased (+0.05%) to 85.559% when pulling 24caa29 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

coveralls · 2017-06-26T13:51:49Z

Coverage increased (+0.05%) to 85.559% when pulling 24caa29 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

diasbruno · 2017-06-26T14:56:50Z

I guess it just needs to be rebase and squash all commits and we can ship it!

AndersDJohnson · 2017-06-26T16:34:59Z

src/components/ModalPortal.js

+    refCount.add(bodyOpenClassName);
+    // Add body class
+    elementClass(document.body).add(bodyOpenClassName);
+    // Add aria-hidden to appELement


@ajfuller appELement with a big L!

fuller · 2017-06-27T00:50:37Z

Screwed up my rebase and accidentally closed, hopefully should be good now

diasbruno · 2017-06-27T00:53:27Z

Just add [change] on the message and we can merge. Both issue and commit message, please.

coveralls · 2017-06-27T00:53:43Z

Coverage increased (+0.05%) to 85.559% when pulling 5c6ed53 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

coveralls · 2017-06-27T00:53:43Z

Coverage increased (+0.05%) to 85.559% when pulling 5c6ed53 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

coveralls · 2017-06-27T00:53:43Z

Coverage increased (+0.05%) to 85.559% when pulling 5c6ed53 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

fuller · 2017-06-27T01:02:12Z

@diasbruno I've updated both PR and commit name to include [change]

coveralls · 2017-06-27T01:03:57Z

Coverage increased (+0.05%) to 85.559% when pulling 8a6ceb3 on ajfuller:fix/bodyOpenClassName-adding-classes into 27579ca on reactjs:master.

diasbruno · 2017-06-27T01:08:08Z

Thank you, @ajfuller.

diasbruno · 2017-06-27T01:12:46Z

Great job. Released v2.1.0.

diasbruno requested changes Jun 24, 2017

View reviewed changes

diasbruno approved these changes Jun 25, 2017

View reviewed changes

diasbruno requested changes Jun 25, 2017

View reviewed changes

diasbruno approved these changes Jun 25, 2017

View reviewed changes

AndersDJohnson reviewed Jun 26, 2017

View reviewed changes

fuller force-pushed the fix/bodyOpenClassName-adding-classes branch 2 times, most recently from 3b445c0 to d38c83a Compare June 27, 2017 00:24

fuller closed this Jun 27, 2017

fuller force-pushed the fix/bodyOpenClassName-adding-classes branch from d38c83a to 27579ca Compare June 27, 2017 00:42

fuller reopened this Jun 27, 2017

fuller changed the title ~~[fixed] Class names being added to body during render of non-opened modals~~ [change] Class names being added to body during render of non-opened modals Jun 27, 2017

[change] Track open body className appropriately

8a6ceb3

fuller force-pushed the fix/bodyOpenClassName-adding-classes branch from 5c6ed53 to 8a6ceb3 Compare June 27, 2017 01:01

diasbruno merged commit 1baebf4 into reactjs:master Jun 27, 2017

[change] Class names being added to body during render of non-opened modals #436

[change] Class names being added to body during render of non-opened modals #436

Conversation

fuller commented Jun 23, 2017 • edited

Changes proposed:

Modal reference tracking

coveralls commented Jun 23, 2017 • edited

diasbruno commented Jun 23, 2017

diasbruno commented Jun 23, 2017

fuller commented Jun 23, 2017

diasbruno commented Jun 23, 2017

fuller commented Jun 24, 2017 • edited

coveralls commented Jun 24, 2017

coveralls commented Jun 24, 2017

diasbruno commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diasbruno commented Jun 25, 2017 • edited

coveralls commented Jun 25, 2017 • edited

coveralls commented Jun 25, 2017

coveralls commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 25, 2017 • edited

diasbruno commented Jun 25, 2017

coveralls commented Jun 26, 2017

coveralls commented Jun 26, 2017

diasbruno commented Jun 26, 2017

Choose a reason for hiding this comment

fuller commented Jun 27, 2017

diasbruno commented Jun 27, 2017 • edited

coveralls commented Jun 27, 2017

coveralls commented Jun 27, 2017 • edited

coveralls commented Jun 27, 2017

fuller commented Jun 27, 2017

coveralls commented Jun 27, 2017 • edited

diasbruno commented Jun 27, 2017

diasbruno commented Jun 27, 2017

fuller commented Jun 23, 2017 •

edited

coveralls commented Jun 23, 2017 •

edited

fuller commented Jun 24, 2017 •

edited

diasbruno commented Jun 25, 2017 •

edited

coveralls commented Jun 25, 2017 •

edited

coveralls commented Jun 25, 2017 •

edited

diasbruno commented Jun 27, 2017 •

edited

coveralls commented Jun 27, 2017 •

edited

coveralls commented Jun 27, 2017 •

edited