Skip to content

Commit 1ca2ce1

Browse files
karaPawel Kozlowski
kara
authored and
Pawel Kozlowski
committed
fix(common): remove default for image width (#47082)
Previously NgOptimizedImage would default to requesting an image at the width matching the width attribute in the image HTML. While this works for width attrs that match the intrinsic size of the image (e.g. responsive images or images sized with CSS), this can be a sharp edge for developers who use the width/height attributes to set rendered size (i.e. instead of CSS, which one can do for a fixed size image). In this case, defaulting to the width attribute for the requested image would mean requesting always at 1x quality, so screens with a DPR of 2+ get an image that is too small. Without a default request width, the image served by the CDN would likely be at intrinsic size, so 2x images would look correct. This PR also updates the NgOptimizedImage sandbox and tests. PR Close #47082
1 parent 0c8eb8b commit 1ca2ce1

File tree

9 files changed

+96
-41
lines changed

9 files changed

+96
-41
lines changed

‎packages/common/src/directives/ng_optimized_image.ts‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@ import {RuntimeErrorCode} from '../errors';
1515
* Config options recognized by the image loader function.
1616
*/
1717
export interface ImageLoaderConfig {
18+
// Name of the image to be added to the image request URL
1819
src: string;
19-
quality?: number;
20+
// Width of the requested image (to be used when generating srcset)
2021
width?: number;
2122
}
2223

@@ -246,13 +247,10 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
246247
}
247248

248249
private getRewrittenSrc(): string {
249-
const imgConfig = {
250-
src: this.rawSrc,
251-
// TODO: if we're going to support responsive serving, we don't want to request the width
252-
// based solely on the intrinsic width (e.g. if it's on mobile and the viewport is smaller).
253-
// The width would require pre-processing before passing to the image loader function.
254-
width: this.width,
255-
};
250+
// ImageLoaderConfig supports setting a width property. However, we're not setting width here
251+
// because if the developer uses rendered width instead of intrinsic width in the HTML width
252+
// attribute, the image requested may be too small for 2x+ screens.
253+
const imgConfig = {src: this.rawSrc};
256254
return this.imageLoader(imgConfig);
257255
}
258256

‎packages/common/src/private_export.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
export {IMAGE_LOADER as ɵIMAGE_LOADER, NgOptimizedImage as ɵNgOptimizedImage} from './directives/ng_optimized_image';
9+
export {IMAGE_LOADER as ɵIMAGE_LOADER, ImageLoaderConfig as ɵImageLoaderConfig, NgOptimizedImage as ɵNgOptimizedImage} from './directives/ng_optimized_image';
1010
export {DomAdapter as ɵDomAdapter, getDOM as ɵgetDOM, setRootDomAdapter as ɵsetRootDomAdapter} from './dom_adapter';
1111
export {BrowserPlatformLocation as ɵBrowserPlatformLocation} from './location/platform_location';

‎packages/common/test/directives/ng_optimized_image_spec.ts‎

Lines changed: 48 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,18 +13,6 @@ import {ComponentFixture, TestBed} from '@angular/core/testing';
1313
import {expect} from '@angular/platform-browser/testing/src/matchers';
1414

1515
describe('Image directive', () => {
16-
it('should set `src` to `rawSrc` value if image loader is not provided', () => {
17-
setupTestingModule();
18-
19-
const template = '<img rawSrc="path/img.png" width="100" height="50">';
20-
const fixture = createTestComponent(template);
21-
fixture.detectChanges();
22-
23-
const nativeElement = fixture.nativeElement as HTMLElement;
24-
const img = nativeElement.querySelector('img')!;
25-
expect(img.src.endsWith('/path/img.png')).toBeTrue();
26-
});
27-
2816
it('should set `loading` and `fetchpriority` attributes before `src`', () => {
2917
// Only run this test in a browser since the Node-based DOM mocks don't
3018
// allow to override `HTMLImageElement.prototype.setAttribute` easily.
@@ -75,20 +63,6 @@ describe('Image directive', () => {
7563
expect(_fetchpriorityAttrId).toBeLessThan(_srcAttrId); // was set after `src`
7664
});
7765

78-
79-
it('should use an image loader provided via `IMAGE_LOADER` token', () => {
80-
const imageLoader = (config: ImageLoaderConfig) => `${config.src}?w=${config.width}`;
81-
setupTestingModule({imageLoader});
82-
83-
const template = '<img rawSrc="path/img.png" width="150" height="50">';
84-
const fixture = createTestComponent(template);
85-
fixture.detectChanges();
86-
87-
const nativeElement = fixture.nativeElement as HTMLElement;
88-
const img = nativeElement.querySelector('img')!;
89-
expect(img.src.endsWith('/path/img.png?w=150')).toBeTrue();
90-
});
91-
9266
describe('setup error handling', () => {
9367
it('should throw if both `src` and `rawSrc` are present', () => {
9468
setupTestingModule();
@@ -315,6 +289,54 @@ describe('Image directive', () => {
315289
expect(img.getAttribute('fetchpriority')).toBe('auto');
316290
});
317291
});
292+
293+
describe('loaders', () => {
294+
it('should set `src` to match `rawSrc` if image loader is not provided', () => {
295+
setupTestingModule();
296+
297+
const template = '<img rawSrc="path/img.png" width="100" height="50">';
298+
const fixture = createTestComponent(template);
299+
fixture.detectChanges();
300+
301+
const nativeElement = fixture.nativeElement as HTMLElement;
302+
const img = nativeElement.querySelector('img')!;
303+
expect(img.src.trim()).toBe('/path/img.png');
304+
});
305+
306+
it('should set `src` using the image loader provided via the `IMAGE_LOADER` token to compose src URL',
307+
() => {
308+
const imageLoader = (config: ImageLoaderConfig) => `path/${config.src}`;
309+
setupTestingModule({imageLoader});
310+
311+
const template = `
312+
<img rawSrc="img.png" width="150" height="50">
313+
<img rawSrc="img-2.png" width="150" height="50">
314+
`;
315+
const fixture = createTestComponent(template);
316+
fixture.detectChanges();
317+
318+
const nativeElement = fixture.nativeElement as HTMLElement;
319+
const imgs = nativeElement.querySelectorAll('img')!;
320+
expect(imgs[0].src.trim()).toBe('/path/img.png');
321+
expect(imgs[1].src.trim()).toBe('/path/img-2.png');
322+
});
323+
324+
it('should set`src` to an image URL that does not include a default width parameter', () => {
325+
const imageLoader = (config: ImageLoaderConfig) => {
326+
const widthStr = config.width ? `?w=${config.width}` : ``;
327+
return `path/${config.src}${widthStr}`;
328+
};
329+
setupTestingModule({imageLoader});
330+
331+
const template = '<img rawSrc="img.png" width="150" height="50">';
332+
const fixture = createTestComponent(template);
333+
fixture.detectChanges();
334+
335+
const nativeElement = fixture.nativeElement as HTMLElement;
336+
const img = nativeElement.querySelector('img')!;
337+
expect(img.src.trim()).toBe('/path/img.png');
338+
});
339+
});
318340
});
319341

320342
// Helpers

‎packages/core/test/bundling/image-directive/BUILD.bazel‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,6 @@ ts_devserver(
5454
static_files = [
5555
"index.html",
5656
":tslib",
57-
"a.png",
58-
"b.png",
5957
],
6058
deps = [":image-directive"],
6159
)

‎packages/core/test/bundling/image-directive/a.png‎

-2.33 KB
Binary file not shown.

‎packages/core/test/bundling/image-directive/b.png‎

-2.33 KB
Binary file not shown.

‎packages/core/test/bundling/image-directive/basic/basic.e2e-spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,6 @@ describe('NgOptimizedImage directive', () => {
1717
await browser.get('/');
1818
const imgs = element.all(by.css('img'));
1919
const src = await imgs.get(0).getAttribute('src');
20-
expect(src.endsWith('b.png')).toBe(true);
20+
expect(/a\.png/.test(src)).toBe(true);
2121
});
2222
});

‎packages/core/test/bundling/image-directive/basic/basic.ts‎

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,51 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {ɵIMAGE_LOADER as IMAGE_LOADER, ɵNgOptimizedImage as NgOptimizedImage} from '@angular/common';
9+
import {ɵIMAGE_LOADER as IMAGE_LOADER, ɵImageLoaderConfig as ImageLoaderConfig, ɵNgOptimizedImage as NgOptimizedImage} from '@angular/common';
1010
import {Component} from '@angular/core';
1111

12+
const CUSTOM_IMGIX_LOADER = (config: ImageLoaderConfig) => {
13+
const widthStr = config.width ? `?w=${config.width}` : ``;
14+
return `https://aurora-project.imgix.net/${config.src}${widthStr}`;
15+
};
16+
17+
1218
@Component({
1319
selector: 'basic',
20+
styles: [`
21+
h1 {
22+
display: flex;
23+
align-items: center;
24+
}
25+
26+
main {
27+
border: 1px solid blue;
28+
margin: 16px;
29+
padding: 16px;
30+
}
31+
32+
.spacer {
33+
height: 3000px;
34+
}
35+
36+
main img {
37+
width: 100%;
38+
height: auto;
39+
}
40+
`],
41+
template: `
42+
<h1>
43+
<img rawSrc="a.png" width="50" height="50" priority>
44+
<span>Angular image app</span>
45+
</h1>
46+
<main>
47+
<div class="spacer"></div>
48+
<img rawSrc="hermes.jpeg" width="4030" height="3020">
49+
</main>
50+
`,
1451
standalone: true,
1552
imports: [NgOptimizedImage],
16-
template: `<img rawSrc="./a.png" width="150" height="150" priority>`,
17-
providers: [{provide: IMAGE_LOADER, useValue: () => 'b.png'}],
53+
providers: [{provide: IMAGE_LOADER, useValue: CUSTOM_IMGIX_LOADER}],
1854
})
1955
export class BasicComponent {
2056
}

‎packages/core/test/bundling/image-directive/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {LcpCheckComponent} from './lcp-check/lcp-check';
1818
standalone: true,
1919
imports: [RouterModule],
2020
template: '<router-outlet></router-outlet>',
21+
2122
})
2223
export class RootComponent {
2324
}

0 commit comments

Comments
 (0)