From 5fd619fbe2177e57ecf001cf96b2de3360b9117d Mon Sep 17 00:00:00 2001
From: David Manthey <david.manthey@kitware.com>
Date: Fri, 16 Mar 2018 10:17:18 -0400
Subject: [PATCH] Improve setting widget position.

The widget position can be any css-style position, or an x, y coordinate
for the map.  If you change the position using the same attributes as
were used previously, it behaved as expected.  However, if different
attributes were specified, surprising results could occur.

For instance `widget.position({left: 10, top: 10})` followed by
`widget.position({right: 20, top: 10})`, instead of switching the css
from using `left` to `right`, combined them, so that functionally, the
widget was specifying `{left: 10, right: 20, top: 10}`.  Although this
could be avoided by explicitly calling `widget.position({left: null,
right: 20, top: 10})` and could change just one coordinate via call like
`widget.position({top: 11})`, this is surprising behavior.

With this change, setting a widget's position clears the old position
attributes if they are not explicitly set.
---
 src/ui/widget.js      |  9 ++++++++-
 tests/cases/widget.js | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/ui/widget.js b/src/ui/widget.js
index ebbc943e4f..42d4f001df 100644
--- a/src/ui/widget.js
+++ b/src/ui/widget.js
@@ -1,5 +1,6 @@
 var inherit = require('../inherit');
 var sceneObject = require('../sceneObject');
+var $ = require('jquery');
 
 /**
  * @typedef {object} geo.gui.widget.position
@@ -148,11 +149,17 @@ var widget = function (arg) {
   this.position = function (pos, actualValue) {
     if (pos !== undefined) {
       this.layer().geoOff(geo_event.pan, m_this.repositionEvent);
+      var clearPosition = {};
+      for (var attr in m_position) {
+        if (m_position.hasOwnProperty(attr)) {
+          clearPosition[attr] = null;
+        }
+      }
       m_position = pos;
       if (m_position.hasOwnProperty('x') && m_position.hasOwnProperty('y')) {
         this.layer().geoOn(geo_event.pan, m_this.repositionEvent);
       }
-      this.reposition();
+      this.reposition($.extend(clearPosition, m_this.position()));
       return this;
     }
     if (m_position.hasOwnProperty('x') && m_position.hasOwnProperty('y') && !actualValue) {
diff --git a/tests/cases/widget.js b/tests/cases/widget.js
index b6031297e6..60e105aeec 100644
--- a/tests/cases/widget.js
+++ b/tests/cases/widget.js
@@ -124,6 +124,18 @@ describe('geo.gui.widget', function () {
       map.pan({x: -5, y: 20});
       expect(closeToEqual(widget.position(), {left: 280.87, top: 165.85, right: null, bottom: null})).toBe(true);
       expect(widget.position(undefined, true)).toEqual({x: -3, y: 3});
+      /* test that position updates the canvas as we expect */
+      var div = document.createElement('div');
+      widget = geo.gui.widget({layer: layer});
+      widget.canvas(div).reposition();
+      expect(div.style.left).toBe('0px');
+      expect(div.style.right).toBe('');
+      widget.position({right: 10, bottom: 20});
+      expect(div.style.left).toBe('');
+      expect(div.style.right).toBe('10px');
+      widget.position({right: '10%', bottom: 20});
+      expect(div.style.left).toBe('');
+      expect(div.style.right).toBe('10%');
     });
     it('reposition and repositionEvent', function () {
       map = createMap();