fix(utils): Replace utils.scrollIntoView with compute-scroll-into-view (

#487) * Add dependency * replace utils.scrollIntoView with new lib * pass a boundary so the behavior is still the same * better implementation * update contributors * replace package with the lower level one * swap implementation * bump version * clientHeight should match clientRect.height * add offsetheight * make sure parentNode works properly * remove tests that conflict with spec * update contrib * can't repro locally, try run ci again
downshift-js · Jul 9, 2018 · 01b7dad · 01b7dad
1 parent c510049
commit 01b7dad
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 143 deletions.
diff --git a/.all-contributorsrc b/.all-contributorsrc
@@ -872,6 +872,15 @@
       "contributions": [
         "code"
       ]
+    },
+    {
+      "login": "stipsan",
+      "name": "Cody Olsen",
+      "avatar_url": "https://avatars2.githubusercontent.com/u/81981?v=4",
+      "profile": "http://twitter.com/stipsan",
+      "contributions": [
+        "code"
+      ]
     }
   ]
 }
diff --git a/README.md b/README.md
@@ -19,7 +19,7 @@ autocomplete/dropdown/select/combobox components</p>
 [![downloads][downloads-badge]][npmcharts] [![version][version-badge]][package]
 [![MIT License][license-badge]][license]
 
-[![All Contributors](https://img.shields.io/badge/all_contributors-88-orange.svg?style=flat-square)](#contributors)
+[![All Contributors](https://img.shields.io/badge/all_contributors-89-orange.svg?style=flat-square)](#contributors)
 [![PRs Welcome][prs-badge]][prs] [![Chat][chat-badge]][chat]
 [![Code of Conduct][coc-badge]][coc]
 [![Join the community on Spectrum][spectrum-badge]][spectrum]
@@ -1020,7 +1020,7 @@ Thanks goes to these people ([emoji key][emojis]):
 | [<img src="https://avatars0.githubusercontent.com/u/29493001?v=4" width="100px;"/><br /><sub><b>Austin Tackaberry</b></sub>](http://austintackaberry.co)<br />[💬](#question-austintackaberry "Answering Questions") [💻](https://github.com/paypal/downshift/commits?author=austintackaberry "Code") [📖](https://github.com/paypal/downshift/commits?author=austintackaberry "Documentation") [🐛](https://github.com/paypal/downshift/issues?q=author%3Aaustintackaberry "Bug reports") [💡](#example-austintackaberry "Examples") [🤔](#ideas-austintackaberry "Ideas, Planning, & Feedback") [👀](#review-austintackaberry "Reviewed Pull Requests") [⚠️](https://github.com/paypal/downshift/commits?author=austintackaberry "Tests") | [<img src="https://avatars3.githubusercontent.com/u/4168055?v=4" width="100px;"/><br /><sub><b>Jean Duthon</b></sub>](https://github.com/jduthon)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Ajduthon "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=jduthon "Code") | [<img src="https://avatars3.githubusercontent.com/u/3889580?v=4" width="100px;"/><br /><sub><b>Anton Telesh</b></sub>](http://antontelesh.github.io)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3AAntontelesh "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=Antontelesh "Code") | [<img src="https://avatars3.githubusercontent.com/u/1060669?v=4" width="100px;"/><br /><sub><b>Eric Edem</b></sub>](https://github.com/ericedem)<br />[💻](https://github.com/paypal/downshift/commits?author=ericedem "Code") [📖](https://github.com/paypal/downshift/commits?author=ericedem "Documentation") [🤔](#ideas-ericedem "Ideas, Planning, & Feedback") [⚠️](https://github.com/paypal/downshift/commits?author=ericedem "Tests") | [<img src="https://avatars3.githubusercontent.com/u/3409645?v=4" width="100px;"/><br /><sub><b>Austin Wood</b></sub>](https://github.com/indiesquidge)<br />[💬](#question-indiesquidge "Answering Questions") [📖](https://github.com/paypal/downshift/commits?author=indiesquidge "Documentation") [👀](#review-indiesquidge "Reviewed Pull Requests") | [<img src="https://avatars3.githubusercontent.com/u/14275790?v=4" width="100px;"/><br /><sub><b>Mark Murray</b></sub>](https://github.com/mmmurray)<br />[🚇](#infra-mmmurray "Infrastructure (Hosting, Build-Tools, etc)") | [<img src="https://avatars0.githubusercontent.com/u/1862172?v=4" width="100px;"/><br /><sub><b>Gianmarco</b></sub>](https://github.com/gsimone)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Agsimone "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=gsimone "Code") |
 | [<img src="https://avatars2.githubusercontent.com/u/6838136?v=4" width="100px;"/><br /><sub><b>Emmanuel Pastor</b></sub>](https://github.com/pastr)<br />[💡](#example-pastr "Examples") | [<img src="https://avatars2.githubusercontent.com/u/10345034?v=4" width="100px;"/><br /><sub><b>dalehurwitz</b></sub>](https://github.com/dalehurwitz)<br />[💻](https://github.com/paypal/downshift/commits?author=dalehurwitz "Code") | [<img src="https://avatars1.githubusercontent.com/u/4813007?v=4" width="100px;"/><br /><sub><b>Bogdan Lobor</b></sub>](https://github.com/blobor)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Ablobor "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=blobor "Code") | [<img src="https://avatars0.githubusercontent.com/u/1127238?v=4" width="100px;"/><br /><sub><b>Luke Herrington</b></sub>](https://github.com/infiniteluke)<br />[💡](#example-infiniteluke "Examples") | [<img src="https://avatars2.githubusercontent.com/u/6361167?v=4" width="100px;"/><br /><sub><b>Brandon Clemons</b></sub>](https://github.com/drobannx)<br />[💻](https://github.com/paypal/downshift/commits?author=drobannx "Code") | [<img src="https://avatars0.githubusercontent.com/u/10591587?v=4" width="100px;"/><br /><sub><b>Kieran</b></sub>](https://github.com/aMollusk)<br />[💻](https://github.com/paypal/downshift/commits?author=aMollusk "Code") | [<img src="https://avatars3.githubusercontent.com/u/11570627?v=4" width="100px;"/><br /><sub><b>Brushedoctopus</b></sub>](https://github.com/Brushedoctopus)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3ABrushedoctopus "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=Brushedoctopus "Code") |
 | [<img src="https://avatars3.githubusercontent.com/u/5456216?v=4" width="100px;"/><br /><sub><b>Cameron Edwards</b></sub>](http://cameronpedwards.com)<br />[💻](https://github.com/paypal/downshift/commits?author=cameronprattedwards "Code") [⚠️](https://github.com/paypal/downshift/commits?author=cameronprattedwards "Tests") | [<img src="https://avatars2.githubusercontent.com/u/179534?v=4" width="100px;"/><br /><sub><b>stereobooster</b></sub>](https://github.com/stereobooster)<br />[💻](https://github.com/paypal/downshift/commits?author=stereobooster "Code") [⚠️](https://github.com/paypal/downshift/commits?author=stereobooster "Tests") | [<img src="https://avatars0.githubusercontent.com/u/934879?v=4" width="100px;"/><br /><sub><b>Trevor Pierce</b></sub>](https://github.com/1Copenut)<br />[👀](#review-1Copenut "Reviewed Pull Requests") | [<img src="https://avatars1.githubusercontent.com/u/1334982?v=4" width="100px;"/><br /><sub><b>Xuefei Li</b></sub>](http://xuefei-frank.com)<br />[💻](https://github.com/paypal/downshift/commits?author=franklixuefei "Code") | [<img src="https://avatars0.githubusercontent.com/u/7252803?v=4" width="100px;"/><br /><sub><b>Alfred Ringstad</b></sub>](https://hyperlab.se)<br />[💻](https://github.com/paypal/downshift/commits?author=alfredringstad "Code") | [<img src="https://avatars0.githubusercontent.com/u/6895497?v=4" width="100px;"/><br /><sub><b>D[oa]vid Weisz</b></sub>](https://github.com/dovidweisz)<br />[💻](https://github.com/paypal/downshift/commits?author=dovidweisz "Code") | [<img src="https://avatars0.githubusercontent.com/u/19773?v=4" width="100px;"/><br /><sub><b>Royston Shufflebotham</b></sub>](https://github.com/RoystonS)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3ARoystonS "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=RoystonS "Code") |
-| [<img src="https://avatars3.githubusercontent.com/u/6643991?v=4" width="100px;"/><br /><sub><b>Michaël De Boey</b></sub>](http://michaeldeboey.be)<br />[💻](https://github.com/paypal/downshift/commits?author=MichaelDeBoey "Code") | [<img src="https://avatars3.githubusercontent.com/u/4412771?v=4" width="100px;"/><br /><sub><b>Henry</b></sub>](https://github.com/EricHenry)<br />[💻](https://github.com/paypal/downshift/commits?author=EricHenry "Code") | [<img src="https://avatars3.githubusercontent.com/u/2180127?v=4" width="100px;"/><br /><sub><b>Andrew Walton</b></sub>](http://www.greenarrow.me)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Agreen-arrow "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=green-arrow "Code") [⚠️](https://github.com/paypal/downshift/commits?author=green-arrow "Tests") | [<img src="https://avatars0.githubusercontent.com/u/13774309?v=4" width="100px;"/><br /><sub><b>Arthur Denner</b></sub>](https://github.com/arthurdenner)<br />[💻](https://github.com/paypal/downshift/commits?author=arthurdenner "Code") |
+| [<img src="https://avatars3.githubusercontent.com/u/6643991?v=4" width="100px;"/><br /><sub><b>Michaël De Boey</b></sub>](http://michaeldeboey.be)<br />[💻](https://github.com/paypal/downshift/commits?author=MichaelDeBoey "Code") | [<img src="https://avatars3.githubusercontent.com/u/4412771?v=4" width="100px;"/><br /><sub><b>Henry</b></sub>](https://github.com/EricHenry)<br />[💻](https://github.com/paypal/downshift/commits?author=EricHenry "Code") | [<img src="https://avatars3.githubusercontent.com/u/2180127?v=4" width="100px;"/><br /><sub><b>Andrew Walton</b></sub>](http://www.greenarrow.me)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Agreen-arrow "Bug reports") [💻](https://github.com/paypal/downshift/commits?author=green-arrow "Code") [⚠️](https://github.com/paypal/downshift/commits?author=green-arrow "Tests") | [<img src="https://avatars0.githubusercontent.com/u/13774309?v=4" width="100px;"/><br /><sub><b>Arthur Denner</b></sub>](https://github.com/arthurdenner)<br />[💻](https://github.com/paypal/downshift/commits?author=arthurdenner "Code") | [<img src="https://avatars2.githubusercontent.com/u/81981?v=4" width="100px;"/><br /><sub><b>Cody Olsen</b></sub>](http://twitter.com/stipsan)<br />[💻](https://github.com/paypal/downshift/commits?author=stipsan "Code") |
 
 <!-- ALL-CONTRIBUTORS-LIST:END -->
 

diff --git a/package.json b/package.json
@@ -61,6 +61,7 @@
     "react": ">=0.14.9"
   },
   "dependencies": {
+    "compute-scroll-into-view": "^1.0.2",
     "prop-types": "^15.6.0"
   },
   "devDependencies": {

diff --git a/src/__tests__/utils.findParent.js b/src/__tests__/utils.findParent.js
diff --git a/src/__tests__/utils.scroll-into-view.js b/src/__tests__/utils.scroll-into-view.js
@@ -49,28 +49,6 @@ test('aligns to top when the node is above the scrollable parent', () => {
   expect(scrollableNode.scrollTop).toBe(5)
 })
 
-test('aligns to top when the node is above view area', () => {
-  const node = getNode({height: 40, top: -15})
-  const scrollableNode = getScrollableNode({
-    top: -50,
-    scrollTop: 100,
-    children: [node],
-  })
-  scrollIntoView(node, scrollableNode)
-  expect(scrollableNode.scrollTop).toBe(85)
-})
-
-test('aligns to top of view area when the node is above view area and scrollable parent top', () => {
-  const node = getNode({height: 40, top: -75})
-  const scrollableNode = getScrollableNode({
-    top: -50,
-    scrollTop: 100,
-    children: [node],
-  })
-  scrollIntoView(node, scrollableNode)
-  expect(scrollableNode.scrollTop).toBe(25)
-})
-
 test('aligns to top of scrollable parent when the node is above view area', () => {
   const node = getNode({height: 40, top: -50})
   const scrollableNode = getScrollableNode({
@@ -82,18 +60,6 @@ test('aligns to top of scrollable parent when the node is above view area', () =
   expect(scrollableNode.scrollTop).toBe(0)
 })
 
-test('aligns to bottom when the node is below the scrollable parent and parent top above view area', () => {
-  const node = getNode({height: 40, top: 280})
-  const scrollableNode = getScrollableNode({
-    top: -60,
-    height: 360,
-    scrollTop: 28,
-    children: [node],
-  })
-  scrollIntoView(node, scrollableNode)
-  expect(scrollableNode.scrollTop).toBe(48)
-})
-
 test('aligns to bottom when the node is below the scrollable parent', () => {
   const nodeTop = 115
   const node = getNode({height: 10, top: nodeTop})
@@ -105,25 +71,11 @@ test('aligns to bottom when the node is below the scrollable parent', () => {
   expect(scrollableNode.scrollTop).toBe(25)
 })
 
-test('aligns to bottom when the the node is below the scrollable parent and scrollable parent has a border', () => {
-  const nodeTop = 115
-  const node = getNode({height: 10, top: nodeTop})
-  const scrollableNode = getScrollableNode({
-    height: 100,
-    children: [node],
-    borderBottomWidth: '2px',
-    borderTopWidth: '2px',
-  })
-  scrollIntoView(node, scrollableNode)
-  expect(scrollableNode.scrollTop).toBe(27)
-})
-
 function getScrollableNode(overrides = {}) {
   return getNode({
     height: 100,
     top: 0,
     scrollTop: 0,
-    clientHeight: 50,
     scrollHeight: 150,
     ...overrides,
   })
@@ -154,8 +106,10 @@ function getNode({
 
   Object.defineProperties(div, {
     clientHeight: {value: clientHeight},
+    offsetHeight: {value: clientHeight},
     scrollHeight: {value: scrollHeight},
   })
   children.forEach(child => div.appendChild(child))
+  document.documentElement.appendChild(div)
   return div
 }
diff --git a/src/utils.js b/src/utils.js
@@ -1,3 +1,5 @@
+import computeScrollIntoView from 'compute-scroll-into-view'
+
 let idCounter = 0
 
 /**
@@ -13,98 +15,25 @@ function cbToCb(cb) {
 }
 function noop() {}
 
-function findParent(finder, node, rootNode) {
-  if (node !== null && node !== rootNode.parentNode) {
-    if (finder(node)) {
-      if (node === document.body && node.scrollTop === 0) {
-        // in chrome body.scrollTop always return 0
-        return document.documentElement
-      }
-      return node
-    } else {
-      return findParent(finder, node.parentNode, rootNode)
-    }
-  } else {
-    return null
-  }
-}
-
-/**
- * Get the closest element that scrolls
- * @param {HTMLElement} node - the child element to start searching for scroll parent at
- * @param {HTMLElement} rootNode - the root element of the component
- * @return {HTMLElement} the closest parentNode that scrolls
- */
-const getClosestScrollParent = findParent.bind(
-  null,
-  node => node.scrollHeight > node.clientHeight,
-)
-
 /**
  * Scroll node into view if necessary
  * @param {HTMLElement} node - the element that should scroll into view
  * @param {HTMLElement} rootNode - the root element of the component
- * @param {Boolean} alignToTop - align element to the top of the visible area of the scrollable ancestor
  */
-// eslint-disable-next-line complexity
 function scrollIntoView(node, rootNode) {
-  const scrollParent = getClosestScrollParent(node, rootNode)
-  if (scrollParent === null) {
+  if (node === null) {
     return
   }
-  const scrollParentStyles = getComputedStyle(scrollParent)
-  const scrollParentRect = scrollParent.getBoundingClientRect()
-  const scrollParentBorderTopWidth = parseInt(
-    scrollParentStyles.borderTopWidth,
-    10,
-  )
-  const scrollParentBorderBottomWidth = parseInt(
-    scrollParentStyles.borderBottomWidth,
-    10,
-  )
-  const bordersWidth =
-    scrollParentBorderTopWidth + scrollParentBorderBottomWidth
-  const scrollParentTop = scrollParentRect.top + scrollParentBorderTopWidth
-  const nodeRect = node.getBoundingClientRect()
 
-  if (nodeRect.top < 0 && scrollParentRect.top < 0) {
-    scrollParent.scrollTop += nodeRect.top
-    return
-  }
-
-  if (nodeRect.top < 0) {
-    // the item is above the viewport and the parent is not above the viewport
-    scrollParent.scrollTop += nodeRect.top - scrollParentTop
-    return
-  }
-
-  if (nodeRect.top > 0 && scrollParentRect.top < 0) {
-    if (
-      scrollParentRect.bottom > 0 &&
-      nodeRect.bottom + bordersWidth > scrollParentRect.bottom
-    ) {
-      // the item is below scrollable area
-      scrollParent.scrollTop +=
-        nodeRect.bottom - scrollParentRect.bottom + bordersWidth
-    }
-    // item and parent top are on different sides of view top border (do nothing)
-    return
-  }
-
-  const nodeOffsetTop = nodeRect.top + scrollParent.scrollTop
-  const nodeTop = nodeOffsetTop - scrollParentTop
-  if (nodeTop < scrollParent.scrollTop) {
-    // the item is above the scrollable area
-    scrollParent.scrollTop = nodeTop
-  } else if (
-    nodeTop + nodeRect.height + bordersWidth >
-    scrollParent.scrollTop + scrollParentRect.height
-  ) {
-    // the item is below the scrollable area
-    scrollParent.scrollTop =
-      nodeTop + nodeRect.height - scrollParentRect.height + bordersWidth
-  }
-  // the item is within the scrollable area (do nothing)
+  const actions = computeScrollIntoView(node, {
+    boundary: rootNode,
+    block: 'nearest',
+    scrollMode: 'if-needed',
+  })
+  actions.forEach(({el, top, left}) => {
+    el.scrollTop = top
+    el.scrollLeft = left
+  })
 }
 
 /**
@@ -313,7 +242,6 @@ export {
   callAll,
   debounce,
   scrollIntoView,
-  findParent,
   generateId,
   getA11yStatusMessage,
   unwrapArray,