fix(Portal): do not take focus on mount (Semantic-Org#1341)

harel · Feb 18, 2017 · 5906d2c · 5906d2c
1 parent 6164e51
commit 5906d2c
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 45 deletions.
diff --git a/src/addons/Portal/Portal.js b/src/addons/Portal/Portal.js
@@ -335,7 +335,7 @@ class Portal extends Component {
     if (!this.state.open) return
     debug('renderPortal()')
 
-    const { children, className, closeOnTriggerBlur, openOnTriggerFocus } = this.props
+    const { children, className } = this.props
 
     this.mountPortal()
 
@@ -358,22 +358,6 @@ class Portal extends Component {
 
     this.portalNode = this.rootNode.firstElementChild
 
-    // don't take focus away from for focus based portal triggers
-    if (!this.didInitialRender && !(openOnTriggerFocus || closeOnTriggerBlur)) {
-      this.didInitialRender = true
-
-      // add a tabIndex so we can focus it, remove outline
-      this.portalNode.tabIndex = -1
-      this.portalNode.style.outline = 'none'
-
-      // Wait a tick for things like popups which need to calculate where the popup shows up.
-      // Otherwise, the element is focused at its initial position, scrolling the browser, then
-      // it is immediately repositioned at the proper location.
-      setTimeout(() => {
-        if (this.portalNode) this.portalNode.focus()
-      })
-    }
-
     this.portalNode.addEventListener('mouseleave', this.handlePortalMouseLeave)
     this.portalNode.addEventListener('mouseenter', this.handlePortalMouseEnter)
   }
@@ -405,7 +389,6 @@ class Portal extends Component {
 
   unmountPortal = () => {
     if (!isBrowser || !this.rootNode) return
-    this.didInitialRender = false
 
     debug('unmountPortal()')
 

diff --git a/test/specs/addons/Portal/Portal-test.js b/test/specs/addons/Portal/Portal-test.js
@@ -463,56 +463,61 @@ describe('Portal', () => {
     })
   })
 
+  // Heads Up!
+  // Portals used to take focus on mount and restore focus to the original activeElement on unMount.
+  // One by one, these auto set/remove focus features were removed and the assertions negated.
+  // Leave these tests here to ensure we aren't ever stealing focus.
   describe('focus', () => {
-    it('takes focus on first render', (done) => {
+    it('does not take focus onMount', (done) => {
       wrapperMount(<Portal defaultOpen><p /></Portal>)
 
       setTimeout(() => {
         const { portalNode } = wrapper.instance()
-        document.activeElement.should.equal(portalNode)
-        expect(portalNode.getAttribute('tabindex')).to.equal('-1')
-        expect(portalNode.style.outline).to.equal('none')
+        document.activeElement.should.not.equal(portalNode)
         done()
       }, 0)
     })
 
-    it('does not take focus when mounted on portals that closeOnTriggerBlur', (done) => {
-      wrapperMount(<Portal defaultOpen closeOnTriggerBlur><p /></Portal>)
+    it('does not take focus on unMount', (done) => {
+      const input = document.createElement('input')
+      document.body.appendChild(input)
 
-      setTimeout(() => {
-        const { portalNode } = wrapper.instance()
-        document.activeElement.should.not.equal(portalNode)
-        expect(portalNode.getAttribute('tabindex')).to.not.equal('-1')
-        expect(portalNode.style.outline).to.not.equal('none')
-        done()
-      }, 0)
-    })
+      input.focus()
+      document.activeElement.should.equal(input)
 
-    it('does not take focus when mounted on portals that openOnTriggerFocus', (done) => {
-      wrapperMount(<Portal defaultOpen openOnTriggerFocus><p /></Portal>)
+      wrapperMount(<Portal open><p /></Portal>)
+      document.activeElement.should.equal(input)
 
       setTimeout(() => {
-        const { portalNode } = wrapper.instance()
-        document.activeElement.should.not.equal(portalNode)
-        expect(portalNode.getAttribute('tabindex')).to.not.equal('-1')
-        expect(portalNode.style.outline).to.not.equal('none')
+        document.activeElement.should.equal(input)
+
+        wrapper.setProps({ open: false })
+        wrapper.unmount()
+
+        document.activeElement.should.equal(input)
+
+        document.body.removeChild(input)
         done()
       }, 0)
     })
 
-    it('does not take focus on subsequent renders', (done) => {
-      wrapperMount(<Portal defaultOpen><input data-focus-me /></Portal>)
+    it('does not take focus on re-render', (done) => {
+      const input = document.createElement('input')
+      document.body.appendChild(input)
 
-      setTimeout(() => {
-        const { portalNode } = wrapper.instance()
-        document.activeElement.should.equal(portalNode)
+      input.focus()
+      document.activeElement.should.equal(input)
+
+      wrapperMount(<Portal defaultOpen><p /></Portal>)
+      document.activeElement.should.equal(input)
 
-        const input = document.querySelector('input[data-focus-me]')
-        input.focus()
+      setTimeout(() => {
         document.activeElement.should.equal(input)
 
         wrapper.render()
         document.activeElement.should.equal(input)
+
+        document.body.removeChild(input)
         done()
       }, 0)
     })