Bug 202049

Summary: Setting border-radius on <video> element clips top and left sections of video
Product: WebKit Reporter: Jonathan Hammer <jonathan>
Component: CompositingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dino, eric.carlson, esprehn+autocc, ews-watchlist, fred.wang, glenn, jer.noble, jonathan, kondapallykalyan, pdr, philipj, sabouhallawa, sergio, simon.fraser, thorton, tsavell, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199135
Attachments:
Description Flags
Patch commit-queue: commit-queue-

Jonathan Hammer
Reported 2019-09-20 11:27:09 PDT
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.
Attachments
Patch (12.29 KB, patch)
2019-10-19 21:54 PDT, Simon Fraser (smfr) 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-09-20 13:25:16 PDT
<rdar://problem/55570024>
Simon Fraser (smfr)
Comment 2 2019-10-07 10:56:20 PDT
I can reproduce.
Simon Fraser (smfr)
Comment 3 2019-10-19 14:11:11 PDT
Probably regressed in r246845.
Simon Fraser (smfr)
Comment 4 2019-10-19 21:54:07 PDT
Created attachment 381389 [details] Patch
zalan
Comment 5 2019-10-21 08:40:30 PDT
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 { }
Said Abou-Hallawa
Comment 6 2019-10-21 10:19:45 PDT
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.
WebKit Commit Bot
Comment 7 2019-10-21 10:37:58 PDT
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
Said Abou-Hallawa
Comment 8 2019-10-21 10:41:00 PDT
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?.
Simon Fraser (smfr)
Comment 9 2019-10-21 13:08:40 PDT
(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.
Simon Fraser (smfr)
Comment 10 2019-10-21 14:36:31 PDT
https://trac.webkit.org/changeset/251385/webkit
Simon Fraser (smfr)
Comment 12 2019-10-24 14:22:31 PDT
Fixed the test in https://trac.webkit.org/r251561.
Note You need to log in before you can comment on or make changes to this bug.