[cr-action-menu] Allow configurable anchors.

ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * @typedef {{
+ *   top: number,
+ *   left: number,
+ *   width: (number| undefined),
+ *   height: (number| undefined),
+ *   anchorPositionX: (number| undefined),
+ *   anchorPositionY: (number| undefined),
+ *   minX: (number| undefined),
+ *   minY: (number| undefined),
+ *   maxX: (number| undefined),
+ *   maxY: (number| undefined),
+ * }}
+ */
+var ShowConfig;
+
 Polymer({
   is: 'cr-action-menu',
   extends: 'dialog',
 
   /**
    * List of all options in this action menu.
    * @private {?NodeList<!Element>}
    */
   options_: null,
 
   /**
    * The element which the action menu will be anchored to. Also the element
    * where focus will be returned after the menu is closed.

[dpapad, 2017/04/14 22:06:31]

This needs a comment now, that says "Only populated if action menu is opened
with showAt()"

[calamity, 2017/04/19 05:31:07]

Done.

    * @private {?Element}
    */
   anchorElement_: null,
 
   /**
    * Bound reference to an event listener function such that it can be removed
    * on detach.
    * @private {?Function}
    */
   boundClose_: null,
@@ skipping 116 matching lines @@
     } while (!nextOption && counter < numOptions);
 
     return nextOption;
   },
 
   /** @override */
   close: function() {
     // Removing 'resize' and 'popstate' listeners when dialog is closed.
     this.removeListeners_();
     HTMLDialogElement.prototype.close.call(this);
-    this.anchorElement_.focus();
+    if (this.anchorElement_)
+      this.anchorElement_.focus();
+
     this.anchorElement_ = null;

[dpapad, 2017/04/14 22:06:31]

This line should be inside the if too,

if (this.anchorElement_) {
  this.anchorElement_.focus();
  this.anchorElement_ = null;
}

[calamity, 2017/04/19 05:31:07]

Done.

   },
 
   /**
    * Shows the menu anchored to the given element.
    * @param {!Element} anchorElement
    */
   showAt: function(anchorElement) {
     this.anchorElement_ = anchorElement;
+    this.anchorElement_.scrollIntoViewIfNeeded();
+    var rect = this.anchorElement_.getBoundingClientRect();
+    this.showAtPosition({
+      top: rect.top,
+      left: rect.left,
+      height: rect.height,
+      width: rect.width,
+      // Default to anchoring towards the left.
+      anchorPositionX: -1,
+    });
+  },
+
+  /**
+   * Returns the point to start along the X or Y axis given a start and end
+   * point to anchor to, the length of the target and the direction to anchor
+   * in. If honoring the anchor would force the menu outside of min/max, this
+   * will ignore the anchor position and try to keep the menu within min/max.
+   * @param {number} start
+   * @param {number} end
+   * @param {number} length
+   * @param {number} anchorPosition
+   * @param {number} min
+   * @param {number} max
+   * @return {number}
+   */
+  getStartPointWithAnchor: function(
+      start, end, length, anchorPosition, min, max) {
+    var startPoint = (start + end - length) / 2 +

[dpapad, 2017/04/14 22:06:31]

TL;DR equivalent to
var startPoint = anchorPosition == 1 ? start : end - length;

This is an overly complex calculation I think. Can be simplified as follows:

1) Solving the equation, a little bit further leads to (using aP for
anchorPosition)
var startPoint = (start * (1 + aP) + (1 - aP) * (end - length)) / 2;

2) Now, I believe that anchorPosition can only be 1 or -1. So:
 - if aP = -1:
   var startPoint = end - length; // Much simpler!
 - if aP = 1:
   var = start; // Much much simpler!!

So to sum up, I think this code would be written simpler as follows
assert(Math.abs(anchorPosition) == 1);
var startPoint = anchorPosition == 1 ? start : end - length;

[calamity, 2017/04/19 05:31:07]

It depends if you ever want to center-align  a menu to a rect (or even do a
percentage offset) by using anchorPosition = 0/0.5/etc (there's a test case for
this).

I think being able to anchor in the general case is a good generalization to
have for this kind of library element. Center-aligning may have uses such as
when implementing a dropdown, for example.

[dpapad, 2017/04/19 21:41:55]

Ok, I kind of understand the generalization need better now. But I am also
realizing that |anchorPosition| semantics are under-documented as it stands.

1) The "position" suffix makes it sound as if it holds px values. Can we change
the suffix from "position" to "percent"? 
2) anchorPosition is a percent of which value, "end - start", "(end - start) /
2"? Needs to be explicitly stated.
3) Anchor position is basically an offset percentage.
  - What is the starting point where the offset is applied at? It took me a bit
to figure out that it is the mid point "(end-start)/2".
 - Can we rename |anchorPosition| to |anchorOffsetPercent|?
 - Can we provide examples? Specifically, see http://imgur.com/a/HWhcw for the
cases I had in mind (which X,Y combination achieves each case).

[calamity, 2017/04/24 05:20:54]

I'm not a fan of calling this percent, given that it's not out of 100. I've
renamed it to anchorConfig so that it's obvious that more reading is required.

I've added a description of how to achieve certain alignments using this API.

Another option, if you prefer, is to have an enum:

enum AnchorAlignment {
  START_OUTSIDE,
  START_INSIDE,
  CENTER,
  END_INSIDE,
  END_OUTSIDE,
}

which would cover all cases and be more understandable.

[dpapad, 2017/04/25 17:42:43]

If you think that will be cleaner, I am fine with it. I am not 100% sure how the
_INSIDE and _OUTSIDE cases can be differentiated though using a single enum. The
graph pasted above is how I currently understand the |anchorConfig| concept.

Regarding percent VS config, I realize now that both are not great names.
Anyway, I am not so worried about the names, if the code has enough explanations
(FWIW, anchorOffsetFactor might be more clear).

[calamity, 2017/04/26 03:15:50]

Changed to AnchorAlignment. I think this covers likely use cases and is easy to
understand.

+        (start - end + length) * anchorPosition / 2;
+    if (startPoint + length > max)
+      startPoint = end - length;
+    if (startPoint < min)
+      startPoint = start;
+    return startPoint;
+  },
+
+  /**
+   * Shows the menu anchored to the given box.
+   * @param {ShowConfig} config

[dpapad, 2017/04/14 22:06:31]

!ShowConfig

[calamity, 2017/04/19 05:31:07]

Done.

+   */
+  showAtPosition: function(config) {
+    /**
+     * @param {number|undefined} value
+     * @param {number} defaultValue
+     * @return {number}
+     */
+    var defaultIfUndefined = function(value, defaultValue) {
+      return value == undefined ? defaultValue : value;
+    };
+
+    var top = config.top;

[dpapad, 2017/04/14 22:06:31]

Lines 232-241 are basically creating a ShowConfig that has all optional values
populated. Can you package this functionality avoiding flattening? How about the
following?

Make a helper method |normalizeConfig| that takes the input config object and
returns the fully populated ShowConfig object.

var effectiveConfig = normalizeConfig(config);
Then use |effectiveConfig| for all subsequent calculations, no need to flatten
all 10 variables.

[calamity, 2017/04/19 05:31:07]

Done.

+    var left = config.left;
+    var bottom = top + defaultIfUndefined(config.height, 0);
+    var right = left + defaultIfUndefined(config.width, 0);
+    var anchorPositionX = defaultIfUndefined(config.anchorPositionX, 1);
+    var anchorPositionY = defaultIfUndefined(config.anchorPositionY, 1);
+    var minX = defaultIfUndefined(config.minX, 0);
+    var maxX = defaultIfUndefined(config.maxX, window.innerWidth);
+    var minY = defaultIfUndefined(config.minY, 0);
+    var maxY = defaultIfUndefined(config.maxY, window.innerHeight);
+
     this.boundClose_ = this.boundClose_ || function() {
       if (this.open)
         this.close();
     }.bind(this);
     window.addEventListener('resize', this.boundClose_);
     window.addEventListener('popstate', this.boundClose_);
 
     // Reset position to prevent previous values from affecting layout.
     this.style.left = '';
     this.style.right = '';
     this.style.top = '';
 
-    this.anchorElement_.scrollIntoViewIfNeeded();
     this.showModal();
 
-    var rect = this.anchorElement_.getBoundingClientRect();
-    if (getComputedStyle(this.anchorElement_).direction == 'rtl') {
-      var right = window.innerWidth - rect.left - this.offsetWidth;
-      this.style.right = right + 'px';
+    // Flip the X anchor in RTL.
+    var rtl = getComputedStyle(this).direction == 'rtl';
+    if (rtl)
+      anchorPositionX *= -1;
+
+    var menuLeft = this.getStartPointWithAnchor(
+        left, right, this.offsetWidth, anchorPositionX, minX, maxX);
+
+    if (rtl) {
+      var menuRight = window.innerWidth - menuLeft - this.offsetWidth;
+      this.style.right = menuRight + 'px';
     } else {
-      var left = rect.right - this.offsetWidth;
-      this.style.left = left + 'px';
+      this.style.left = menuLeft + 'px';
     }
 
-    // Attempt to show the menu starting from the top of the rectangle and
-    // extending downwards. If that does not fit within the window, fallback to
-    // starting from the bottom and extending upwards.
-    var top = rect.top + this.offsetHeight <= window.innerHeight ? rect.top :
-                                                                   rect.bottom -
-            this.offsetHeight - Math.max(rect.bottom - window.innerHeight, 0);
-
-    this.style.top = top + 'px';
+    var menuTop = this.getStartPointWithAnchor(
+        top, bottom, this.offsetHeight, anchorPositionY, minY, maxY);
+    this.style.top = menuTop + 'px';
   },
 });