From ce2fda54b3b7c6e700300e76a8c1751f9c36cd80 Mon Sep 17 00:00:00 2001 From: Ben Rohlfs Date: Fri, 12 Mar 2021 15:48:32 +0100 Subject: [PATCH] Remove remaining usage of LegacyElementMixin functionality Change-Id: I84eada80e2fb0dfbc0af6025d16722e8b52e75f6 --- .../gr-reply-dialog-it_test.js | 30 ++- .../change/gr-reply-dialog/gr-reply-dialog.ts | 6 +- .../gr-reply-dialog/gr-reply-dialog_test.js | 196 +++++++++--------- .../gr-diff-builder-element.ts | 5 +- .../gr-diff-builder-element_test.js | 2 +- .../diff/gr-diff-host/gr-diff-host_test.js | 3 +- .../gr-diff-processor_test.js | 2 - .../app/elements/diff/gr-diff/gr-diff.ts | 12 +- .../app/elements/diff/gr-diff/gr-diff_test.js | 6 +- .../gr-endpoint-decorator.ts | 20 +- .../gr-hovercard/gr-hovercard-behavior.ts | 1 - 11 files changed, 140 insertions(+), 143 deletions(-) diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js index c16ed124c9..5f35fd379f 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js @@ -18,7 +18,6 @@ import '../../../test/common-test-setup-karma.js'; import {resetPlugins, stubRestApi} from '../../../test/test-utils.js'; import './gr-reply-dialog.js'; -import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js'; import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader.js'; import {_testOnly_initGerritPluginApi} from '../../shared/gr-js-api-interface/gr-gerrit.js'; @@ -104,7 +103,7 @@ suite('gr-reply-dialog-it tests', () => { assert.isTrue(sendStub.called); }); - test('lgtm plugin', done => { + test('lgtm plugin', async () => { resetPlugins(); pluginApi.install(plugin => { const replyApi = plugin.changeReply(); @@ -121,20 +120,17 @@ suite('gr-reply-dialog-it tests', () => { element = basicFixture.instantiate(); setupElement(element); getPluginLoader().loadPlugins([]); - getPluginLoader().awaitPluginsLoaded() - .then(() => { - flush(() => { - const textarea = element.$.textarea.getNativeTextarea(); - textarea.value = 'LGTM'; - textarea.dispatchEvent(new CustomEvent( - 'input', {bubbles: true, composed: true})); - const labelScoreRows = dom(element.$.labelScores.root) - .querySelector('gr-label-score-row[name="Code-Review"]'); - const selectedBtn = dom(labelScoreRows.root) - .querySelector('gr-button[data-value="+1"].iron-selected'); - assert.isOk(selectedBtn); - done(); - }); - }); + await getPluginLoader().awaitPluginsLoaded(); + await flush(); + const textarea = element.$.textarea.getNativeTextarea(); + textarea.value = 'LGTM'; + textarea.dispatchEvent(new CustomEvent( + 'input', {bubbles: true, composed: true})); + await flush(); + const labelScoreRows = element.$.labelScores.shadowRoot + .querySelector('gr-label-score-row[name="Code-Review"]'); + const selectedBtn = labelScoreRows.shadowRoot + .querySelector('gr-button[data-value="+1"].iron-selected'); + assert.isOk(selectedBtn); }); }); diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts index d11ad48529..319dc8fec9 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts @@ -757,13 +757,13 @@ export class GrReplyDialog extends KeyboardShortcutMixin( } if (section === FocusTarget.BODY) { const textarea = this.$.textarea; - textarea.async(() => textarea.getNativeTextarea().focus()); + setTimeout(() => textarea.getNativeTextarea().focus()); } else if (section === FocusTarget.REVIEWERS) { const reviewerEntry = this.$.reviewers.focusStart; - reviewerEntry.async(() => reviewerEntry.focus()); + setTimeout(() => reviewerEntry.focus()); } else if (section === FocusTarget.CCS) { const ccEntry = this.$.ccs.focusStart; - ccEntry.async(() => ccEntry.focus()); + setTimeout(() => ccEntry.focus()); } } diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js index 8d59bc82e3..d0d40ca401 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.js @@ -609,7 +609,7 @@ suite('gr-reply-dialog tests', () => { return false; } - function testConfirmationDialog(done, cc) { + async function testConfirmationDialog(cc) { const yesButton = element .shadowRoot .querySelector('.reviewerConfirmationButtons gr-button:first-child'); @@ -651,95 +651,90 @@ suite('gr-reply-dialog tests', () => { element._pendingConfirmationDetails); } - observer - .then(() => { - assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay)); - observer = overlayObserver('closed'); - const expected = 'Group name has 10 members'; - assert.notEqual( - element.$.reviewerConfirmationOverlay.innerText - .indexOf(expected), - -1); - MockInteractions.tap(noButton); // close the overlay - return observer; - }).then(() => { - assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay)); + await observer; + assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay)); + observer = overlayObserver('closed'); + const expected = 'Group name has 10 members'; + assert.notEqual( + element.$.reviewerConfirmationOverlay.innerText + .indexOf(expected), + -1); + MockInteractions.tap(noButton); // close the overlay - // We should be focused on account entry input. - assert.isTrue( - isFocusInsideElement( - element.$.reviewers.$.entry.$.input.$.input - ) - ); + await observer; + assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay)); - // No reviewer/CC should have been added. - assert.equal(element.$.ccs.additions().length, 0); - assert.equal(element.$.reviewers.additions().length, 0); + // We should be focused on account entry input. + assert.isTrue( + isFocusInsideElement( + element.$.reviewers.$.entry.$.input.$.input + ) + ); - // Reopen confirmation dialog. - observer = overlayObserver('opened'); - if (cc) { - element._ccPendingConfirmation = { - group, - count: 10, - }; - } else { - element._reviewerPendingConfirmation = { - group, - count: 10, - }; - } - return observer; - }) - .then(() => { - assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay)); - observer = overlayObserver('closed'); - MockInteractions.tap(yesButton); // Confirm the group. - return observer; - }) - .then(() => { - assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay)); - const additions = cc ? - element.$.ccs.additions() : - element.$.reviewers.additions(); - assert.deepEqual( - additions, - [ - { - group: { - id: 'id', - name: 'name', - confirmed: true, - _group: true, - _pendingAdd: true, - }, - }, - ]); + // No reviewer/CC should have been added. + assert.equal(element.$.ccs.additions().length, 0); + assert.equal(element.$.reviewers.additions().length, 0); - // We should be focused on account entry input. - if (cc) { - assert.isTrue( - isFocusInsideElement( - element.$.ccs.$.entry.$.input.$.input - ) - ); - } else { - assert.isTrue( - isFocusInsideElement( - element.$.reviewers.$.entry.$.input.$.input - ) - ); - } - }) - .then(done); + // Reopen confirmation dialog. + observer = overlayObserver('opened'); + if (cc) { + element._ccPendingConfirmation = { + group, + count: 10, + }; + } else { + element._reviewerPendingConfirmation = { + group, + count: 10, + }; + } + + await observer; + assert.isTrue(isVisible(element.$.reviewerConfirmationOverlay)); + observer = overlayObserver('closed'); + MockInteractions.tap(yesButton); // Confirm the group. + + await observer; + assert.isFalse(isVisible(element.$.reviewerConfirmationOverlay)); + const additions = cc ? + element.$.ccs.additions() : + element.$.reviewers.additions(); + assert.deepEqual( + additions, + [ + { + group: { + id: 'id', + name: 'name', + confirmed: true, + _group: true, + _pendingAdd: true, + }, + }, + ]); + + // We should be focused on account entry input. + if (cc) { + assert.isTrue( + isFocusInsideElement( + element.$.ccs.$.entry.$.input.$.input + ) + ); + } else { + assert.isTrue( + isFocusInsideElement( + element.$.reviewers.$.entry.$.input.$.input + ) + ); + } } - test('cc confirmation', done => { - testConfirmationDialog(done, true); + test('cc confirmation', async () => { + testConfirmationDialog(true); }); - test('reviewer confirmation', done => { - testConfirmationDialog(done, false); + test('reviewer confirmation', async () => { + testConfirmationDialog(false); }); test('_getStorageLocation', () => { @@ -887,42 +882,37 @@ suite('gr-reply-dialog tests', () => { assert.isFalse(filter({group: cc2})); }); - test('_focusOn', () => { + test('_focusOn', async () => { sinon.spy(element, '_chooseFocusTarget'); - flush(); - const textareaStub = sinon.stub(element.$.textarea, 'async'); - const reviewerEntryStub = sinon.stub(element.$.reviewers.focusStart, - 'async'); - const ccStub = sinon.stub(element.$.ccs.focusStart, 'async'); element._focusOn(); + await flush(); assert.equal(element._chooseFocusTarget.callCount, 1); - assert.deepEqual(textareaStub.callCount, 1); - assert.deepEqual(reviewerEntryStub.callCount, 0); - assert.deepEqual(ccStub.callCount, 0); + assert.equal(element.shadowRoot.activeElement.tagName, 'GR-TEXTAREA'); + assert.equal(element.shadowRoot.activeElement.id, 'textarea'); element._focusOn(element.FocusTarget.ANY); + await flush(); assert.equal(element._chooseFocusTarget.callCount, 2); - assert.deepEqual(textareaStub.callCount, 2); - assert.deepEqual(reviewerEntryStub.callCount, 0); - assert.deepEqual(ccStub.callCount, 0); + assert.equal(element.shadowRoot.activeElement.tagName, 'GR-TEXTAREA'); + assert.equal(element.shadowRoot.activeElement.id, 'textarea'); element._focusOn(element.FocusTarget.BODY); + await flush(); assert.equal(element._chooseFocusTarget.callCount, 2); - assert.deepEqual(textareaStub.callCount, 3); - assert.deepEqual(reviewerEntryStub.callCount, 0); - assert.deepEqual(ccStub.callCount, 0); + assert.equal(element.shadowRoot.activeElement.tagName, 'GR-TEXTAREA'); + assert.equal(element.shadowRoot.activeElement.id, 'textarea'); element._focusOn(element.FocusTarget.REVIEWERS); + await flush(); assert.equal(element._chooseFocusTarget.callCount, 2); - assert.deepEqual(textareaStub.callCount, 3); - assert.deepEqual(reviewerEntryStub.callCount, 1); - assert.deepEqual(ccStub.callCount, 0); + assert.equal(element.shadowRoot.activeElement.tagName, 'GR-ACCOUNT-LIST'); + assert.equal(element.shadowRoot.activeElement.id, 'reviewers'); element._focusOn(element.FocusTarget.CCS); + await flush(); assert.equal(element._chooseFocusTarget.callCount, 2); - assert.deepEqual(textareaStub.callCount, 3); - assert.deepEqual(reviewerEntryStub.callCount, 1); - assert.deepEqual(ccStub.callCount, 1); + assert.equal(element.shadowRoot.activeElement.tagName, 'GR-ACCOUNT-LIST'); + assert.equal(element.shadowRoot.activeElement.id, 'ccs'); }); test('_chooseFocusTarget', () => { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts index 4c10d5cf5f..aed52b9aac 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts @@ -168,8 +168,9 @@ export class GrDiffBuilderElement extends LegacyElementMixin(PolymerElement) { super.disconnectedCallback(); } - get diffElement() { - return this.queryEffectiveChildren('#diffTable') as HTMLTableElement; + get diffElement(): HTMLTableElement { + // Not searching in shadowRoot, because the diff table is slotted! + return this.querySelector('#diffTable') as HTMLTableElement; } _computeLeftCoverageRanges(coverageRanges: CoverageRange[]) { diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js index 0ae0e84cb6..7c01a9561c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js @@ -843,7 +843,7 @@ suite('gr-diff-builder tests', () => { }, ]; element = basicFixture.instantiate(); - outputEl = element.queryEffectiveChildren('#diffTable'); + outputEl = element.querySelector('#diffTable'); keyLocations = {left: {}, right: {}}; sinon.stub(element, '_getDiffBuilder').callsFake(() => { const builder = new GrDiffBuilderSideBySide({content}, prefs, outputEl); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js index 53a240488d..d31d934fcf 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js @@ -102,8 +102,7 @@ suite('gr-diff-host tests', () => { threadEls[0].dispatchEvent( new CustomEvent('thread-discard', {detail: {rootId: 4711}})); - const attachedThreads = element.queryAllEffectiveChildren( - 'gr-comment-thread'); + const attachedThreads = element.querySelectorAll('gr-comment-thread'); assert.equal(attachedThreads.length, 1); assert.equal(attachedThreads[0].comments[0].id, 42); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js index 5ecc96263e..0bb5eac54c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js @@ -750,7 +750,6 @@ suite('gr-diff-processor tests', () => { ], }; const content = _.times(200, _.constant(contentRow)); - sinon.stub(element, 'async'); element._isScrolling = true; element.process(content); // Just the files group - no more processing during scrolling. @@ -770,7 +769,6 @@ suite('gr-diff-processor tests', () => { ], }; const content = _.times(200, _.constant(contentRow)); - sinon.stub(element, 'async'); element.process(content, true); assert.equal(element.groups.length, 2); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts index 5214c64be4..eeaa72f27e 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts @@ -282,6 +282,9 @@ export class GrDiff extends LegacyElementMixin(PolymerElement) { @property({type: Array}) layers?: DiffLayer[]; + @property({type: Boolean}) + isAttached = false; + private renderDiffTableTask?: DelayedTask; constructor() { @@ -298,10 +301,12 @@ export class GrDiff extends LegacyElementMixin(PolymerElement) { connectedCallback() { super.connectedCallback(); this._observeNodes(); + this.isAttached = true; } /** @override */ disconnectedCallback() { + this.isAttached = false; this.renderDiffTableTask?.cancel(); this._unobserveIncrementalNodes(); this._unobserveNodes(); @@ -325,12 +330,7 @@ export class GrDiff extends LegacyElementMixin(PolymerElement) { } @observe('loggedIn', 'isAttached') - _enableSelectionObserver(loggedIn: boolean, isAttached?: boolean) { - // Polymer 2: check for undefined - if ([loggedIn, isAttached].includes(undefined)) { - return; - } - + _enableSelectionObserver(loggedIn: boolean, isAttached: boolean) { if (loggedIn && isAttached) { document.addEventListener( '-shadow-selectionchange', diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js index 1827def8da..175ee7937a 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js @@ -52,15 +52,17 @@ suite('gr-diff tests', () => { sinon.stub(element.$.highlights, 'handleSelectionChange'); }); - test('enabled if logged in', () => { + test('enabled if logged in', async () => { element.loggedIn = true; emulateSelection(); + await flush(); assert.isTrue(element.$.highlights.handleSelectionChange.called); }); - test('ignored if logged out', () => { + test('ignored if logged out', async () => { element.loggedIn = false; emulateSelection(); + await flush(); assert.isFalse(element.$.highlights.handleSelectionChange.called); }); }); diff --git a/polygerrit-ui/app/elements/plugins/gr-endpoint-decorator/gr-endpoint-decorator.ts b/polygerrit-ui/app/elements/plugins/gr-endpoint-decorator/gr-endpoint-decorator.ts index 4290e75b9d..39aca13d4e 100644 --- a/polygerrit-ui/app/elements/plugins/gr-endpoint-decorator/gr-endpoint-decorator.ts +++ b/polygerrit-ui/app/elements/plugins/gr-endpoint-decorator/gr-endpoint-decorator.ts @@ -68,7 +68,9 @@ export class GrEndpointDecorator extends LegacyElementMixin(PolymerElement) { return this._initProperties( el, plugin, - this.getContentChildren().find(el => el.nodeName !== 'GR-ENDPOINT-PARAM') + // The direct children are slotted into , so this is identical to + // this.shadowRoot.querySelector('slot').assignedElements()[0]. + this.firstElementChild ).then(el => { const slotEl = slot ? this.querySelector(`gr-endpoint-slot[name=${slot}]`) @@ -82,9 +84,16 @@ export class GrEndpointDecorator extends LegacyElementMixin(PolymerElement) { }); } + // As of March 2021 the only known plugin that replaces an endpoint instead + // of decorating it is codemirror_editor. _initReplacement(name: string, plugin: PluginApi): Promise { - this.getContentChildNodes() + // The direct children are slotted into , so they are identical to + // this.shadowRoot.querySelector('slot').assignedElements(). + const directChildren = [...this.childNodes]; + const shadowChildren = [...(this.shadowRoot?.childNodes ?? [])]; + [...directChildren, ...shadowChildren] .filter(node => node.nodeName !== 'GR-ENDPOINT-PARAM') + .filter(node => node.nodeName !== 'SLOT') .forEach(node => (node as ChildNode).remove()); const el = document.createElement(name); return this._initProperties(el, plugin).then((el: HTMLElement) => @@ -99,13 +108,16 @@ export class GrEndpointDecorator extends LegacyElementMixin(PolymerElement) { _initProperties( htmlEl: HTMLElement, plugin: PluginApi, - content?: HTMLElement + content?: Element | null ) { const el = htmlEl as HTMLElement & { plugin?: PluginApi; - content?: HTMLElement; + content?: Element; }; el.plugin = plugin; + // The content is (only?) used in ChangeReplyPluginApi. + // Maybe it would be better for the consumer side to figure out the content + // with something like el.getRootNode().host, etc. if (content) { el.content = content; } diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts index b1c695eaa2..7e8e7c0d7f 100644 --- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts +++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts @@ -478,7 +478,6 @@ export const hovercardBehaviorMixin = dedupingMixin( ); export interface GrHovercardBehaviorInterface { - attached(): void; ready(): void; removeListeners(): void; debounceHide(): void;