Setting border-radius on a <video> element to a value greater than zero causes large chunks of the video to be clipped incorrectly. Please see https://directmailmac.com/safari-13-bug.php for an example. On that page, the <video> element has a border-radius of 5px, but approx. 40px of the left and 30px of the top of the video are clipped off. The workaround (which can be seen at https://directmailmac.com) is to set the border-radius to zero.
<rdar://problem/55570024>
I can reproduce.
Probably regressed in r246845.
Created attachment 381389 [details] Patch
Comment on attachment 381389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381389&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:1934 > + contentsClippingRect.move(LayoutSize(-clipLayer->offsetFromRenderer())); moveBy()? or LayoutSize { }
Comment on attachment 381389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381389&action=review > Source/WebCore/platform/graphics/FloatRoundedRect.h:106 > + void setLocation(FloatPoint location) { m_rect.setLocation(location); } Can't we use setRect() or move() to change the location? Or can't we make rect() non-const and use rect().setLocation()? Maybe a better solution is to make FloatRoundedRect inherits FloatRect. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2648 > + clippingRectRelativeToClippingLayer.setLocation({ }); Shouldn't clippingRectRelativeToClippingLayer be translated to the coordinates of m_contentsClippingLayer instead of just making its location at { 0, 0 }? > LayoutTests/compositing/video/video-border-radius-clipping-expected.html:27 > + var totalCount = document.getElementsByTagName('video').length; > + var count = totalCount; There is only one <video> element in the <body> element. Why do we have to get the count and check if (!--count...) in the script below? > LayoutTests/compositing/video/video-border-radius-clipping.html:28 > + var totalCount = document.getElementsByTagName('video').length; > + var count = totalCount; Ditto.
Comment on attachment 381389 [details] Patch Rejecting attachment 381389 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 381389, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=381389&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=202049&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Updating working directory Processing patch 381389 from bug 202049. Fetching: https://bugs.webkit.org/attachment.cgi?id=381389 /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Updating OpenSource Current branch master is up to date. Full output: https://webkit-queues.webkit.org/results/13159497
I am sorry I did not mean to set r? There was a review conflict and I chose to submit it anyway. It seems this replaced the r+ with the r?.
(In reply to Said Abou-Hallawa from comment #6) > Comment on attachment 381389 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381389&action=review > > > Source/WebCore/platform/graphics/FloatRoundedRect.h:106 > > + void setLocation(FloatPoint location) { m_rect.setLocation(location); } > > Can't we use setRect() or move() to change the location? > > Or can't we make rect() non-const and use rect().setLocation()? Maybe a > better solution is to make FloatRoundedRect inherits FloatRect. I considered these. setLocation() seemed simplest. > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2648 > > + clippingRectRelativeToClippingLayer.setLocation({ }); > > Shouldn't clippingRectRelativeToClippingLayer be translated to the > coordinates of m_contentsClippingLayer instead of just making its location > at { 0, 0 }? clippingRectRelativeToClippingLayer was sized and positioned based on the rounded rect. > > LayoutTests/compositing/video/video-border-radius-clipping-expected.html:27 > > + var totalCount = document.getElementsByTagName('video').length; > > + var count = totalCount; > > There is only one <video> element in the <body> element. Why do we have to > get the count and check if (!--count...) in the script below? > > > LayoutTests/compositing/video/video-border-radius-clipping.html:28 > > + var totalCount = document.getElementsByTagName('video').length; > > + var count = totalCount; > > Ditto. Copy-pasted from another test.
https://trac.webkit.org/changeset/251385/webkit
The new test compositing/video/video-border-radius-clipping.html added in https://trac.webkit.org/changeset/251385/webkit is failing on Mojave wk1 History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=compositing%2Fvideo%2Fvideo-border-radius-clipping.html Diff: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r251477%20(8605)/compositing/video/video-border-radius-clipping-diffs.html It is unfortunately only missing by one pixel
Fixed the test in https://trac.webkit.org/r251561.