RESOLVED FIXED 217294
CSS image-orientation: none should be ignored for cross-origin images
https://bugs.webkit.org/show_bug.cgi?id=217294
Summary CSS image-orientation: none should be ignored for cross-origin images
Noam Rosenthal
Reported 2020-10-04 07:13:39 PDT
See https://github.com/w3c/csswg-drafts/issues/5165 To avoid leaking metadata to the embedder, images that can't be accessed by the document's security origin should always render with their default (from-image) orientation.
Attachments
Patch (23.84 KB, patch)
2020-10-04 23:05 PDT, Noam Rosenthal
no flags
Patch (23.80 KB, patch)
2020-10-06 03:02 PDT, Noam Rosenthal
no flags
Patch (23.85 KB, patch)
2020-10-06 03:09 PDT, Noam Rosenthal
no flags
Patch (24.51 KB, patch)
2020-10-08 06:00 PDT, Noam Rosenthal
no flags
Patch (24.53 KB, patch)
2020-10-08 08:44 PDT, Noam Rosenthal
no flags
Patch (21.37 KB, patch)
2020-10-08 10:49 PDT, Noam Rosenthal
no flags
Patch for landing (20.91 KB, patch)
2020-10-09 00:36 PDT, Noam Rosenthal
no flags
Noam Rosenthal
Comment 1 2020-10-04 07:17:56 PDT
Noam Rosenthal
Comment 2 2020-10-04 23:05:33 PDT
EWS Watchlist
Comment 3 2020-10-04 23:06:32 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Noam Rosenthal
Comment 4 2020-10-06 03:02:15 PDT
Noam Rosenthal
Comment 5 2020-10-06 03:09:59 PDT
Noam Rosenthal
Comment 6 2020-10-06 10:06:04 PDT
The iOS layout test failure is a slight image-diff... I think the patch can be reviewed.
youenn fablet
Comment 7 2020-10-08 01:16:45 PDT
Comment on attachment 410623 [details] Patch iOS bot fails http/tests/security/canvas-remote-image-orientation.html. View in context: https://bugs.webkit.org/attachment.cgi?id=410623&action=review > Source/WebCore/html/HTMLImageElement.h:143 > + bool allowsOrientationOverride() const override; final? > Source/WebCore/rendering/RenderElement.cpp:2151 > + return orientation; Why not: return element() && !element()->allowsOrientationOverride() ? ImageOrientation::FromImage : style().imageOrientation();
Noam Rosenthal
Comment 8 2020-10-08 06:00:33 PDT
Noam Rosenthal
Comment 9 2020-10-08 08:44:20 PDT
youenn fablet
Comment 10 2020-10-08 08:59:24 PDT
Comment on attachment 410847 [details] Patch LGTM overall but I hope we can change http/tests/security/canvas-remote-image-orientation.html so that it passes on iOS. View in context: https://bugs.webkit.org/attachment.cgi?id=410847&action=review > Source/WebCore/ChangeLog:31 > + Only apply orientation for eligible images. Strange indentation here and above. > Source/WebCore/html/HTMLImageElement.cpp:690 > + return image->isOriginClean(&(document().securityOrigin())); Could be a one liner too. > LayoutTests/ChangeLog:11 > + Added a test to ensure image-orientation: none is not respected for remote images in canvas Strange indentation. > LayoutTests/imported/w3c/ChangeLog:10 > + Imported a new W3C test for remote image with image-orientation. Strange indentation. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:11 > +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ] Isn't there a way we could change the test slightly? It seems the WPT tests are passing, so maybe the idea is to change the canvas size? Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point).
Noam Rosenthal
Comment 11 2020-10-08 09:07:27 PDT
Comment on attachment 410847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410847&action=review >> LayoutTests/platform/ios-simulator-wk2/TestExpectations:11 >> +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ] > > Isn't there a way we could change the test slightly? > It seems the WPT tests are passing, so maybe the idea is to change the canvas size? > > Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point). There is already a wpt test for this. I wasn't aware of http/wpt, I'll import it there instead of the webkit-specific test. Should I land this with the test working on ios, or do you want to re-review once I have that ready?
youenn fablet
Comment 12 2020-10-08 09:08:53 PDT
> There is already a wpt test for this. I wasn't aware of http/wpt, I'll > import it there instead of the webkit-specific test. Should I land this with > the test working on ios, or do you want to re-review once I have that ready? I think the patch is good to land once the test works on iOS.
Noam Rosenthal
Comment 13 2020-10-08 10:49:50 PDT
Said Abou-Hallawa
Comment 14 2020-10-08 11:25:35 PDT
Comment on attachment 410861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410861&action=review > Source/WebCore/dom/Element.h:340 > + virtual bool allowsOrientationOverride() const { return true; } I do not think it is worthy adding this as a virtual method because it is overridden by one superclass which is HTMLImageElement. All it does is it saves you a casting to HTMLImageElement in RenderElement::imageOrientation(). > Source/WebCore/rendering/RenderElement.cpp:2143 > + return (element() && !element()->allowsOrientationOverride()) ? ImageOrientation(ImageOrientation::FromImage) : style().imageOrientation(); Can be written like this: auto* imageElement = is<HTMLImageElement>(element()) ? downcast<HTMLImageElement>(element()) : nullptr; return (imageElement && ...
Noam Rosenthal
Comment 15 2020-10-09 00:36:04 PDT
Created attachment 410920 [details] Patch for landing
EWS
Comment 16 2020-10-09 01:19:05 PDT
Committed r268249: <https://trac.webkit.org/changeset/268249> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410920 [details].
Radar WebKit Bug Importer
Comment 17 2020-10-09 01:20:21 PDT
Note You need to log in before you can comment on or make changes to this bug.