search

web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
5k stars 3.1k forks source link

Write Tests for Image-Orientation Initial Value #18549

Open nomadtechie opened 5 years ago

nomadtechie commented 5 years ago

The CSS Spec has been recently updated to interpret EXIF values from images by default. See discussion and patch here https://github.com/w3c/csswg-drafts/issues/3799

nomadtechie commented 5 years ago

Also, this test will need to change https://github.com/web-platform-tests/wpt/blob/master/css/css-images/inheritance.html

zcorpan commented 5 years ago

In https://github.com/web-platform-tests/wpt/tree/master/css/css-images there are some tests already for image-orientation (in particular inheritance.html and in parsing/ )

Per https://github.com/w3c/csswg-drafts/issues/4165#issuecomment-522114958 it's possible that the image-orientation CSS property may be dropped altogether, so we probably shouldn't spend too much time right now on fully testing the CSS property itself. But testing the change of the initial value is easy (1 line change in inheritance.html).

The different cases on the web that can show an image, that seem interesting to test (without any 'image-orientation' specified, with an image with some EXIF rotation):

There might be some other place on the web platform that displays images, but this covers almost everything I think, and also we don't need to test everything in the first patch. 😊

noell commented 5 years ago

Images for testing purposes https://github.com/noell/jpg-exif-test-images

zcorpan commented 4 years ago

It appears this patch added wpt tests for image-orientation

https://chromium.googlesource.com/chromium/src.git/+/0f40bb895ba85e8ba7bbc2d431512d33c7a55020

@schenney-chromium how many of the cases in my comment above is covered by those tests?

schenney-chromium commented 4 years ago

Copied the list above with comments, and added some more. Of the TODO, I will tackle some in the short term but probably not all. There's a lot of testing required here.

Done: CSS

HTML

TODO: CSS

SVG

HTML

zcorpan commented 4 years ago

Thanks @schenney-chromium!

@smfr would it be possible to upstream WebKit's tests to wpt?

heycam commented 4 years ago

Hi @schenney-chromium. I've been working on some Firefox patches to honor EXIF orientation in various places other than HTML img elements, and have just rebased on top of the tests you've added to WPT last month.

