-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Write Tests for Image-Orientation Initial Value #18549
Comments
Also, this test will need to change https://github.com/web-platform-tests/wpt/blob/master/css/css-images/inheritance.html |
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 w3c/csswg-drafts#4165 (comment) 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. 😊 |
Images for testing purposes https://github.com/noell/jpg-exif-test-images |
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? |
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:
HTML
TODO:
SVG
HTML
|
Thanks @schenney-chromium! @smfr would it be possible to upstream WebKit's tests to wpt? |
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? |
On Sun, Mar 15, 2020 at 6:45 PM Cameron McCormack ***@***.***> 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
<#18549 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4M64KS5WPZPU3M6WWPXN3RHVK65ANCNFSM4INCW52A>
.
|
@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 #18549 (comment) 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. |
Per #18549 (comment). Differential Revision: https://phabricator.services.mozilla.com/D67936 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1616169 gecko-commit: 23d1f2dbee27a56124f6d1a817933500afc7af40 gecko-integration-branch: autoland gecko-reviewers: tnikkel
…=tnikkel Per web-platform-tests/wpt#18549 (comment). Differential Revision: https://phabricator.services.mozilla.com/D67936
…=tnikkel Per web-platform-tests/wpt#18549 (comment). Differential Revision: https://phabricator.services.mozilla.com/D67936 --HG-- rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-orientation-none.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-orientation-none.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-swap-width-height-orientation-none.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-swap-width-height-orientation-none.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-swap-width-height.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap-swap-width-height.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-bitmap.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-orientation-none.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-orientation-none.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-swap-width-height-orientation-none.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-swap-width-height-orientation-none.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-swap-width-height.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-swap-width-height.tentative.html rename : testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element.html => testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/image-orientation/drawImage-from-element.tentative.html extra : moz-landing-system : lando
Per #18549 (comment). Differential Revision: https://phabricator.services.mozilla.com/D67936 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1616169 gecko-commit: 23d1f2dbee27a56124f6d1a817933500afc7af40 gecko-integration-branch: autoland gecko-reviewers: tnikkel
So we've got a few tests now from @heycam and @schenney-chromium - thank you! But sadly they're contradictory.
we also have
I think the resolution in w3c/csswg-drafts#4165 is that And, as https://drafts.csswg.org/css-images-3/#the-image-orientation states that 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 |
On Wed, Jun 17, 2020 at 2:03 PM Mike Bremford ***@***.***> 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
<w3c/csswg-drafts#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
<w3c/csswg-drafts#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
<#18549 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4M64PQCFB7VQI6NFZS72TRXEAQVANCNFSM4INCW52A>
.
|
| (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:
So for image-orientation-none-content-images.html, the first 8 images are correct - they honor If you think the language of the spec needs revising based on some discussion post w3c/csswg-drafts#4165, it's certainly possible - but I can't find any reference for that. Although it is certainly possible I have missed something. |
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 |
To be clear, after my test changes land, the items from @zcorpan's list that still remain untested includes:
And |
The CSS Spec has been recently updated to interpret EXIF values from images by default. See discussion and patch here w3c/csswg-drafts#3799
The text was updated successfully, but these errors were encountered: