1355675 - Provide a ChromeOnly API for Element to report its root-relative transform matrix

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Brad Werth [:bradwerth]]

Devtools draws overlays on top of elements, and most of the time it uses bounding corners of transformed quads to do this. There are some cases where the overlay needs more information than the bounding quad (like drawing grid lines on a display:grid element), and to handle that we need to provide the transform matrix.

Comment 1

[Robert Longson [:longsonr]]

Something like https://www.w3.org/TR/SVG/types.html#__svg__SVGLocatable__getTransformToElement but for all elements?

Note that our implementation of getTransformToElement only handles SVG transforms and not CSS transforms currently but it could be extended to do so. Perhaps it could be made Chrome only for elements that don't implement SVGLocatable and content accessible to those that do.

Alternatively perhaps doing a similar thing to getScreenCTM() would more meet your needs.

Comment 2

[Brad Werth [:bradwerth]]

nsLayoutUtils::GetTransformToAncestor seems helpful for implementing this.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1355675 Part 2: Extend DOMMatrixReadOnly to allow instantiation with a Matrix4x4.

Review commit: https://reviewboard.mozilla.org/r/137706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137706/

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

Review commit: https://reviewboard.mozilla.org/r/137708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137708/

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Review commit: https://reviewboard.mozilla.org/r/137710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137710/

Comment 6

[Brad Werth [:bradwerth]]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137708/diff/1-2/

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137710/diff/1-2/

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Matteo, is this the sort of thing you're looking for to draw more accurate overlays in dev tools?

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1355675 Part 1: Add optional inCSSUnits parameters to GetTransformMatrix and GetTransformToAncestor.

Review commit: https://reviewboard.mozilla.org/r/142980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142980/

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8866104 [details]
Bug 1355675 Part 2: Extend DOMMatrixReadOnly to allow instantiation with a Matrix4x4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137706/diff/1-2/

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137708/diff/2-3/

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137710/diff/2-3/

Comment 13

[Brad Werth [:bradwerth]]

Matteo, are these methods useful for you?

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8871512 [details]
Bug 1355675 Part 1: Add optional inCSSUnits parameters to GetTransformMatrix and GetTransformToAncestor.

https://reviewboard.mozilla.org/r/142980/#review147378

::: layout/base/nsLayoutUtils.h:856
(Diff revision 1)
>     * Gets the transform for aFrame relative to aAncestor. Pass null for
>     * aAncestor to go up to the root frame.
>     */
> -  static Matrix4x4 GetTransformToAncestor(nsIFrame *aFrame, const nsIFrame *aAncestor);
> +  static Matrix4x4 GetTransformToAncestor(nsIFrame *aFrame,
> +                                          const nsIFrame *aAncestor,
> +                                          bool aInCSSUnits = false);

Please add a comment about this new parameter.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8866104 [details]
Bug 1355675 Part 2: Extend DOMMatrixReadOnly to allow instantiation with a Matrix4x4.

https://reviewboard.mozilla.org/r/137706/#review147382

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

https://reviewboard.mozilla.org/r/137708/#review147384

::: dom/base/Element.cpp:3429
(Diff revision 3)
>  }
>  
> +already_AddRefed<DOMMatrixReadOnly>
> +Element::GetTransformToAncestor(Element& aAncestor)
> +{
> +  nsIFrame* primaryFrame = GetPrimaryFrame();

GetPrimaryFrame is fallible, so you want to check for that, here and elsewhere.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Matt should probably review this

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

https://reviewboard.mozilla.org/r/137708/#review147490

::: dom/webidl/Element.webidl:171
(Diff revision 3)
>    [ChromeOnly, Pure]
>    sequence<Grid> getGridFragments();
> +
> +  [ChromeOnly]
> +  DOMMatrixReadOnly getTransformToAncestor(Element ancestor);
> +  DOMMatrixReadOnly getTransformToParent();

So you expose getTransformToParent() and 
getTransformToViewport() to the web.

We don't want that.

Comment 19

[Brad Werth [:bradwerth]]

Comment on attachment 8871512 [details]
Bug 1355675 Part 1: Add optional inCSSUnits parameters to GetTransformMatrix and GetTransformToAncestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142980/diff/1-2/

Comment 20

[Brad Werth [:bradwerth]]

Comment on attachment 8866104 [details]
Bug 1355675 Part 2: Extend DOMMatrixReadOnly to allow instantiation with a Matrix4x4.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137706/diff/2-3/

Comment 21

[Brad Werth [:bradwerth]]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137708/diff/3-4/

Comment 22

[Brad Werth [:bradwerth]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137710/diff/3-4/

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866105 [details]
Bug 1355675 Part 3: Add some Chrome-only getTransformTo... methods to Element.

https://reviewboard.mozilla.org/r/137708/#review148050

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

I really think Matt should review the tests since he know how the (ChromeOnly) API should work.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8866106 [details]
Bug 1355675 Part 4: Add tests of Element::getTransformTo... methods.

https://reviewboard.mozilla.org/r/137710/#review148256

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a204d31127ce
Part 1: Add optional inCSSUnits parameters to GetTransformMatrix and GetTransformToAncestor. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/ad325c236e95
Part 2: Extend DOMMatrixReadOnly to allow instantiation with a Matrix4x4. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fcfd741290a4
Part 3: Add some Chrome-only getTransformTo... methods to Element. r=mattwoodrow,smaug
https://hg.mozilla.org/integration/autoland/rev/cb9066e8f9f3
Part 4: Add tests of Element::getTransformTo... methods. r=mattwoodrow

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a204d31127ce
https://hg.mozilla.org/mozilla-central/rev/ad325c236e95
https://hg.mozilla.org/mozilla-central/rev/fcfd741290a4
https://hg.mozilla.org/mozilla-central/rev/cb9066e8f9f3

Comment 28

[Matteo Ferretti [:zer0] [:matteo]]

Forgot to remove the ni here, me and Brad discussed that by email and over vidyo, providing the examples needed.