Created attachment 276814 [details] Testcase for border Adding a `border` CSS property on a <canvas> element does not correctly position the WebGL content of the canvas, showing it displaced compared to its background. This only happens on iOS and drawing into a WebGL context, OS X positions the content correctly and requesting a 2D context on iOS also draws the context in the right place.
<rdar://problem/25826290>
Per my testing, box-shadow and padding also cause problems.
Created attachment 280091 [details] Test case for padding
Created attachment 280092 [details] Testcase for box-shadow
It looks like the device pixel ratio may have something to do with this. If we measure the distance from the border to the WebGL layer (dGL) and the distance from the border to the background (dBG) I can see that on a 2x display dBG is 2x dGL while on a 3x display it is 3x dGL. Alas, I can't test on a 1x iOS display, but my guess is that the bug would not reproduce there.
On the Mac, the WebGLLayer has contentsScale = 2 while on iOS it has contentsScale = 1.
So, the contentsScale is set to the device pixel ratio on Mac only in `-(id)initWithGraphicsContext3D:(GraphicsContext3D*)context`, while we set a transform with a scale set to the device pixel ratio on iOS only in `PlatformCALayerRemoteCustom::PlatformCALayerRemoteCustom()`. Overriding `-(void)setPosition:(CGPoint)position` on WebGLLayer to account for the device pixel ratio fixes the issue, but I'm not sure this is the right fix.
I think the root of the issue is that WKRemoteView applies a scale that is the inverse of the scale factor in - (instancetype)initWithFrame:(CGRect)frame contextID:(uint32_t)contextID hostingDeviceScaleFactor:(float)scaleFactor and this causes the position to be divided by scale factor, which explains the offset varying with the device pixel ratio seen on iOS with WebGL layer.
Created attachment 280125 [details] Patch
I misread the code in RemoteLayerTreeHost::createLayer(), WKRemoteViews are created not just for WebGL layers but also custom layers and video layers.
The changes were made as part of https://bugs.webkit.org/show_bug.cgi?id=136879.
I was puzzled by why this didn't apply to <video> elements, and I found there was an ad-hoc fix for those in https://bugs.webkit.org/show_bug.cgi?id=144274 which introduces a container layer which does basically the same thing as my patch does, albeit more elegantly, by applying the transform when setting the position.
Created attachment 280144 [details] Patch
This patch is kind of an ad-hoc fix, much like the one that was made for <video>. I wonder if we should introduce a CALayer subclass that we would add as the layer for WKRemoteView and which would apply the transform, which should let the WebGLLayer and WebVideoContainerLayer be positioned without any manual application of the transform to the CGPoint passed to -setPosition:.
Comment on attachment 280144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review I’m saying review+ but I am not sure this is the best fix. > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185 > +- (void)setPosition:(CGPoint)position Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method. > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:192 > + if (!CATransform3DIsIdentity(self.transform)) { > + // Account for the transform applied to the WKRemoteView hosting the WebGLLayer > + // which applies a scale that is the inverse of the device pixel ratio. > + // https://bugs.webkit.org/show_bug.cgi?id=156790 > + position = CGPointApplyAffineTransform(position, CATransform3DGetAffineTransform(self.transform)); > + } Why do we need to check CATransform3DIsIdentity? Is this a performance optimization? Does it improve correctness?
Comment on attachment 280144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review >> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185 >> +- (void)setPosition:(CGPoint)position > > Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method. I agree with Darin. I'm also worried about a few things: - If the transform changes, is setPosition always called? If not, the layer will be in the wrong place. - This seems to be an issue in the WKRemoteView. Maybe it should special-case WebGLLayer? Why aren't other WKRemoteViews having the same problem? - (Something I've been meaning to fix for a while) It's annoying that we have iOS-only code in platform/graphics/mac.
(In reply to comment #16) > Comment on attachment 280144 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280144&action=review > > >> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185 > >> +- (void)setPosition:(CGPoint)position > > > > Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method. > > I agree with Darin. I think I'll investigate adding a container layer for WKRemoteView which applies the inverse transform and let sublayers of that container be positioned, which would correctly position things with respect to the page scale factor. > I'm also worried about a few things: > - If the transform changes, is setPosition always called? If not, the layer > will be in the wrong place. > - This seems to be an issue in the WKRemoteView. Maybe it should > special-case WebGLLayer? Why aren't other WKRemoteViews having the same > problem? They do, see my comment above about WebVideoContainerLayer.
Comment on attachment 280144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280144&action=review >>>> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:185 >>>> +- (void)setPosition:(CGPoint)position >>> >>> Seems a peculiar choice for this adjustment to be done inside the setPosition method. I would have expected this code to instead be at the call sites that are setting the position or in a helper function or new method that we add. This change would not be correct if any code other than the code in WebKit was calling the setPosition: method. >> >> I agree with Darin. >> >> I'm also worried about a few things: >> - If the transform changes, is setPosition always called? If not, the layer will be in the wrong place. >> - This seems to be an issue in the WKRemoteView. Maybe it should special-case WebGLLayer? Why aren't other WKRemoteViews having the same problem? >> - (Something I've been meaning to fix for a while) It's annoying that we have iOS-only code in platform/graphics/mac. > > I think I'll investigate adding a container layer for WKRemoteView which applies the inverse transform and let sublayers of that container be positioned, which would correctly position things with respect to the page scale factor. Sorry, I didn't notice your comment earlier on bugzilla. Yeah, I like this idea. Hopefully that means we can be certain that transform and position are in sync.
Created attachment 280614 [details] Patch
Comment on attachment 280614 [details] Patch Clearing flags on attachment: 280614 Committed r201727: <http://trac.webkit.org/changeset/201727>
All reviewed patches have been landed. Closing bug.