Fix 'press c to comment' on first line of diff

In the file list, divs are displayed as block elements, so that their contents can be scrollable. It's impossible to fix this with a z-index. Instead, this change shows the tooltip below rather than above in the case that the first line is part of the selection range. Bug: Issue 7320 Change-Id: I29a4ebc619cd0a5523fb4900abccb491cf9a5194
2017-11-16 12:46:46 -08:00
parent 943579dcb9
commit 060763157c
6 changed files with 115 additions and 42 deletions
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -251,6 +251,22 @@
      };
    },

+    /**
+     * The only line in which add a comment tooltip is cut off is the first
+     * line. Even if there is a collapsed section, The first visible line is
+     * in the position where the second line would have been, if not for the
+     * collapsed section, so don't need to worry about this case for
+     * positioning the tooltip.
+     */
+    _positionActionBox(actionBox, startLine, range) {
+      if (startLine > 1) {
+        actionBox.placeAbove(range);
+        return;
+      }
+      actionBox.positionBelow = true;
+      actionBox.placeBelow(range);
+    },
+
    _handleSelection() {
      const normalizedRange = this._getNormalizedRange();
      if (!normalizedRange) {
@@ -285,17 +301,18 @@
      };
      actionBox.side = start.side;
      if (start.line === end.line) {
-        actionBox.placeAbove(domRange);
+        this._positionActionBox(actionBox, start.line, domRange);
      } else if (start.node instanceof Text) {
        if (start.column) {
-          actionBox.placeAbove(start.node.splitText(start.column));
+          this._positionActionBox(actionBox, start.line,
+              start.node.splitText(start.column));
        }
        start.node.parentElement.normalize(); // Undo splitText from above.
      } else if (start.node.classList.contains('content') &&
-                 start.node.firstChild) {
-        actionBox.placeAbove(start.node.firstChild);
+          start.node.firstChild) {
+        this._positionActionBox(actionBox, start.line, start.node.firstChild);
      } else {
-        actionBox.placeAbove(start.node);
+        this._positionActionBox(actionBox, start.line, start.node);
      }
    },

--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
@@ -37,6 +37,27 @@ limitations under the License.
    <gr-diff-highlight>
      <table id="diffTable">

+        <tbody class="section both">
+           <tr class="diff-row side-by-side" left-type="both" right-type="both">
+            <td class="left lineNum" data-value="1"></td>
+            <td class="content both"><div class="contentText">[1] Nam cum ad me in Cumanum salutandi causa uterque venisset,</div></td>
+            <td class="right lineNum" data-value="1"></td>
+            <td class="content both"><div class="contentText">[1] Nam cum ad me in Cumanum salutandi causa uterque</div></td>
+          </tr>
+        </tbody>
+
+        <tbody class="section delta">
+          <tr class="diff-row side-by-side" left-type="remove" right-type="add">
+            <td class="left lineNum" data-value="2"></td>
+            <!-- Next tag is formatted to eliminate zero-length text nodes. -->
+            <td class="content remove"><div class="contentText">na💢ti <hl class="foo">te, inquit</hl>, sumus <hl class="bar">aliquando</hl> otiosum, <hl>certe</hl> a <hl><span class="tab-indicator" style="tab-size:8;">	</span></hl>udiam, <hl>quid</hl> sit, <span class="tab-indicator" style="tab-size:8;">	</span>quod <hl>Epicurum</hl></div></td>
+            <td class="right lineNum" data-value="2"></td>
+            <!-- Next tag is formatted to eliminate zero-length text nodes. -->
+            <td class="content add"><div class="contentText">nacti , <hl>,</hl> sumus <hl><span class="tab-indicator" style="tab-size:8;">	</span></hl> otiosum,  <span class="tab-indicator" style="tab-size:8;">	</span> audiam,  sit, quod</div></td>
+          </tr>
+        </tbody>
+
+
        <tbody class="section both">
          <tr class="diff-row side-by-side" left-type="both" right-type="both">
            <td class="left lineNum" data-value="138"></td>
@@ -253,6 +274,7 @@ limitations under the License.
        contentStubs = [];
        stub('gr-selection-action-box', {
          placeAbove: sandbox.stub(),
+          placeBelow: sandbox.stub(),
        });
        diff = element.querySelector('#diffTable');
        builder = {
@@ -270,9 +292,29 @@ limitations under the License.
        window.getSelection().removeAllRanges();
      });

