Bug 156790 – Position of WebGL <canvas> on iOS is incorrect with CSS borders

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Antoine Quint]

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.

[Radar WebKit Bug Importer]

<rdar://problem/25826290>

[Antoine Quint]

Per my testing, box-shadow and padding also cause problems.

[Antoine Quint]

Created attachment 280091 [details]
Test case for padding

[Antoine Quint]

Created attachment 280092 [details]
Testcase for box-shadow

[Antoine Quint]

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.

[Antoine Quint]

On the Mac, the WebGLLayer has contentsScale = 2 while on iOS it has contentsScale = 1.

[Antoine Quint]

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.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 280125 [details]
Patch

[Antoine Quint]

I misread the code in RemoteLayerTreeHost::createLayer(), WKRemoteViews are created not just for WebGL layers but also custom layers and video layers.

[Antoine Quint]

The changes were made as part of https://bugs.webkit.org/show_bug.cgi?id=136879.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 280144 [details]
Patch

[Antoine Quint]

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:.

[Darin Adler]

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?

[Dean Jackson]

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.

[Antoine Quint]

(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.

[Dean Jackson]

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.

[Antoine Quint]

Created attachment 280614 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280614 [details]
Patch

Clearing flags on attachment: 280614

Committed r201727: <http://trac.webkit.org/changeset/201727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.