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.

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

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

Merged
merged 5 commits into from Apr 21, 2021
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Apr 19, 2021

Fixes #12218


We consider the image "sized" (thus passing) if aspect-ratio is used in conjunction with either a width or a height:

(explicitWidth && explicitAspectRatio) ||
(explicitHeight && explicitAspectRatio);

@adamraine adamraine requested a review from a team as a code owner April 19, 2021 20:14
@adamraine adamraine requested review from patrickhulce and removed request for a team April 19, 2021 20:14
@google-cla google-cla bot added the cla: yes label Apr 19, 2021
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

impl LGTM

a smoketest would be great here too since all of this working correctly hinges on us getting the protocol styles response right ;) byte efficiency tester should have plenty of examples to modify or copy

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?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting "missing explicit width and height" on pages despite defining aspect-ratio in CSS
4 participants