Fix adding event listeners in change-view only once
The change view had a bug where it would add event listeners to itself
every time `connectedCallback()` was called. So these listeners were
accumulating and handlers were called multiple times, if the
component was disconnected from the DOM and the re-connected.
Before migrating gr-app-element to Lit in change 336243 this bug was not
exposed, because Polymer was just hiding the component, not
disconnecting it. So there was just no scenario where a change view
would be disconnected and then reconnected again. In Lit the view
caching is handled differently, so we are now seeing issues due to the
bug, e.g. events for saving the commit message being handled twice.
The change view is disconnected and then reconnected every time that you
go from a change view to a diff view (or any other view actually) and
then back.
Release-Notes: skip
Google-Bug-Id: b/232499174
Change-Id: I5c57d29b36816f667160fe3118efeae5a8627c7b
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 7e4cf2c..a27c5e6 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -633,6 +633,12 @@
private connected$ = new BehaviorSubject(false);
+ /**
+ * For `connectedCallback()` to distinguish between connecting to the DOM for
+ * the first time or if just re-connecting.
+ */
+ private isFirstConnection = true;
+
/** Simply reflects the router-model value. */
// visible for testing
routerPatchNum?: PatchSetNum;
@@ -649,12 +655,29 @@
'fullscreen-overlay-opened',
() => this._handleHideBackgroundContent()
);
-
this.addEventListener('fullscreen-overlay-closed', () =>
this._handleShowBackgroundContent()
);
-
this.addEventListener('open-reply-dialog', () => this._openReplyDialog());
+ this.addEventListener('change-message-deleted', () => fireReload(this));
+ this.addEventListener('editable-content-save', e =>
+ this._handleCommitMessageSave(e)
+ );
+ this.addEventListener('editable-content-cancel', () =>
+ this._handleCommitMessageCancel()
+ );
+ this.addEventListener('open-fix-preview', e => this._onOpenFixPreview(e));
+ this.addEventListener('close-fix-preview', e => this._onCloseFixPreview(e));
+
+ this.addEventListener(EventType.SHOW_PRIMARY_TAB, e =>
+ this._setActivePrimaryTab(e)
+ );
+ this.addEventListener('reload', e => {
+ this.loadData(
+ /* isLocationChange= */ false,
+ /* clearPatchset= */ e.detail && e.detail.clearPatchset
+ );
+ });
}
private setupSubscriptions() {
@@ -699,24 +722,23 @@
override connectedCallback() {
super.connectedCallback();
+ this.firstConnectedCallback();
this.connected$.next(true);
- this.setupSubscriptions();
- this._throttledToggleChangeStar = throttleWrap<KeyboardEvent>(_ =>
- this._handleToggleChangeStar()
- );
- this._getServerConfig().then(config => {
- this._serverConfig = config;
- this._replyDisabled = false;
- });
- this._getLoggedIn().then(loggedIn => {
- this._loggedIn = loggedIn;
- if (loggedIn) {
- this.restApiService.getAccount().then(acct => {
- this._account = acct;
- });
- }
- });
+ // Make sure to reverse everything below this line in disconnectedCallback().
+ // Or consider using either firstConnectedCallback() or constructor().
+ this.setupSubscriptions();
+ document.addEventListener('visibilitychange', this.handleVisibilityChange);
+ document.addEventListener('scroll', this.handleScroll);
+ }
+
+ /**
+ * For initialization that should only happen once, not again when
+ * re-connecting to the DOM later.
+ */
+ private firstConnectedCallback() {
+ if (!this.isFirstConnection) return;
+ this.isFirstConnection = false;
getPluginLoader()
.awaitPluginsLoaded()
@@ -734,26 +756,21 @@
})
.then(() => this._initActiveTabs(this.params));
- this.addEventListener('change-message-deleted', () => fireReload(this));
- this.addEventListener('editable-content-save', e =>
- this._handleCommitMessageSave(e)
+ this._throttledToggleChangeStar = throttleWrap<KeyboardEvent>(_ =>
+ this._handleToggleChangeStar()
);
- this.addEventListener('editable-content-cancel', () =>
- this._handleCommitMessageCancel()
- );
- this.addEventListener('open-fix-preview', e => this._onOpenFixPreview(e));
- this.addEventListener('close-fix-preview', e => this._onCloseFixPreview(e));
- document.addEventListener('visibilitychange', this.handleVisibilityChange);
- document.addEventListener('scroll', this.handleScroll);
+ this._getServerConfig().then(config => {
+ this._serverConfig = config;
+ this._replyDisabled = false;
+ });
- this.addEventListener(EventType.SHOW_PRIMARY_TAB, e =>
- this._setActivePrimaryTab(e)
- );
- this.addEventListener('reload', e => {
- this.loadData(
- /* isLocationChange= */ false,
- /* clearPatchset= */ e.detail && e.detail.clearPatchset
- );
+ this._getLoggedIn().then(loggedIn => {
+ this._loggedIn = loggedIn;
+ if (loggedIn) {
+ this.restApiService.getAccount().then(acct => {
+ this._account = acct;
+ });
+ }
});
}