+      test('single first line', () => {
+        const content = stubContent(1, 'right');
+        sandbox.spy(element, '_positionActionBox');
+        emulateSelection(content.firstChild, 5, content.firstChild, 12);
+        const actionBox = element.$$('gr-selection-action-box');
+        assert.isTrue(actionBox.positionBelow);
+      });
+
+      test('multiline starting on first line', () => {
+        const startContent = stubContent(1, 'right');
+        const endContent = stubContent(2, 'right');
+        sandbox.spy(element, '_positionActionBox');
+        emulateSelection(
+            startContent.firstChild, 10, endContent.lastChild, 7);
+        const actionBox = element.$$('gr-selection-action-box');
+        assert.isTrue(actionBox.positionBelow);
+      });
+
      test('single line', () => {
        const content = stubContent(138, 'left');
+        sandbox.spy(element, '_positionActionBox');
        emulateSelection(content.firstChild, 5, content.firstChild, 12);
+        const actionBox = element.$$('gr-selection-action-box');
        assert.isTrue(element.isRangeSelected());
        assert.deepEqual(getActionRange(), {
          startLine: 138,
@@ -281,14 +323,18 @@ limitations under the License.
          endChar: 12,
        });
        assert.equal(getActionSide(), 'left');
+        assert.notOk(actionBox.positionBelow);
      });

      test('multiline', () => {
        const startContent = stubContent(119, 'right');
        const endContent = stubContent(120, 'right');
+        sandbox.spy(element, '_positionActionBox');
        emulateSelection(
            startContent.firstChild, 10, endContent.lastChild, 7);
        assert.isTrue(element.isRangeSelected());
+        const actionBox = element.$$('gr-selection-action-box');
+
        assert.deepEqual(getActionRange(), {
          startLine: 119,
          startChar: 10,
@@ -296,6 +342,7 @@ limitations under the License.
          endChar: 36,
        });
        assert.equal(getActionSide(), 'right');
+        assert.notOk(actionBox.positionBelow);
      });

      test('multiple ranges aka firefox implementation', () => {
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.html
@@ -17,34 +17,24 @@ limitations under the License.
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
 <link rel="import" href="../../../styles/shared-styles.html">
+<link rel="import" href="../../shared/gr-tooltip/gr-tooltip.html">

 <dom-module id="gr-selection-action-box">
  <template>
    <style include="shared-styles">
      :host {
-        --gr-arrow-size: .65em;
-
-        background-color: rgba(22, 22, 22, .9);
-        border-radius: 3px;
-        color: #fff;
        cursor: pointer;
        font-family: var(--font-family);
-        padding: .5em .75em;
        position: absolute;
        white-space: nowrap;
      }
-      .arrow {
-        border: var(--gr-arrow-size) solid transparent;
-        border-top: var(--gr-arrow-size) solid rgba(22, 22, 22, 0.9);
-        height: 0;
-        left: calc(50% - var(--gr-arrow-size));
-        margin-top: .5em;
-        position: absolute;
-        width: 0;
+      #tooltip {
+        --tooltip-background-color: rgba(22, 22, 22, .9);
      }
    </style>
-    Press <strong>c</strong> to comment.
-    <div class="arrow"></div>
+    <gr-tooltip id="tooltip"
+        text="press c to comment"
+        position-below="[[positionBelow]]"></gr-tooltip>
  </template>
  <script src="gr-selection-action-box.js"></script>
 </dom-module>
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
@@ -37,6 +37,7 @@
          endChar: NaN,
        },
      },
