Fix regression with the selection-action-box flickering
Change 314023 introduced a regression. The positioning of the action box
would happen before the tooltip is rendered and thus being initially
placed in the wrong position, which unfortunately would make the mouse
pointer hover over the tooltip, thus leading to an invalid selection
range, thus to the tooltip and action box being removed - and then
re-created, seen as a flicker by the user.
The actual fix here is to await the rendering of the tooltip before
positioning the action box.
On top of that we prevent the tooltip from interfering with the
selection.
And we also fix a small bug with `positionBelow` potentially ending up
in a wrong permanent state.
Google-Bug-Id: b/166482375
Change-Id: I9292b7f942fe8f026725988757780894f01bdb11
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
index 3e292fe..a47db20 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
@@ -408,6 +408,7 @@
range: Text | Element | Range
) {
if (startLine > 1) {
+ actionBox.positionBelow = false;
actionBox.placeAbove(range);
return;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
index 551889f..0f64d9e 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -18,7 +18,6 @@
import '../../shared/gr-tooltip/gr-tooltip';
import {GrTooltip} from '../../shared/gr-tooltip/gr-tooltip';
import {customElement, property} from '@polymer/decorators';
-import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-selection-action-box_html';
import {fireEvent} from '../../../utils/event-util';
@@ -56,8 +55,8 @@
this.addEventListener('mousedown', e => this._handleMouseDown(e));
}
- placeAbove(el: Text | Element | Range) {
- flush();
+ async placeAbove(el: Text | Element | Range) {
+ await this.$.tooltip.updateComplete;
const rect = this._getTargetBoundingRect(el);
const boxRect = this.$.tooltip.getBoundingClientRect();
const parentRect = this._getParentBoundingClientRect();
@@ -70,8 +69,8 @@
}px`;
}
- placeBelow(el: Text | Element | Range) {
- flush();
+ async placeBelow(el: Text | Element | Range) {
+ await this.$.tooltip.updateComplete;
const rect = this._getTargetBoundingRect(el);
const boxRect = this.$.tooltip.getBoundingClientRect();
const parentRect = this._getParentBoundingClientRect();
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
index 24d63b3..558742a 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_html.ts
@@ -23,6 +23,9 @@
font-family: var(--font-family);
position: absolute;
white-space: nowrap;
+ /* This prevents the mouse over the tooltip from interfering with the
+ selection. */
+ pointer-events: none;
}
</style>
<gr-tooltip
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
index 81cf0d6..c978c37 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.js
@@ -83,35 +83,35 @@
{width: 10, height: 10});
});
- test('placeAbove for Element argument', () => {
- element.placeAbove(target);
+ test('placeAbove for Element argument', async () => {
+ await element.placeAbove(target);
assert.equal(element.style.top, '25px');
assert.equal(element.style.left, '72px');
});
- test('placeAbove for Text Node argument', () => {
- element.placeAbove(target.firstChild);
+ test('placeAbove for Text Node argument', async () => {
+ await element.placeAbove(target.firstChild);
assert.equal(element.style.top, '25px');
assert.equal(element.style.left, '72px');
});
- test('placeBelow for Element argument', () => {
- element.placeBelow(target);
+ test('placeBelow for Element argument', async () => {
+ await 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);
+ test('placeBelow for Text Node argument', async () => {
+ await element.placeBelow(target.firstChild);
assert.equal(element.style.top, '45px');
assert.equal(element.style.left, '72px');
});
- test('uses document.createRange', () => {
+ test('uses document.createRange', async () => {
sinon.spy(document, 'createRange');
element._getTargetBoundingRect.restore();
sinon.spy(element, '_getTargetBoundingRect');
- element.placeAbove(target.firstChild);
+ await element.placeAbove(target.firstChild);
assert.isTrue(document.createRange.called);
});
});
