From 201610028d85a0aff38829544464a96e24034454 Mon Sep 17 00:00:00 2001 From: Ole Rehmsen Date: Tue, 18 Dec 2018 16:56:10 +0100 Subject: [PATCH] Make gr-diff-selection work with native Shadow DOM With native Shadow DOM, the selections returned by document.getSelection() cannot reference the elements actually making up the diff because those live in the Shadow DOM of gr-diff. Instead, a shadowRoot.getSelection() method is offered that works as the pre-Shadow DOM Selection used to, but that one is only available from gr-diff. This change moves listening for the selectionchange event and retrieving the selection to gr-diff, which then delegates the extraction and normalization of the ranges to gr-diff-highlight (it's current location). This change also fixes an incompatibility with native Shadow DOM in gr-selection-action-box, where a call to parentElement cannot cross the Shadow DOM boundary and needs special handling to retrieve the Shadow Host. Together these changes fix creating range comments for native Shadow DOM. Bug: Issue 6372 Change-Id: I4bce21025ab5a4c06d77db096d6aa56e7840987b --- .../gr-diff-highlight/gr-diff-highlight.js | 39 ++++++++++--------- .../gr-diff-highlight_test.html | 35 ++--------------- .../app/elements/diff/gr-diff/gr-diff.js | 28 +++++++++++++ .../elements/diff/gr-diff/gr-diff_test.html | 24 ++++++++++++ .../gr-selection-action-box.js | 15 +++++-- 5 files changed, 87 insertions(+), 54 deletions(-) diff --git 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 index ff003835e8..12b30b2728 100644 --- 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 @@ -33,7 +33,6 @@ * @type {?HTMLElement} * */ _cachedDiffBuilder: Object, - isAttached: Boolean, }, listeners: { @@ -42,10 +41,6 @@ 'create-range-comment': '_createRangeComment', }, - observers: [ - '_enableSelectionObserver(loggedIn, isAttached)', - ], - get diffBuilder() { if (!this._cachedDiffBuilder) { this._cachedDiffBuilder = @@ -54,24 +49,30 @@ return this._cachedDiffBuilder; }, - _enableSelectionObserver(loggedIn, isAttached) { - if (loggedIn && isAttached) { - this.listen(document, 'selectionchange', '_handleSelectionChange'); - } else { - this.unlisten(document, 'selectionchange', '_handleSelectionChange'); - } - }, isRangeSelected() { return !!this.$$('gr-selection-action-box'); }, - _handleSelectionChange() { + /** + * Determines side/line/range for a DOM selection and shows a tooltip. + * + * With native shadow DOM, gr-diff-highlight cannot access a selection that + * references the DOM elements making up the diff because they are in the + * shadow DOM the gr-diff element. For this reason, we listen to the + * selectionchange event and retrieve the selection in gr-diff, and then + * call this method to process the Selection. + * + * @param {Selection} selection A DOM Selection living in the shadow DOM of + * the diff element. + */ + handleSelectionChange(selection) { // Can't use up or down events to handle selection started and/or ended in // in comment threads or outside of diff. // Debounce removeActionBox to give it a chance to react to click/tap. this._removeActionBoxDebounced(); - this.debounce('selectionChange', this._handleSelection, 200); + this.debounce( + 'selectionChange', () => this._handleSelection(selection), 200); }, _getThreadEl(e) { @@ -128,6 +129,7 @@ * Merges multiple ranges, accounts for triple click, accounts for * syntax highligh, convert native DOM Range objects to Gerrit concepts * (line, side, etc). + * @param {Selection} selection * @return {({ * start: { * node: Node, @@ -143,8 +145,7 @@ * } * })|null|!Object} */ - _getNormalizedRange() { - const selection = window.getSelection(); + _getNormalizedRange(selection) { const rangeCount = selection.rangeCount; if (rangeCount === 0) { return null; @@ -289,12 +290,12 @@ actionBox.placeBelow(range); }, - _handleSelection() { - const normalizedRange = this._getNormalizedRange(); + _handleSelection(selection) { + const normalizedRange = this._getNormalizedRange(selection); if (!normalizedRange) { return; } - const domRange = window.getSelection().getRangeAt(0); + const domRange = selection.getRangeAt(0); /** @type {?} */ const start = normalizedRange.start; if (!start) { diff --git 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 index 8e3c7b0a87..9ec2b2906a 100644 --- 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 @@ -158,33 +158,6 @@ limitations under the License. sandbox.restore(); }); - suite('selectionchange event handling', () => { - const emulateSelection = function() { - document.dispatchEvent(new CustomEvent('selectionchange')); - element.flushDebouncer('selectionChange'); - element.flushDebouncer('removeActionBox'); - }; - - setup(() => { - sandbox.stub(element, '_handleSelection'); - sandbox.stub(element, '_removeActionBox'); - }); - - test('enabled if logged in', () => { - element.loggedIn = true; - emulateSelection(); - assert.isTrue(element._handleSelection.called); - assert.isTrue(element._removeActionBox.called); - }); - - test('ignored if logged out', () => { - element.loggedIn = false; - emulateSelection(); - assert.isFalse(element._handleSelection.called); - assert.isFalse(element._removeActionBox.called); - }); - }); - suite('comment events', () => { let builder; @@ -293,7 +266,7 @@ limitations under the License. range.setStart(startNode, startOffset); range.setEnd(endNode, endOffset); selection.addRange(range); - element._handleSelection(); + element._handleSelection(selection); }; const getActionRange = () => @@ -400,12 +373,12 @@ limitations under the License. getRangeAtStub .onFirstCall().returns(startRange) .onSecondCall().returns(endRange); - sandbox.stub(window, 'getSelection').returns({ + const selection = { rangeCount: 2, getRangeAt: getRangeAtStub, removeAllRanges: sandbox.stub(), - }); - element._handleSelection(); + }; + element._handleSelection(selection); assert.isTrue(element.isRangeSelected()); assert.deepEqual(getActionRange(), { start_line: 119, diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index b134d4ef3a..f2606461c4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -252,6 +252,9 @@ * @type {?PolymerDomApi.ObserveHandle} */ _nodeObserver: Object, + + /** Set by Polymer. */ + isAttached: Boolean, }, behaviors: [ @@ -263,6 +266,10 @@ 'render-content': '_handleRenderContent', }, + observers: [ + '_enableSelectionObserver(loggedIn, isAttached)', + ], + attached() { this._observeNodes(); }, @@ -272,6 +279,27 @@ this._unobserveNodes(); }, + _enableSelectionObserver(loggedIn, isAttached) { + if (loggedIn && isAttached) { + this.listen(document, 'selectionchange', '_handleSelectionChange'); + } else { + this.unlisten(document, 'selectionchange', '_handleSelectionChange'); + } + }, + + _handleSelectionChange() { + // When using native shadow DOM, the selection returned by + // document.getSelection() cannot reference the actual DOM elements making + // up the diff, because they are in the shadow DOM of the gr-diff element. + // For this reason, we handle the selectionchange here, and pass the + // shadow DOM selection into gr-diff-highlight, where the corresponding + // range is determined and normalized. + const selection = this.root.getSelection ? + this.root.getSelection() : + document.getSelection(); + this.$.highlights.handleSelectionChange(selection); + }, + _observeNodes() { this._nodeObserver = Polymer.dom(this).observeNodes(info => { const addedThreadEls = info.addedNodes.filter(isThreadEl); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html index 99f2ded955..300807c2e9 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html @@ -48,6 +48,30 @@ limitations under the License. sandbox.restore(); }); + suite('selectionchange event handling', () => { + const emulateSelection = function() { + document.dispatchEvent(new CustomEvent('selectionchange')); + }; + + setup(() => { + element = fixture('basic'); + sandbox.stub(element.$.highlights, 'handleSelectionChange'); + }); + + test('enabled if logged in', () => { + element.loggedIn = true; + emulateSelection(); + assert.isTrue(element.$.highlights.handleSelectionChange.called); + }); + + test('ignored if logged out', () => { + element.loggedIn = false; + emulateSelection(); + assert.isFalse(element.$.highlights.handleSelectionChange.called); + }); + }); + + test('cancel', () => { element = fixture('basic'); const cancelStub = sandbox.stub(element.$.diffBuilder, 'cancel'); diff --git 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 index fa5c810b8a..abf8e73447 100644 --- 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 @@ -63,7 +63,7 @@ Polymer.dom.flush(); const rect = this._getTargetBoundingRect(el); const boxRect = this.$.tooltip.getBoundingClientRect(); - const parentRect = this.parentElement.getBoundingClientRect(); + const parentRect = this._getParentBoundingClientRect(); this.style.top = rect.top - parentRect.top - boxRect.height - 6 + 'px'; this.style.left = @@ -74,11 +74,18 @@ Polymer.dom.flush(); const rect = this._getTargetBoundingRect(el); const boxRect = this.$.tooltip.getBoundingClientRect(); - const parentRect = this.parentElement.getBoundingClientRect(); + const parentRect = this._getParentBoundingClientRect(); this.style.top = - rect.top - parentRect.top + boxRect.height - 6 + 'px'; + rect.top - parentRect.top + boxRect.height - 6 + 'px'; this.style.left = - rect.left - parentRect.left + (rect.width - boxRect.width) / 2 + 'px'; + rect.left - parentRect.left + (rect.width - boxRect.width) / 2 + 'px'; + }, + + _getParentBoundingClientRect() { + // With native shadow DOM, the parent is the shadow root, not the gr-diff + // element + const parent = this.parentElement || this.parentNode.host; + return parent.getBoundingClientRect(); }, _getTargetBoundingRect(el) {