I'm really wondering whether the canvas drawImage behavior that is being tested is what we want to do. (And I do note that it's not something that has made its way into the HTML spec yet, and so should probably be tentative tests) It seems pretty odd to me to have a CSS property appyling to the canvas element affect the behavior of its rendering context methods. (I know that relative font-related settings are resolved against the font property values of the canvas element, but I think that's all?) If there is a need to override the orientation of the image, why shouldn't this be a parameter to drawImage or some other setting on the context?

schenney-chromium commented 4 years ago

On Sun, Mar 15, 2020 at 6:45 PM Cameron McCormack notifications@github.com wrote:

Hi @schenney-chromium https://github.com/schenney-chromium. I've been working on some Firefox patches to honor EXIF orientation in various places other than HTML img elements, and have just rebased on top of the tests you've added to WPT last month.

I'm really wondering whether the canvas drawImage behavior that is being tested is what we want to do. (And I do note that it's not something that has made its way into the HTML spec yet, and so should probably be tentative tests) It seems pretty odd to me to have a CSS property appyling to the canvas element affect the behavior of its rendering context methods. (I know that relative font-related settings are resolved against the font property values of the canvas element, but I think that's all?) If there is a need to override the orientation of the image, why shouldn't this be a parameter to drawImage or some other setting on the context?

I would certainly support a spec change in canvas to add respect-orientation to the canvas drawImage call. I agree it's way more straightforward than pulling from the canvas element, and it would also remove the current problem where a canvas not in the DOM does now know the orientation style setting. Plus it's more obvious for offscreen canvas. From the bugs we had filed related to orientation and canvas I think developers would approve.

I implemented the "take orientation from the canvas element" approach in Chrome because it was the most expedient way for canvas to workaround "respect EXIF everywhere". There's no reason why we can't change it if we can get spec agreement.

Feel free to move the tests to tentative; no complaints there. Or let me know if you want me to do it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/web-platform-tests/wpt/issues/18549#issuecomment-599275177, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4M64KS5WPZPU3M6WWPXN3RHVK65ANCNFSM4INCW52A .

heycam commented 4 years ago

@schenney-chromium Thanks, I can rename them while we discuss the right behavior. Did you get user requests to be able to provide a workaround for drawImage? (Generally would prefer not to add a flag unless we have an idea that it's needed.)

I have tests locally for the CSS properties mentioned in https://github.com/web-platform-tests/wpt/issues/18549#issuecomment-572816283 apart from shape-outside (which I think is not testable, as JPEGs have no transparency, and due to the way shape-outside images are stretched to fit the box, there's no way to detect whether a re-orientation was performed) and cursor. I'll land these as part of Gecko bugs.

faceless2 commented 4 years ago

So we've got a few tests now from @heycam and @schenney-chromium - thank you! But sadly they're contradictory.

  1. image-orientation-none-content-images.html - images 9..12 seem to be asserting that the EXIF orientation is not honored on images used in background-image.
  2. image-orientation-background-image.html is asserting that the EXIF orientation is honored for background-image, and that the image-orientation property is ignored.

we also have

  1. image-orientation-none-content-images.html again - the first 8 images assert that the image-orientation property does apply to generated content.
  2. image-orientation-list-style-image.html which asserts that the image-orientation property does not apply to list-style-image.

I think the resolution in https://github.com/w3c/csswg-drafts/issues/4165 is that image-orientation does not apply to background-image, and that the EXIF orientation is aways honored. Which means assertion 1 is wrong and 2 is correct.

And, as https://drafts.csswg.org/css-images-3/#the-image-orientation states that image-orientation applies to generated content, the question is whether list-style-image is generated content? I think it has to be, otherwise there is a disconnect between setting a list marker with this property, and setting one with ::marker { content: url(...) }. Which means assertion 3 is correct and assertion 4 is wrong.

Which is pleasingly balanced, if nothing else. As far as I can see these are test issues not spec clarification issues, but either of you disagree I'm happy to move this back to https://github.com/w3c/csswg-drafts/issues/4165

schenney-chromium commented 4 years ago

On Wed, Jun 17, 2020 at 2:03 PM Mike Bremford notifications@github.com wrote:

So we've got a few tests now from @heycam https://github.com/heycam and @schenney-chromium https://github.com/schenney-chromium - thank you! But sadly they're contradictory.

  1. image-orientation-none-content-images.html https://github.com/web-platform-tests/wpt/blob/master/css/css-images/image-orientation/image-orientation-none-content-images.html
    • images 9..12 seem to be asserting that the EXIF orientation is not honored on images used in background-image.

These tests have image-orientation: none applied, so the exif is not respected.

  1. image-orientation-background-image.html https://github.com/web-platform-tests/wpt/blob/master/css/css-images/image-orientation/image-orientation-background-image.html is asserting that the EXIF orientation is honored for background-image, and that the image-orientation property is ignored.

This I believe has the wrong reference, as the style specifies image-orientation: none but the reference is an image that has already been rotated as if exif is applied. It should be using ../support/exif-orientation-1.jpg

we also have

  1. image-orientation-none-content-images.html https://github.com/web-platform-tests/wpt/blob/master/css/css-images/image-orientation/image-orientation-none-content-images.html again - the first 8 images assert that the image-orientation property does apply to generated content.

This test has image-orientation: none so the exif is not respected.

  1. image-orientation-list-style-image.html https://github.com/web-platform-tests/wpt/blob/master/css/css-images/image-orientation/image-orientation-list-style-image.html which asserts that the image-orientation property does not apply to list-style-image.

This has the same issue in using a pre-rotated image when it should be using something without image orientation.

I think the resolution in w3c/csswg-drafts#4165 https://github.com/w3c/csswg-drafts/issues/4165 is that image-orientation does not apply to background-image, and that the EXIF orientation is aways honored. Which means assertion 1 is wrong and 2 is correct.

All browsers are converging to have image-orientation apply to content and style images, to make it consistent in how images appear when used in different situations. But note that image-orientation: none is supported, or will be, and this causes the exif orientation to be ignored.

And, as https://drafts.csswg.org/css-images-3/#the-image-orientation states that image-orientation applies to generated content, the question is whether list-style-image is generated content? I think it has to be, otherwise there is a disconnect between setting a list marker with this property, and setting one with ::marker { content: url(...) }. Which means assertion 3 is correct and assertion 4 is wrong.

The list style image is a style image, so should be oriented (unless image-orientation: none is present).

Which is pleasingly balanced, if nothing else. As far as I can see these are test issues not spec clarification issues, but either of you disagree I'm happy to move this back to w3c/csswg-drafts#4165 https://github.com/w3c/csswg-drafts/issues/4165

At this point if you feel the spec doesn't match the implementations we should take another look at the spec. My understanding is that there was agreement on the behavior but it's totally plausible that the spec does not reflect that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/web-platform-tests/wpt/issues/18549#issuecomment-645532399, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4M64PQCFB7VQI6NFZS72TRXEAQVANCNFSM4INCW52A .

faceless2 commented 4 years ago

| (re images 9..12) These tests have image-orientation: none applied, so the exif is not respected.

I'm afraid I disagree on this. There are two points here:

It applies only to content images (e.g. replaced elements and generated content), not decorative images (such as background-image).

So for image-orientation-none-content-images.html, the first 8 images are correct - they honor image-orientation: none, as they are displayed with the content property. The next 4 images do not, because they're displayed with the background-image property.

If you think the language of the spec needs revising based on some discussion post https://github.com/w3c/csswg-drafts/issues/4165, it's certainly possible - but I can't find any reference for that. Although it is certainly possible I have missed something.

heycam commented 4 years ago

To follow up, we discussed this in the CSSWG in w3c/csswg-drafts#4165 and resolved that image-orientation does apply to those decorative images. I've written a PR for those changes in w3c/csswg-drafts#5294 with test updates in https://phabricator.services.mozilla.com/D82471 which hasn't landed yet.

The tests I changed / added aren't completely comprehensive (I didn't write one for feImage for example) but there's probably not much more to do here.

heycam commented 4 years ago

To be clear, after my test changes land, the items from @zcorpan's list that still remain untested includes:

  • HTML
    • input type=image
  • load an image in a top-level doc (this already respects EXIF in all browsers I believe)
  • Favicon link rel=icon / favicon.ico (manual tests?)
  • web app manifest icon (manual test?)

And feImage is worth testing too.