Bug 233026 – Web Inspector: Layers Tab: the position of composited layer with box-shadow is wrong

• Component: Web Inspector
• Status: RESOLVED FIXED

[Fujii Hironori]

Created attachment 444038 [details]
test case

Web Inspector: Layer Tab: the position of composited layer with box-shadow is wrong

1. Open the test case
2. Open Web Inspector
3. Open Layer tab
4. Select canvas element
5. the position of the composited layer is wrong

[Fujii Hironori]

Created attachment 444039 [details]
wrong screenshot

The orange layer of <canvas> is placed correctly on the webview.
But, the blue box layer of <canvas> in the layers tab is wrongly placed at the visible area of it.

[Fujii Hironori]

Comment on attachment 444039 [details]
wrong screenshot

My bad. This screenshot is wrong. the orange are is just a margin area. It's not a layer.

[Fujii Hironori]

Created attachment 444041 [details]
screenshot

Mac MiniBrowser can show Layer Border.
Because this <canvas> has a large box-shadow, it has a large layer.
Layers Tab shows <canvas> layer box placed wrongly at the visible position of the element.

[Fujii Hironori]

Created attachment 444044 [details]
Patch

[Blaze Burg]

Comment on attachment 444044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444044&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        If an elemenet has box-shadow, Layers Tab showed the composited

Nit: element

[Devin Rousso]

Comment on attachment 444044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444044&action=review

r- as this doesn't take into account older inspected targets, but the logic looks good otherwise :)

> Source/WebInspectorUI/UserInterface/Models/Layer.js:-46
> -        // FIXME: This should probably be moved to the backend.
> -        this._compositedBounds.x = this._bounds.x;
> -        this._compositedBounds.y = this._bounds.y;

This is not a valid fix when remotely inspecting a backend that does _not_ have the above change (e.g. inspecting an older iOS).  You'll need to add some sort of compatibility check.  AFAICT, the only thing that's changed in the protocol that's checkable is `InspectorBackend.Enum.CSS.PseudoId.Backdrop` (from r281229), so you'd wanna do something like this
```
    // COMPATIBILITY (iOS 15.0): the `bounds` and `computedBounds` properties of `LayerTree.Layer` did not account for parent element positioning.
    // Since support can't be tested directly, check for the `Backdrop` value in the `CSS.PseudoId` enum.
    // FIXME: Use explicit version checking once <https://webkit.org/b/148680> is fixed.
    if (!InspectorBackend.Enum.CSS.PseudoId.Backdrop) {
        this._compositedBounds.x = this._bounds.x;
        this._compositedBounds.y = this._bounds.y;
    }
```

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:272
> -            layerGroup.position.set(layer.bounds.x, -layer.bounds.y, index * zInterval);
> +            layerGroup.position.set(0, 0, index * zInterval);

ditto (Source/WebInspectorUI/UserInterface/Models/Layer.js:44)

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:289
> +    _createLayerMesh({x, y, width, height}, isOutline = false)

ditto (Source/WebInspectorUI/UserInterface/Models/Layer.js:44)

tho in this case it may be clearer to adjust the callsite instead 🤔

[Fujii Hironori]

Comment on attachment 444044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444044&action=review

>> Source/WebInspectorUI/UserInterface/Models/Layer.js:-46
>> -        this._compositedBounds.y = this._bounds.y;
> 
> This is not a valid fix when remotely inspecting a backend that does _not_ have the above change (e.g. inspecting an older iOS).  You'll need to add some sort of compatibility check.  AFAICT, the only thing that's changed in the protocol that's checkable is `InspectorBackend.Enum.CSS.PseudoId.Backdrop` (from r281229), so you'd wanna do something like this
> ```
>     // COMPATIBILITY (iOS 15.0): the `bounds` and `computedBounds` properties of `LayerTree.Layer` did not account for parent element positioning.
>     // Since support can't be tested directly, check for the `Backdrop` value in the `CSS.PseudoId` enum.
>     // FIXME: Use explicit version checking once <https://webkit.org/b/148680> is fixed.
>     if (!InspectorBackend.Enum.CSS.PseudoId.Backdrop) {
>         this._compositedBounds.x = this._bounds.x;
>         this._compositedBounds.y = this._bounds.y;
>     }
> ```

Good point. This issue can be fixed without the protocol change. I think it's better. Will fix.

[Fujii Hironori]

Created attachment 444184 [details]
Patch

[Devin Rousso]

Comment on attachment 444184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444184&action=review

r=me, nice!

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:296
> +            new THREE.Vector3(x,       -y-height, 0),
> +            new THREE.Vector3(x+width, -y-height, 0),
> +            new THREE.Vector3(x+width, -y,        0),

Style: please add spaces between operators
```
            new THREE.Vector3(x, -y - height, 0),
            new THREE.Vector3(x + width, -y - height, 0),
            new THREE.Vector3(x + width, -y, 0),
```

[Fujii Hironori]

Created attachment 444302 [details]
Patch for landing

Thank you for the review.

[EWS]

Committed r285839 (244271@main): <https://commits.webkit.org/244271@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444302 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85433224>