feat(menu-surface): Update setPosition adapter API to use numeric values (#4351)

Accepting `string` values implies that we support all valid CSS units (`em`, `rem`, `%`, etc.), but in truth we only support `px`.

Changing the type of the properties from `string` to `number` makes this clearer, and also simplifies converting the package to TypeScript in PR #4273.

BREAKING CHANGE: `MDCMenuSurfaceAdapter.setPosition()` now expects an object with properties of type `number` rather than `string`. E.g., `setPosition({top: '5px', left: '10px'})` is now `setPosition({top: 5, left: 10})`.

Author: acdvorak

packages/mdc-menu-surface/README.md

@@ -209,7 +209,7 @@ Method Signature | Description
 `getBodyDimensions() => {width: number, height: number}` | Returns an object with width and height of the body, in pixels.
 `getWindowDimensions() => {width: number, height: number}` | Returns an object with width and height of the viewport, in pixels.
 `getWindowScroll() => {x: number, y: number}` | Returns an object with the amount the body has been scrolled on the `x` and `y` axis.
-`setPosition(position: {top: string, right: string, bottom: string, left: string}) => void` | Sets the position of the menu surface element.
+`setPosition(position: {top: number, right: number, bottom: number, left: number}) => void` | Sets the position of the menu surface element.
 `setMaxHeight(value: string) => void` | Sets `max-height` style for the menu surface element.
 
 ### `MDCMenuSurfaceFoundation`

packages/mdc-menu-surface/adapter.js

@@ -113,10 +113,10 @@ class MDCMenuSurfaceAdapter {
   getWindowScroll() {}
 
   /** @param {!{
-  *   top: (string|undefined),
-  *   right: (string|undefined),
-  *   bottom: (string|undefined),
-  *   left: (string|undefined)
+  *   top: (number|undefined),
+  *   right: (number|undefined),
+  *   bottom: (number|undefined),
+  *   left: (number|undefined)
   * }} position */
   setPosition(position) {}
 

packages/mdc-menu-surface/foundation.js

@@ -41,7 +41,7 @@ let AnchorMargin;
  *   surfaceHeight: number,
  *   surfaceWidth: number,
  *   bodyDimensions,
- *   windowScroll,
+ *   windowScroll
  * }}
  */
 let AutoLayoutMeasurements;
