Bug 217808 – REGRESSION(r268249): image-orientation:none is broken for local (file://) urls

• Component: Images
• Status: RESOLVED FIXED

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

I'm testing on ToT WebKit on 19G2021

[Said Abou-Hallawa]

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.

[Noam Rosenthal]

A possible solution would be to make this an http test

[Said Abou-Hallawa]

(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.

[Noam Rosenthal]

Sure, I'll work on a patch that

[Noam Rosenthal]

(In reply to Noam Rosenthal from comment #5)
> Sure, I'll work on a patch that
... fixes this.

[Radar WebKit Bug Importer]

<rdar://problem/70603407>

[Noam Rosenthal]

Created attachment 412172 [details]
Patch

[Said Abou-Hallawa]

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.

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 412200 [details]
Patch

[Said Abou-Hallawa]

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.

[Noam Rosenthal]

(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.

[Said Abou-Hallawa]

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.

[Noam Rosenthal]

Created attachment 412297 [details]
Patch

[EWS]

Committed r269155: <https://trac.webkit.org/changeset/269155>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412297 [details].