[cr-action-menu] Allow configurable anchors.

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

Index: ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
diff --git a/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js b/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
index 42b00655dd9e1ba6b45a7acd27a3892e9ef6d2fa..eea5b77ea414752ccdd53f3af71ab1418ddf76b9 100644
--- a/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
+++ b/ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js
@@ -2,6 +2,22 @@
 // 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',
@@ -151,7 +167,9 @@ Polymer({
     // 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.

   },
 
@@ -161,6 +179,67 @@ Polymer({
    */
   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();
@@ -173,25 +252,25 @@ Polymer({
     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';
   },
 });