Convert image diff building to Lit
This was tested comparing the diff of added and modified images at HEAD
with this change. Both for the new and old image diff view.
Release-Notes: skip
Google-Bug-Id: b/237393560
Change-Id: I14442b44bfc4922153a2fe53bc693a17b684c419
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 6cd3cb0..096c28e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -7,7 +7,12 @@
import '../../../elements/shared/gr-hovercard/gr-hovercard';
import './gr-diff-builder-side-by-side';
import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
-import {DiffBuilder, DiffContextExpandedEventDetail} from './gr-diff-builder';
+import {
+ DiffBuilder,
+ ImageDiffBuilder,
+ DiffContextExpandedEventDetail,
+ isImageDiffBuilder,
+} from './gr-diff-builder';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {GrDiffBuilderImage} from './gr-diff-builder-image';
import {GrDiffBuilderUnified} from './gr-diff-builder-unified';
@@ -114,7 +119,7 @@
layers: DiffLayer[] = [];
// visible for testing
- builder?: DiffBuilder;
+ builder?: DiffBuilder | ImageDiffBuilder;
/**
* All layers, both from the outside and the default ones. See `layers` for
@@ -206,8 +211,8 @@
return (
this.cancelableRenderPromise
.then(async () => {
- if (this.isImageDiff) {
- (this.builder as GrDiffBuilderImage).renderDiff();
+ if (isImageDiffBuilder(this.builder)) {
+ this.builder.renderImageDiff();
}
await this.untilGroupsRendered();
this.fireDiffEvent('render-content');
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
index 75ee088..096d32e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-image.ts
@@ -3,228 +3,265 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side';
import {ImageInfo} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {GrEndpointParam} from '../../../elements/plugins/gr-endpoint-param/gr-endpoint-param';
-import {RenderPreferences} from '../../../api/diff';
+import {RenderPreferences, Side} from '../../../api/diff';
import '../gr-diff-image-viewer/gr-image-viewer';
-import {GrImageViewer} from '../gr-diff-image-viewer/gr-image-viewer';
-import {createElementDiff} from '../gr-diff/gr-diff-utils';
+import {ImageDiffBuilder} from './gr-diff-builder';
+import {html, LitElement, nothing, render} from 'lit';
+import {customElement, property, query, state} from 'lit/decorators.js';
// MIME types for images we allow showing. Do not include SVG, it can contain
// arbitrary JavaScript.
const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|x-icon|jpeg|jpg|png|tiff|webp)$/;
-export class GrDiffBuilderImage extends GrDiffBuilderSideBySide {
+export class GrDiffBuilderImage
+ extends GrDiffBuilderSideBySide
+ implements ImageDiffBuilder
+{
constructor(
diff: DiffInfo,
prefs: DiffPreferencesInfo,
outputEl: HTMLElement,
- private readonly _baseImage: ImageInfo | null,
- private readonly _revisionImage: ImageInfo | null,
+ private readonly baseImage: ImageInfo | null,
+ private readonly revisionImage: ImageInfo | null,
renderPrefs?: RenderPreferences,
- private readonly _useNewImageDiffUi: boolean = false
+ private readonly useNewImageDiffUi: boolean = false
) {
super(diff, prefs, outputEl, [], renderPrefs);
}
- public renderDiff() {
- const section = createElementDiff('tbody', 'image-diff');
-
- if (this._useNewImageDiffUi) {
- this._emitImageViewer(section);
-
- this.outputEl.appendChild(section);
- } else {
- this._emitImagePair(section);
- this._emitImageLabels(section);
-
- this.outputEl.appendChild(section);
- this.outputEl.appendChild(this._createEndpoint());
- }
- }
-
- private _createEndpoint() {
- const tbody = createElementDiff('tbody');
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
-
- // TODO(kaspern): Support blame for image diffs and remove the hardcoded 4
- // column limit.
- td.setAttribute('colspan', '4');
- const endpointDomApi = createElementDiff('gr-endpoint-decorator');
- endpointDomApi.setAttribute('name', 'image-diff');
- endpointDomApi.appendChild(
- this._createEndpointParam('baseImage', this._baseImage)
+ public renderImageDiff() {
+ render(
+ html`
+ ${this.useNewImageDiffUi
+ ? html`
+ <gr-diff-image-new
+ .automaticBlink=${this.autoBlink()}
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-new>
+ `
+ : html`
+ <gr-diff-image-old
+ .baseImage=${this.baseImage ?? undefined}
+ .revisionImage=${this.revisionImage ?? undefined}
+ ></gr-diff-image-old>
+ `}
+ `,
+ this.outputEl
);
- endpointDomApi.appendChild(
- this._createEndpointParam('revisionImage', this._revisionImage)
- );
- td.appendChild(endpointDomApi);
- tr.appendChild(td);
- tbody.appendChild(tr);
- return tbody;
}
- private _createEndpointParam(name: string, value: ImageInfo | null) {
- const endpointParam = createElementDiff(
- 'gr-endpoint-param'
- ) as GrEndpointParam;
- endpointParam.name = name;
- endpointParam.value = value;
- return endpointParam;
- }
-
- private _emitImageViewer(section: HTMLElement) {
- const tr = createElementDiff('tr');
- const td = createElementDiff('td');
- // TODO(hermannloose): Support blame for image diffs, see above.
- td.setAttribute('colspan', '4');
- const imageViewer = createElementDiff('gr-image-viewer') as GrImageViewer;
-
- imageViewer.baseUrl = this._getImageSrc(this._baseImage);
- imageViewer.revisionUrl = this._getImageSrc(this._revisionImage);
- imageViewer.automaticBlink =
- !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
-
- td.appendChild(imageViewer);
- tr.appendChild(td);
- section.appendChild(tr);
- }
-
- private _getImageSrc(image: ImageInfo | null): string {
- return image && IMAGE_MIME_PATTERN.test(image.type)
- ? `data:${image.type};base64,${image.body}`
- : '';
- }
-
- private _emitImagePair(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- tr.appendChild(this._createImageCell(this._baseImage, 'left', section));
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- tr.appendChild(
- this._createImageCell(this._revisionImage, 'right', section)
- );
-
- section.appendChild(tr);
- }
-
- private _createImageCell(
- image: ImageInfo | null,
- className: string,
- section: HTMLElement
- ) {
- const td = createElementDiff('td', className);
- const src = this._getImageSrc(image);
- if (image && src) {
- const imageEl = createElementDiff('img') as HTMLImageElement;
- imageEl.onload = () => {
- image._height = imageEl.naturalHeight;
- image._width = imageEl.naturalWidth;
- this._updateImageLabel(section, className, image);
- };
- imageEl.addEventListener('error', (e: Event) => {
- imageEl.remove();
- td.textContent = '[Image failed to load] ' + e.type;
- });
- imageEl.setAttribute('src', src);
- td.appendChild(imageEl);
- }
- return td;
- }
-
- private _updateImageLabel(
- section: HTMLElement,
- className: string,
- image: ImageInfo
- ) {
- const label = section.querySelector(
- '.' + className + ' span.label'
- ) as HTMLElement;
- this._setLabelText(label, image);
- }
-
- private _setLabelText(label: HTMLElement, image: ImageInfo | null) {
- label.textContent = getImageLabel(image);
- }
-
- private _emitImageLabels(section: HTMLElement) {
- const tr = createElementDiff('tr');
-
- let addNamesInLabel = false;
-
- if (
- this._baseImage &&
- this._revisionImage &&
- this._baseImage._name !== this._revisionImage._name
- ) {
- addNamesInLabel = true;
- }
-
- tr.appendChild(createElementDiff('td', 'left lineNum blank'));
- let td = createElementDiff('td', 'left');
- let label = createElementDiff('label');
- let nameSpan;
- let labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._baseImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._baseImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- tr.appendChild(createElementDiff('td', 'right lineNum blank'));
- td = createElementDiff('td', 'right');
- label = createElementDiff('label');
- labelSpan = createElementDiff('span', 'label');
-
- if (addNamesInLabel) {
- nameSpan = createElementDiff('span', 'name');
- nameSpan.textContent = this._revisionImage?._name ?? '';
- label.appendChild(nameSpan);
- label.appendChild(createElementDiff('br'));
- }
-
- this._setLabelText(labelSpan, this._revisionImage);
-
- label.appendChild(labelSpan);
- td.appendChild(label);
- tr.appendChild(td);
-
- section.appendChild(tr);
+ private autoBlink(): boolean {
+ return !!this.renderPrefs?.image_diff_prefs?.automatic_blink;
}
override updateRenderPrefs(renderPrefs: RenderPreferences) {
- const imageViewer = this.outputEl.querySelector(
- 'gr-image-viewer'
- ) as GrImageViewer;
- if (this._useNewImageDiffUi && imageViewer) {
- imageViewer.automaticBlink =
- !!renderPrefs?.image_diff_prefs?.automatic_blink;
- }
+ this.renderPrefs = renderPrefs;
+
+ // We have to update `imageDiff.automaticBlink` manually, because `this` is
+ // not a LitElement.
+ const imageDiff = this.outputEl.querySelector(
+ 'gr-diff-image-new'
+ ) as GrDiffImageNew;
+ if (imageDiff) imageDiff.automaticBlink = this.autoBlink();
}
}
-function getImageLabel(image: ImageInfo | null) {
- if (image) {
- const type = image.type ?? image._expectedType;
- if (image._width && image._height) {
- return `${image._width}×${image._height} ${type}`;
- } else {
- return type;
- }
+@customElement('gr-diff-image-new')
+class GrDiffImageNew extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @property() automaticBlink = false;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
}
- return 'No image';
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-image-viewer
+ class="gr-diff"
+ .baseUrl=${imageSrc(this.baseImage)}
+ .revisionUrl=${imageSrc(this.revisionImage)}
+ .automaticBlink=${this.automaticBlink}
+ >
+ </gr-image-viewer>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+}
+
+@customElement('gr-diff-image-old')
+class GrDiffImageOld extends LitElement {
+ @property() baseImage?: ImageInfo;
+
+ @property() revisionImage?: ImageInfo;
+
+ @query('img.left') baseImageEl?: HTMLImageElement;
+
+ @query('img.right') revisionImageEl?: HTMLImageElement;
+
+ @state() baseError?: string;
+
+ @state() revisionError?: string;
+
+ /**
+ * The browser API for handling selection does not (yet) work for selection
+ * across multiple shadow DOM elements. So we are rendering gr-diff components
+ * into the light DOM instead of the shadow DOM by overriding this method,
+ * which was the recommended workaround by the lit team.
+ * See also https://github.com/WICG/webcomponents/issues/79.
+ */
+ override createRenderRoot() {
+ return this;
+ }
+
+ override render() {
+ return html`
+ <tbody class="gr-diff image-diff">
+ ${this.renderImagePairRow()} ${this.renderImageLabelRow()}
+ </tbody>
+ ${this.renderEndpoint()}
+ `;
+ }
+
+ private renderEndpoint() {
+ return html`
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <td class="gr-diff" colspan="4">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ ${this.renderEndpointParam('baseImage', this.baseImage)}
+ ${this.renderEndpointParam('revisionImage', this.revisionImage)}
+ </gr-endpoint-decorator>
+ </td>
+ </tr>
+ </tbody>
+ `;
+ }
+
+ private renderEndpointParam(name: string, value: unknown) {
+ if (!value) return nothing;
+ return html`
+ <gr-endpoint-param class="gr-diff" name=${name} .value=${value}>
+ </gr-endpoint-param>
+ `;
+ }
+
+ private renderImagePairRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">${this.renderImage(Side.LEFT)}</td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">${this.renderImage(Side.RIGHT)}</td>
+ </tr>
+ `;
+ }
+
+ private renderImage(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ if (!image) return nothing;
+ const error = side === Side.LEFT ? this.baseError : this.revisionError;
+ if (error) return error;
+ const src = imageSrc(image);
+ if (!src) return nothing;
+
+ return html`
+ <img
+ class="gr-diff ${side}"
+ src=${src}
+ @load=${this.handleLoad}
+ @error=${(e: Event) => this.handleError(e, side)}
+ >
+ </img>
+ `;
+ }
+
+ private handleLoad() {
+ this.requestUpdate();
+ }
+
+ private handleError(e: Event, side: Side) {
+ const msg = `[Image failed to load] ${e.type}`;
+ if (side === Side.LEFT) this.baseError = msg;
+ if (side === Side.RIGHT) this.revisionError = msg;
+ }
+
+ private renderImageLabelRow() {
+ return html`
+ <tr class="gr-diff">
+ <td class="gr-diff left lineNum blank"></td>
+ <td class="gr-diff left">
+ <label class="gr-diff">
+ ${this.renderName(this.baseImage?._name ?? '')}
+ <span class="gr-diff label">${this.imageLabel(Side.LEFT)}</span>
+ </label>
+ </td>
+ <td class="gr-diff right lineNum blank"></td>
+ <td class="gr-diff right">
+ <label class="gr-diff">
+ ${this.renderName(this.revisionImage?._name ?? '')}
+ <span class="gr-diff label"> ${this.imageLabel(Side.RIGHT)} </span>
+ </label>
+ </td>
+ </tr>
+ `;
+ }
+
+ private renderName(name?: string) {
+ const addNamesInLabel =
+ this.baseImage &&
+ this.revisionImage &&
+ this.baseImage._name !== this.revisionImage._name;
+ if (!addNamesInLabel) return nothing;
+ return html`
+ <span class="gr-diff name">${name}</span><br class="gr-diff" />
+ `;
+ }
+
+ private imageLabel(side: Side) {
+ const image = side === Side.LEFT ? this.baseImage : this.revisionImage;
+ const imageEl =
+ side === Side.LEFT ? this.baseImageEl : this.revisionImageEl;
+ if (image) {
+ const type = image.type ?? image._expectedType;
+ if (imageEl?.naturalWidth && imageEl.naturalHeight) {
+ return `${imageEl?.naturalWidth}×${imageEl.naturalHeight} ${type}`;
+ } else {
+ return type;
+ }
+ }
+ return 'No image';
+ }
+}
+
+function imageSrc(image?: ImageInfo): string {
+ return image && IMAGE_MIME_PATTERN.test(image.type)
+ ? `data:${image.type};base64,${image.body}`
+ : '';
+}
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'gr-diff-image-new': GrDiffImageNew;
+ 'gr-diff-image-old': GrDiffImageOld;
+ }
}
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
index f3c88a9..5ca5197 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder.ts
@@ -64,6 +64,16 @@
updateRenderPrefs(renderPrefs: RenderPreferences): void;
}
+export interface ImageDiffBuilder extends DiffBuilder {
+ renderImageDiff(): void;
+}
+
+export function isImageDiffBuilder(
+ x: DiffBuilder | ImageDiffBuilder | undefined
+): x is ImageDiffBuilder {
+ return !!x && !!(x as ImageDiffBuilder).renderImageDiff;
+}
+
/**
* Base class for different diff builders, like side-by-side, unified etc.
*
@@ -82,7 +92,7 @@
// visible for testing
readonly _prefs: DiffPreferencesInfo;
- protected readonly renderPrefs?: RenderPreferences;
+ protected renderPrefs?: RenderPreferences;
protected readonly outputEl: HTMLElement;
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index 6029e65..78f71d4 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -971,6 +971,8 @@
z-index: 10;
}
+ gr-diff-image-new,
+ gr-diff-image-old,
gr-diff-section,
gr-context-controls-section,
gr-diff-row {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index d49a2b9..41fdecf 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -2049,14 +2049,14 @@
<td class="blank gr-diff left lineNum"></td>
<td class="gr-diff left">
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
</td>
<td class="blank gr-diff lineNum right"></td>
<td class="gr-diff right">
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
</td>
@@ -2078,6 +2078,22 @@
</tbody>
`
);
+ const endpoint = queryAndAssert(element, 'tbody.endpoint');
+ assert.dom.equal(
+ endpoint,
+ /* HTML */ `
+ <tbody class="gr-diff endpoint">
+ <tr class="gr-diff">
+ <gr-endpoint-decorator class="gr-diff" name="image-diff">
+ <gr-endpoint-param class="gr-diff" name="baseImage">
+ </gr-endpoint-param>
+ <gr-endpoint-param class="gr-diff" name="revisionImage">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </tr>
+ </tbody>
+ `
+ );
});
test('renders image diffs with a different file name', async () => {
@@ -2156,7 +2172,7 @@
rightImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff right"
src="data:image/bmp;base64,${mockFile2.body}"
/>
`
@@ -2190,7 +2206,7 @@
leftImage,
/* HTML */ `
<img
- class="gr-diff"
+ class="gr-diff left"
src="data:image/bmp;base64,${mockFile1.body}"
/>
`
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 93a3f2a..7370c96 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -750,8 +750,6 @@
type: string;
_name?: string;
_expectedType?: string;
- _width?: number;
- _height?: number;
}
/**
