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(aspect-ratio): skip aspect ratio audit for svg #3722

Merged
merged 7 commits into from
Nov 2, 2017

Conversation

peterjanes
Copy link

I'm pretty confident that the code matches the suggestion in #3716. I made an attempt at a new test, but it isn't correct (passes with and without the new check); suggestions welcome.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@peterjanes
Copy link
Author

I've added my GitHub email to my Google account.

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.

thanks for picking this up so quickly @peterjanes! 🎉 👏

@@ -32,7 +32,7 @@ describe('Images: aspect-ratio audit', () => {
generateImage(
{width: data.clientSize[0], height: data.clientSize[1]},
{naturalWidth: data.naturalSize[0], naturalHeight: data.naturalSize[1]},
generateRecord(),
generateRecord({mimeType: data.mimeType}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's expecting two parameters so I believe this should look like generateRecord(undefined, data.mimeType)

then again, it doesn't really look like generateRecord is very useful for this test at all and was copied from another file so you can probably just ditch the function entirely and pass in the object directly to generateImage

@@ -73,6 +73,7 @@ class ImageAspectRatio extends Audit {
// filter out images that don't have following properties
// networkRecord, width, height, images that use `object-fit`: `cover` or `contain`
return image.networkRecord &&
image.networkRecord.mimeType !== 'image/svg+xml' &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

want to update the comment real quick to explain svg's don't have valid natural dimensions :)

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.

woohoo great work @peterjanes!

🎩 s off for both finding and fixing! 👏 🍰

@patrickhulce patrickhulce merged commit cfc2afe into GoogleChrome:master Nov 2, 2017
@paulirish
Copy link
Member

huge! thanks so much @peterjanes 😁

christhompson pushed a commit to christhompson/lighthouse that referenced this pull request Nov 28, 2017
* core(aspect-ratio): skip aspect ratio audit for svg, fixes GoogleChrome#3716

* core(aspect-ratio): use the correct svg mimeType in the test

* core(aspect-ratio): skip aspect ratio audit for svg

* core(aspect-ratio): use the correct svg mimeType in the test

* core(aspect-ratio): explain why svg is filtered

* core(aspect-ratio): better unit test (fails when it should)
@piotr-cz
Copy link

piotr-cz commented Jan 3, 2018

Does it fix inline SVG images?
I'm on Chrome Version 63.0.3239.108 (Official Build) (64-bit)

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jan 3, 2018

@piotr-cz this PR won't show up in Chrome until M65 unfortunately

But inline SVG images shouldn't be showing up currently at all since they don't have a network record. Do you have a reproducible case of this happening?

@piotr-cz
Copy link

piotr-cz commented Jan 3, 2018

No, I don't but it should be easy to reproduce with just any inline SVG image

@patrickhulce
Copy link
Collaborator

@piotr-cz it shouldn't be possible, and I'm not able to reproduce with any inline SVG image. That's why I'm asking if you remember where you saw it :)

@piotr-cz
Copy link

piotr-cz commented Jan 8, 2018

@patrickhulce The app I'm working on is quite complex and it would pretty hard to create test case out of it. It's not public, but I can send you a link by email to investigate if that's ok.

@patrickhulce
Copy link
Collaborator

sure that'd be great, email should be available on my GitHub profile

@piotr-cz
Copy link

piotr-cz commented Jan 9, 2018

@patrickhulce I've just send the link to email at Your GitHub profile.

Would be helpful, if Lighthouse would add more info, for example which image doesn't pass the test.

@patrickhulce
Copy link
Collaborator

@piotr-cz thanks for sending, I see the confusion now. You're referring to a different bug, #3551, which was fixed in 2.6 and should also be in Chrome 65 (use Chrome Canary, CLI, or extension for immediate workaround).

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

Successfully merging this pull request may close these issues.

5 participants