Created attachment 411533 [details] Repro View attached example. The two images should have different orientations. One image is rotated via EXIF, and the other has rotated pixel data.
I'm testing on ToT WebKit on 19G2021
When opening LayoutTests/fast/images/image-orientation-none.html in Safari or mini browser, the display looks broken. However run-webkit-tests passes this test and matches the display of LayoutTests/fast/images/image-orientation-none-expected.html. The bug happens because CachedImage::isOriginClean() returns false of local images files. So HTMLImageElement::allowsOrientationOverride() return false as well and finally RenderElement::imageOrientation() returns ImageOrientation::FromImage and ignores the css styling image-orientation. When accessing the same page through an HTTP server, the display looks fine.
A possible solution would be to make this an http test
(In reply to Noam Rosenthal from comment #3) > A possible solution would be to make this an http test No. I think we need to fix it for local files. Having the layout for local images different from the layout for remote files is very confusing.
Sure, I'll work on a patch that
(In reply to Noam Rosenthal from comment #5) > Sure, I'll work on a patch that ... fixes this.
<rdar://problem/70603407>
Created attachment 412172 [details] Patch
Comment on attachment 412172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412172&action=review > Source/WebCore/html/HTMLImageElement.cpp:689 > + || (document().securityOrigin().canLoadLocalResources() && image->origin() && image->origin()->isLocal()); Do we need to check whether we canLoadLocalResources() or not here? I think this function should be used before loading the resource not after loading it. > LayoutTests/fast/images/image-orientation-none-local-expected.html:10 > + l.testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${l}`)) Why does the path have file:///tmp/? Are you sure this test load any resources at all? Does this test fail without your patch? > LayoutTests/fast/images/image-orientation-none-local.html:6 > + localImageLocation = testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${localImageLocation}`); Ditto.
Comment on attachment 412172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412172&action=review >> Source/WebCore/html/HTMLImageElement.cpp:689 >> + || (document().securityOrigin().canLoadLocalResources() && image->origin() && image->origin()->isLocal()); > > Do we need to check whether we canLoadLocalResources() or not here? I think this function should be used before loading the resource not after loading it. Maybe the check should be whether the >> LayoutTests/fast/images/image-orientation-none-local-expected.html:10 >> + l.testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${l}`)) > > Why does the path have file:///tmp/? Are you sure this test load any resources at all? Does this test fail without your patch? Yes, see other tests using pathToLocalResource. This seems to be the way to do it.
Created attachment 412200 [details] Patch
Comment on attachment 412172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412172&action=review >>> LayoutTests/fast/images/image-orientation-none-local-expected.html:10 >>> + l.testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${l}`)) >> >> Why does the path have file:///tmp/? Are you sure this test load any resources at all? Does this test fail without your patch? > > Yes, see other tests using pathToLocalResource. This seems to be the way to do it. I applied your patch and I changed the text in the <p> element in image-orientation-none-local.html. I ran "run-webkit-test image-orientation-none-local.html" and of course the test failed. But when seeing the difference, there was no images displayed either in the test or in the expected page. Only the <p> is displayed. I reverted your change HTMLImageElement::allowsOrientationOverride() and I ran "run-webkit-test image-orientation-none-local.html" without changing the <p> text. The test passed. This confirms no images were loaded in either the test of the expected page.
(In reply to Said Abou-Hallawa from comment #12) > Comment on attachment 412172 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=412172&action=review > > >>> LayoutTests/fast/images/image-orientation-none-local-expected.html:10 > >>> + l.testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${l}`)) > >> > >> Why does the path have file:///tmp/? Are you sure this test load any resources at all? Does this test fail without your patch? > > > > Yes, see other tests using pathToLocalResource. This seems to be the way to do it. > > I applied your patch and I changed the text in the <p> element in > image-orientation-none-local.html. I ran "run-webkit-test > image-orientation-none-local.html" and of course the test failed. But when > seeing the difference, there was no images displayed either in the test or > in the expected page. Only the <p> is displayed. > > I reverted your change HTMLImageElement::allowsOrientationOverride() and I > ran "run-webkit-test image-orientation-none-local.html" without changing the > <p> text. The test passed. This confirms no images were loaded in either the > test of the expected page. Hmm I wonder what's the way to do this... I tested with mini-browser and loading those images with file URLs. Will investigate.
Comment on attachment 412172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412172&action=review >>>> LayoutTests/fast/images/image-orientation-none-local-expected.html:10 >>>> + l.testRunner.pathToLocalResource(`file:///tmp/LayoutTests/fast/images/${l}`)) >>> >>> Why does the path have file:///tmp/? Are you sure this test load any resources at all? Does this test fail without your patch? >> >> Yes, see other tests using pathToLocalResource. This seems to be the way to do it. > > I applied your patch and I changed the text in the <p> element in image-orientation-none-local.html. I ran "run-webkit-test image-orientation-none-local.html" and of course the test failed. But when seeing the difference, there was no images displayed either in the test or in the expected page. Only the <p> is displayed. > > I reverted your change HTMLImageElement::allowsOrientationOverride() and I ran "run-webkit-test image-orientation-none-local.html" without changing the <p> text. The test passed. This confirms no images were loaded in either the test of the expected page. I looked at all the instances of "pathToLocalResource('file:///tmp/" and I found all of them under LayoutTests/http/tests/. Maybe you need to change the location of this test to be under LayoutTests/http/tests/images.
Created attachment 412297 [details] Patch
Committed r269155: <https://trac.webkit.org/changeset/269155> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412297 [details].