Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(unsized-images): pass with explicit aspect-ratio #12377

Merged
merged 5 commits into from
Apr 21, 2021
Merged

core(unsized-images): pass with explicit aspect-ratio #12377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa5782f
start
adamraine Apr 19, 2021
a605745
cool
adamraine Apr 19, 2021
3d4a22c
rn
adamraine Apr 20, 2021
0f2d87f
smoke
adamraine Apr 20, 2021
e73c61c
license
adamraine Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lighthouse-core/audits/unsized-images.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,17 @@ class UnsizedImages extends Audit {
const attrHeight = image.attributeHeight;
const cssWidth = image._privateCssSizing.width;
const cssHeight = image._privateCssSizing.height;
const cssAspectRatio = image._privateCssSizing.aspectRatio;
const htmlWidthIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrWidth);
const cssWidthIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssWidth);
const htmlHeightIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrHeight);
const cssHeightIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssHeight);
const explicitAspectRatio = UnsizedImages.doesCssPropProvideExplicitSize(cssAspectRatio);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this method? I was expecting a check for numbers or size markers, forgetting that all we care about is !== 'auto' :)

now that we're using it for things that aren't just size, perhaps something more generic like isCssPropExplicitlySet?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually come to think of it we should probably be banning initial/unset/inherit too, do those come through on the protocol?

const explicitWidth = htmlWidthIsExplicit || cssWidthIsExplicit;
const explicitHeight = htmlHeightIsExplicit || cssHeightIsExplicit;
return explicitWidth && explicitHeight;
return explicitWidth && explicitHeight ||
explicitWidth && explicitAspectRatio ||
adamraine marked this conversation as resolved.
Show resolved Hide resolved
explicitHeight && explicitAspectRatio;
}

/**
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/gather/gatherers/image-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,11 @@ class ImageElements extends Gatherer {
});
const width = getEffectiveSizingRule(matchedRules, 'width');
const height = getEffectiveSizingRule(matchedRules, 'height');
const aspectRatio = getEffectiveSizingRule(matchedRules, 'aspect-ratio');
// COMPAT: Maintain backcompat for <= 7.0.1
element.cssWidth = width === null ? undefined : width;
element.cssHeight = height === null ? undefined : height;
element._privateCssSizing = {width, height};
element._privateCssSizing = {width, height, aspectRatio};
} catch (err) {
if (/No node.*found/.test(err.message)) return;
throw err;
Expand Down
54 changes: 54 additions & 0 deletions lighthouse-core/test/audits/unsized-images-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,60 @@ describe('Sized images audit', () => {
});
});

describe('has defined aspect-ratio', () => {
it('fails when an image only has explicit CSS aspect-ratio', async () => {
const result = await runAudit({
attributeWidth: '',
attributeHeight: '',
_privateCssSizing: {
width: null,
height: null,
aspectRatio: '1 / 1',
},
});
expect(result.score).toEqual(0);
});

it('fails when an image only has non-explicit CSS aspect-ratio', async () => {
const result = await runAudit({
attributeWidth: '100',
attributeHeight: '',
_privateCssSizing: {
width: null,
height: null,
aspectRatio: 'auto',
},
});
expect(result.score).toEqual(0);
});

it('passes when CSS aspect-ratio and attribute width are explicit', async () => {
const result = await runAudit({
attributeWidth: '100',
attributeHeight: '',
_privateCssSizing: {
width: null,
height: null,
aspectRatio: '1 / 1',
},
});
expect(result.score).toEqual(1);
});

it('passes when CSS aspect-ratio and width are explicit', async () => {
const result = await runAudit({
attributeWidth: '',
attributeHeight: '',
_privateCssSizing: {
width: '100',
height: null,
aspectRatio: '1 / 1',
},
});
expect(result.score).toEqual(1);
});
});

it('is not applicable when there are no images', async () => {
const result = await UnsizedImagesAudit.audit({
ImageElements: [],
Expand Down
2 changes: 2 additions & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,8 @@ declare global {
width: string | null;
/** The height of the image as expressed by CSS rules. Set to `null` if there was no height set in CSS. */
height: string | null;
/** The aspect ratio of the image as expressed by CSS rules. Set to `null` if there was no aspect ratio set in CSS. */
aspectRatio: string | null;
}
/** The BoundingClientRect of the element. */
clientRect: {
Expand Down