| Summary: | Setting border-radius on <video> element clips top and left sections of video | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Hammer <jonathan> | ||||
| Component: | Compositing | Assignee: | 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
Jonathan Hammer
2019-09-20 11:27:09 PDT
I can reproduce. 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. 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. |