@@ -410,8 +410,8 @@ class MDCMenuSurfaceFoundation extends MDCFoundation {
     const horizontalOffset = this.getHorizontalOriginOffset_(corner);
     const verticalOffset = this.getVerticalOriginOffset_(corner);
     let position = {
-      [horizontalAlignment]: horizontalOffset ? horizontalOffset : '0',
-      [verticalAlignment]: verticalOffset ? verticalOffset : '0',
+      [horizontalAlignment]: horizontalOffset,
+      [verticalAlignment]: verticalOffset,
     };
     const {anchorWidth, surfaceWidth} = this.measures_;
     // Center align when anchor width is comparable or greater than menu surface, otherwise keep corner.
@@ -424,12 +424,6 @@ class MDCMenuSurfaceFoundation extends MDCFoundation {
       position = this.adjustPositionForHoistedElement_(position);
     }
 
-    for (const prop in position) {
-      if (position.hasOwnProperty(prop) && position[prop] !== '0') {
-        position[prop] = `${parseInt(position[prop], 10)}px`;
-      }
-    }
-
     this.adapter_.setTransformOrigin(`${horizontalAlignment} ${verticalAlignment}`);
     this.adapter_.setPosition(position);
     this.adapter_.setMaxHeight(maxMenuSurfaceHeight ? maxMenuSurfaceHeight + 'px' : '');
@@ -442,16 +436,16 @@ class MDCMenuSurfaceFoundation extends MDCFoundation {
    * Calculates the offsets for positioning the menu-surface when the menu-surface has been
    * hoisted to the body.
    * @param {!{
-   *   top: (string|undefined),
-   *   right: (string|undefined),
-   *   bottom: (string|undefined),
-   *   left: (string|undefined)
+   *   top: (number|undefined),
+   *   right: (number|undefined),
+   *   bottom: (number|undefined),
+   *   left: (number|undefined)
    * }} position
    * @return {!{
-   *   top: (string|undefined),
-   *   right: (string|undefined),
-   *   bottom: (string|undefined),
-   *   left: (string|undefined)
+   *   top: (number|undefined),
+   *   right: (number|undefined),
+   *   bottom: (number|undefined),
+   *   left: (number|undefined)
    * }} position
    * @private
    */
@@ -463,20 +457,20 @@ class MDCMenuSurfaceFoundation extends MDCFoundation {
         // Hoisted surfaces need to have the anchor elements location on the page added to the
         // position properties for proper alignment on the body.
         if (viewportDistance.hasOwnProperty(prop)) {
-          position[prop] = parseInt(position[prop], 10) + viewportDistance[prop];
+          position[prop] += viewportDistance[prop];
         }
 
         // Surfaces that are absolutely positioned need to have additional calculations for scroll
         // and bottom positioning.
         if (!this.isFixedPosition_) {
           if (prop === 'top') {
-            position[prop] = parseInt(position[prop], 10) + windowScroll.y;
+            position[prop] += windowScroll.y;
           } else if (prop === 'bottom') {
-            position[prop] = parseInt(position[prop], 10) - windowScroll.y;
+            position[prop] -= windowScroll.y;
           } else if (prop === 'left') {
-            position[prop] = parseInt(position[prop], 10) + windowScroll.x;
+            position[prop] += windowScroll.x;
           } else if (prop === 'right') {
-            position[prop] = parseInt(position[prop], 10) - windowScroll.x;
+            position[prop] -= windowScroll.x;
           }
         }
       }

packages/mdc-menu-surface/index.js

@@ -247,7 +247,7 @@ class MDCMenuSurface extends MDCComponent {
       getInnerDimensions: () => {
         return {width: this.root_.offsetWidth, height: this.root_.offsetHeight};
       },
-      getAnchorDimensions: () => this.anchorElement && this.anchorElement.getBoundingClientRect(),
+      getAnchorDimensions: () => this.anchorElement ? this.anchorElement.getBoundingClientRect() : null,
       getWindowDimensions: () => {
         return {width: window.innerWidth, height: window.innerHeight};
       },
@@ -258,10 +258,10 @@ class MDCMenuSurface extends MDCComponent {
         return {x: window.pageXOffset, y: window.pageYOffset};
       },
       setPosition: (position) => {
-        this.root_.style.left = 'left' in position ? position.left : null;
-        this.root_.style.right = 'right' in position ? position.right : null;
-        this.root_.style.top = 'top' in position ? position.top : null;
-        this.root_.style.bottom = 'bottom' in position ? position.bottom : null;
+        this.root_.style.left = 'left' in position ? `${position.left}px` : '';
+        this.root_.style.right = 'right' in position ? `${position.right}px` : '';
+        this.root_.style.top = 'top' in position ? `${position.top}px` : '';
+        this.root_.style.bottom = 'bottom' in position ? `${position.bottom}px` : '';
       },
       setMaxHeight: (height) => {
         this.root_.style.maxHeight = height;

test/unit/mdc-menu-surface/mdc-menu-surface.test.js

@@ -27,12 +27,15 @@ import domEvents from 'dom-events';
 import td from 'testdouble';
 
 import {MDCMenuSurface, MDCMenuSurfaceFoundation} from '../../../packages/mdc-menu-surface/index';
-import {strings, cssClasses, Corner} from '../../../packages/mdc-menu-surface/constants';
+import {Corner, cssClasses, strings} from '../../../packages/mdc-menu-surface/constants';
 import {getTransformPropertyName} from '../../../packages/mdc-menu-surface/util';
 
-function getFixture(open) {
+function getFixture(open, fixedPosition = false) {
+  const openClass = open ? 'mdc-menu-surface--open' : '';
+  const fixedClass = fixedPosition ? 'mdc-menu-surface--fixed' : '';
+
   return bel`
-    <div class="mdc-menu-surface ${open ? 'mdc-menu-surface--open' : ''}" tabindex="-1">
+    <div class="mdc-menu-surface ${openClass} ${fixedClass}" tabindex="-1">
       <ul class="mdc-list" role="menu">
         <li class="mdc-list-item" role="menuitem" tabindex="0">Item</a>
         <li role="separator"></li>
@@ -42,8 +45,8 @@ function getFixture(open) {
   `;
 }
 
-function setupTest(open = false) {
-  const root = getFixture(open);
+function setupTest(open = false, fixedPosition = false) {
+  const root = getFixture(open, fixedPosition);
   const MockFoundationConstructor = td.constructor(MDCMenuSurfaceFoundation);
   const mockFoundation = new MockFoundationConstructor();
   const component = new MDCMenuSurface(root, mockFoundation);
@@ -147,6 +150,11 @@ test('setIsHoisted', () => {
   td.verify(mockFoundation.setIsHoisted(true));
 });
 
+test('setFixedPosition is called when CSS class is present', () => {
+  const {mockFoundation} = setupTest(false, true);
+  td.verify(mockFoundation.setFixedPosition(true));
+});
+
 test('setFixedPosition is true', () => {
   const {root, component, mockFoundation} = setupTest();
   component.setFixedPosition(true);
@@ -174,12 +182,18 @@ test('setAnchorCorner', () => {
   td.verify(mockFoundation.setAnchorCorner(Corner.TOP_START));
 });
 
-test('setAnchorMargin', () => {
+test('setAnchorMargin with all object properties defined', () => {
   const {component, mockFoundation} = setupTest();
   component.setAnchorMargin({top: 0, right: 0, bottom: 0, left: 0});
   td.verify(mockFoundation.setAnchorMargin({top: 0, right: 0, bottom: 0, left: 0}));
 });
 
+test('setAnchorMargin with empty object', () => {
+  const {component, mockFoundation} = setupTest();
+  component.setAnchorMargin({});
+  td.verify(mockFoundation.setAnchorMargin({}));
+});
+
 test('setQuickOpen', () => {
   const {component, mockFoundation} = setupTest();
   component.quickOpen = false;
@@ -362,7 +376,7 @@ test('adapter#getAnchorDimensions returns undefined if there is no anchor elemen
   const {root, component} = setupTest(true);
   document.body.appendChild(root);
   component.initialSyncWithDOM();
-  assert.isUndefined(component.getDefaultFoundation().adapter_.getAnchorDimensions());
+  assert.isNull(component.getDefaultFoundation().adapter_.getAnchorDimensions());
   document.body.removeChild(root);
 });
 
@@ -466,10 +480,10 @@ test('adapter#setTransformOrigin sets the correct transform origin on the menu s
 
 test('adapter#setPosition sets the correct position on the menu surface element', () => {
   const {root, component} = setupTest();
-  component.getDefaultFoundation().adapter_.setPosition({top: '10px', left: '11px'});
+  component.getDefaultFoundation().adapter_.setPosition({top: 10, left: 11});
   assert.equal(root.style.top, '10px');
   assert.equal(root.style.left, '11px');
-  component.getDefaultFoundation().adapter_.setPosition({bottom: '10px', right: '11px'});
+  component.getDefaultFoundation().adapter_.setPosition({bottom: 10, right: 11});
   assert.equal(root.style.bottom, '10px');
   assert.equal(root.style.right, '11px');
 });
@@ -479,3 +493,35 @@ test('adapter#setMaxHeight sets the maxHeight style on the menu surface element'
   component.getDefaultFoundation().adapter_.setMaxHeight('100px');
   assert.equal(root.style.maxHeight, '100px');
 });
+
+test('Pressing Shift+Tab on first element focuses the last menu surface element', () => {
+  const root = getFixture(true);
+  document.body.appendChild(root);
+  const firstItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0];
+  const lastItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1];
+  const component = new MDCMenuSurface(root);
+  component.open = true;
+
+  firstItem.focus();
+  component.getDefaultFoundation().handleKeydown({key: 'Tab', shiftKey: true, preventDefault: () => {}});
+  assert.equal(document.activeElement, lastItem);
+
+  component.open = false;
+  document.body.removeChild(root);
+});
+
+test('Pressing Tab on last element focuses the first menu surface element', () => {
+  const root = getFixture(true);
+  document.body.appendChild(root);
+  const firstItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[0];
+  const lastItem = root.querySelectorAll(strings.FOCUSABLE_ELEMENTS)[1];
+  const component = new MDCMenuSurface(root);
+  component.open = true;
+
+  lastItem.focus();
+  component.getDefaultFoundation().handleKeydown({key: 'Tab', shiftKey: false, preventDefault: () => {}});
+  assert.equal(document.activeElement, firstItem);
+
+  component.open = false;
+  document.body.removeChild(root);
+});