+      positionBelow: Boolean,
      side: {
        type: String,
        value: '',
@@ -58,7 +59,7 @@
    placeAbove(el) {
      Polymer.dom.flush();
      const rect = this._getTargetBoundingRect(el);
-      const boxRect = this.getBoundingClientRect();
+      const boxRect = this.$.tooltip.getBoundingClientRect();
      const parentRect = this.parentElement.getBoundingClientRect();
      this.style.top =
          rect.top - parentRect.top - boxRect.height - 6 + 'px';
@@ -66,6 +67,17 @@
          rect.left - parentRect.left + (rect.width - boxRect.width) / 2 + 'px';
    },

+    placeBelow(el) {
+      Polymer.dom.flush();
+      const rect = this._getTargetBoundingRect(el);
+      const boxRect = this.$.tooltip.getBoundingClientRect();
+      const parentRect = this.parentElement.getBoundingClientRect();
+      this.style.top =
+          rect.top - parentRect.top + boxRect.height - 6 + 'px';
+      this.style.left =
+          rect.left - parentRect.left + (rect.width - boxRect.width) / 2 + 'px';
+    },
+
    _getTargetBoundingRect(el) {
      let rect;
      if (el instanceof Text) {
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html
@@ -38,15 +38,17 @@ limitations under the License.
  suite('gr-selection-action-box', () => {
    let container;
    let element;
+    let sandbox;

    setup(() => {
      container = fixture('basic');
      element = container.querySelector('gr-selection-action-box');
-      sinon.stub(element, 'fire');
+      sandbox = sinon.sandbox.create();
+      sandbox.stub(element, 'fire');
    });

    teardown(() => {
-      element.fire.restore();
+      sandbox.restore();
    });

    test('ignores regular keys', () => {
@@ -65,10 +67,10 @@ limitations under the License.
      setup(() => {
        e = {
          button: 0,
-          preventDefault: sinon.stub(),
-          stopPropagation: sinon.stub(),
+          preventDefault: sandbox.stub(),
+          stopPropagation: sandbox.stub(),
        };
-        sinon.stub(element, '_fireCreateComment');
+        sandbox.stub(element, '_fireCreateComment');
      });

      test('event handled if main button', () => {
@@ -107,20 +109,14 @@ limitations under the License.

      setup(() => {
        target = container.querySelector('.target');
-        sinon.stub(container, 'getBoundingClientRect').returns(
+        sandbox.stub(container, 'getBoundingClientRect').returns(
            {top: 1, bottom: 2, left: 3, right: 4, width: 50, height: 6});
-        sinon.stub(element, '_getTargetBoundingRect').returns(
+        sandbox.stub(element, '_getTargetBoundingRect').returns(
            {top: 42, bottom: 20, left: 30, right: 40, width: 100, height: 60});
-        sinon.stub(element, 'getBoundingClientRect').returns(
+        sandbox.stub(element.$.tooltip, 'getBoundingClientRect').returns(
            {width: 10, height: 10});
      });

-      teardown(() => {
-        element.getBoundingClientRect.restore();
-        container.getBoundingClientRect.restore();
-        element._getTargetBoundingRect.restore();
-      });
-
      test('placeAbove for Element argument', () => {
        element.placeAbove(target);
        assert.equal(element.style.top, '25px');
@@ -133,13 +129,24 @@ limitations under the License.
        assert.equal(element.style.left, '72px');
      });

+      test('placeBelow for Element argument', () => {
+        element.placeBelow(target);
+        assert.equal(element.style.top, '45px');
+        assert.equal(element.style.left, '72px');
+      });
+
+      test('placeBelow for Text Node argument', () => {
+        element.placeBelow(target.firstChild);
+        assert.equal(element.style.top, '45px');
+        assert.equal(element.style.left, '72px');
+      });
+
      test('uses document.createRange', () => {
-        sinon.spy(document, 'createRange');
+        sandbox.spy(document, 'createRange');
        element._getTargetBoundingRect.restore();
-        sinon.spy(element, '_getTargetBoundingRect');
+        sandbox.spy(element, '_getTargetBoundingRect');
        element.placeAbove(target.firstChild);
        assert.isTrue(document.createRange.called);
-        document.createRange.restore();
      });
    });
  });
--- a/polygerrit-ui/app/elements/shared/gr-tooltip/gr-tooltip.html
+++ b/polygerrit-ui/app/elements/shared/gr-tooltip/gr-tooltip.html
@@ -24,7 +24,7 @@ limitations under the License.
        --gr-tooltip-arrow-size: .5em;
        --gr-tooltip-arrow-center-offset: 0;

-        background-color: #333;
+        background-color: var(--tooltip-background-color, #333);
        box-shadow: 0 1px 3px rgba(0, 0, 0, .3);
        color: #fff;
        font-size: .75rem;
@@ -52,11 +52,11 @@ limitations under the License.
        width: 0;
      }
      .arrowPositionAbove {
-        border-top: var(--gr-tooltip-arrow-size) solid #333;
+        border-top: var(--gr-tooltip-arrow-size) solid var(--tooltip-background-color, #333);
        bottom: -var(--gr-tooltip-arrow-size);
      }
      .arrowPositionBelow {
-        border-bottom: var(--gr-tooltip-arrow-size) solid #333;
+        border-bottom: var(--gr-tooltip-arrow-size) solid var(--tooltip-background-color, #333);
        top: -var(--gr-tooltip-arrow-size);
      }
    </style>