1297072 - The CSS Grid Inspector should take transforms into account

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Patrick Brosset <:pbro>]

Attachment: transformed-grid.PNG

STR:

- open http://labs.jensimmons.com/examples/grid-content-1-rotated.html
- display the CSS Grid highlighter on the <ul> element in the page

Notice how the grid highlighter doesn't match the actual grid on the page.
See the screenshot.

Comment 1

[Patrick Brosset <:pbro>]

Maybe canvas' matrix transform function can help here.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Patrick Brosset <:pbro>]

I have started to work on this today and have something that almost works.

Let's assume we're only dealing with rows, not columns.
Let P1 and P2 be the start and end points of 1 of the row lines we're drawing.
If the element that the grid is defined in is transformed, we need the "real" (transformed) coordinates of P1 and P2.

Here's the information we have at our disposal to determine this:

- The Y position of the line P1-P2 relative to A-C (its vertical offset). getGridFragments gives this to us (line.start).
- The list of all row lines in the grid. In the simplified figure below, that
  would be A-C, P1-P2, B-D.
- The real coordinates of the element (getBoxQuads gives us the coordinates of the 4 points, after transformation).

 A +------+------+ C
   |      |      |
P1 +------+------+ P2
   |      |      |
 B +------+------+ D

Using the data from getGridFragments, we know the the length of A-B (since we know the offset for all lines in the grid), and we know the length of A-P1.

And using getBoxQuads, we know the actual length of A-B, after transformation, and the actual coordinates of A and B.

So, with a little bit of math, we can deduce the actual coordinates of P1 too.
And then, focusing on line C-D, we can do the same for P2.

Now, this works almost perfectly, but there's a problem when the grid doesn't fully cover the element, or extends out of it.
In other words, the logic above assumes that the rectangle ABCD fully covers the element, which it might not. Depending on the CSS grid-template-rows/columns, the grid may be smaller or bigger, and therefore our math to deduce the coordinates is incorrect.

getGridFragments is nice because it gives us distances in an untransformed scale, so we can compare that to the actual transformed element's measurements to correctly position the lines in the page, but we miss the untransformed width and height of the element. 
Alternatively, if getGridFragments gave us transformed coordinates, that would be even simpler for us. But I'm not sure that is feasible.

Brad: any thoughts about this? Do you think getGridFragments could return, for each fragment, something like this:
{areas, cols, rows, element}
where areas, cols and rows are what it already returns, and element would simply be {width, height} but on the scale as the other distances already returned by the API?

Comment 4

[Brad Werth [:bradwerth]]

Seems doable. Since I'm not that familiar with the CSS styles that can affect transformed coordinates, it would help me immensely if you could provide a testcase that sets up such a scenario. Also: if styling can affect the transformed width and height of the grid within the element, can it also affect the offset? If it can, then it seems we would need to return a full rect (also doable) and not just the transformed size. If that's possible, a testcase demonstrating that would be even more useful for me.

Comment 5

[Brad Werth [:bradwerth]]

Sorry, I see the existing testcase now. I'll use this and see what I can do to solve the problem.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: test_grid_transform.html

Gabe, I have built a test and I need you to verify that it matches your intent. Presuming that we have a grid template of:

+----+----+
| 60 | 60 |
+----+----+
| 60 | 60 |
+----+----+

...with all cells square and a 30 pixel gap between cells. In such a case, an untransformed full grid would have dimensions 150 x 150. The test case tests four versions of this grid with these expected values:

1) Only top left cell filled: 150 x 150.
2) All cells filled plus one more added in new row: 150 x 240.
3) All cells filled with a 2x scale transform applied: 150 x 150.
4) All cells filled with a 45 degree rotate applied: 150 x 150.

If I create an API that produces these values, does that meet the need expressed in this bug? I'm skeptical that this is what you want, because these values are exactly what you would get if you added the breadth of all the tracks and lines together.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Brad Werth [:bradwerth] from comment #6)
> Created attachment 8797714 [details]
> test_grid_transform.html
> 
> Gabe, I have built a test and I need you to verify that it matches your
> intent. Presuming that we have a grid template of:
> 
> +----+----+
> | 60 | 60 |
> +----+----+
> | 60 | 60 |
> +----+----+
> 
> ...with all cells square and a 30 pixel gap between cells. In such a case,
> an untransformed full grid would have dimensions 150 x 150. The test case
> tests four versions of this grid with these expected values:
> 
> 1) Only top left cell filled: 150 x 150.
> 2) All cells filled plus one more added in new row: 150 x 240.
> 3) All cells filled with a 2x scale transform applied: 150 x 150.
> 4) All cells filled with a 45 degree rotate applied: 150 x 150.
> 
> If I create an API that produces these values, does that meet the need
> expressed in this bug? I'm skeptical that this is what you want, because
> these values are exactly what you would get if you added the breadth of all
> the tracks and lines together.

Hi Brad, I haven't focus on what we need to get CSS grid highlights working with transform. I will need some time to catch up on the discussion and run some tests to see what we need. I am gonna add a needinfo to pbro to see if he can fill in some details for the time being, and I will also be talking to pbro tomorrow to get a bit more caught up while we are at our 1:1. So, I would hope to be able to give you an answer by tomorrow.

My naive answer is that we were looking for the transformed coordinates/sizes and it would've just worked with the existing way we are currently drawing the lines on the css grid highlighter.

Comment 8

[Patrick Brosset <:pbro>]

I think the problem we have today is: we assume the element is untransformed. And so we only just draw straight lines, and we use getGridFragments to know where to start drawing them.

If getGridFragments would return something more along the lines of what el.getBoxQuads does, that is: the actual geometry of the grid, as it appears in the page, then that'd make it very easy to fix this problem.

But that would require much more than just track breadths. It would require a pair of x/y coordinates per line.
And, although I haven't looked at how getGridFragments is implemented, I have an impression that it just returns the computed track breadth, so I'm not sure it can retrieve the geometry for the grid once the page has been rendered.

One solution I just thought about just now. Why not apply to the overlay the same transform/transform-origin/transform-style/perspective that is applied to the element itself?

It might be a bit hard because the grid is positioned relative to the parent document, so tranform-origin for example may not match, but this way we could draw the grid exactly as if the element was untransformed (like today), and then just apply the right CSS transform on it to make it match the element's final geometry.

This needs some experimentation, but this would be by far the easiest.

Comment 9

[Brad Werth [:bradwerth]]

Thanks for clarifying what is needed. I'll see how hard it would be to get transformed bounding box coordinates similiar to what getBoxQuads delivers, which would suffice for all affine transforms -- everything in spec except for matrix, skew, and perspective.

Comment 10

[Mats Palmgren (inactive)]

Fwiw, using the same coordinates as getBoxQuads sounds right to me.
We probably have layout code for doing that transformation already.

Comment 11

[Brad Werth [:bradwerth]]

This will need to be implemented in Gecko, so I'll take the bug back.

Comment 12

[Brad Werth [:bradwerth]]

(In reply to Patrick Brosset <:pbro> from comment #8)
> I think the problem we have today is: we assume the element is
> untransformed. And so we only just draw straight lines, and we use
> getGridFragments to know where to start drawing them.
> 
> If getGridFragments would return something more along the lines of what
> el.getBoxQuads does, that is: the actual geometry of the grid, as it appears
> in the page, then that'd make it very easy to fix this problem.

I started implementing this, and realized that my implementation WAS just calling getBoxQuads on the element that holds the "display:grid" style. You have this information already. Behold:

1) Navigate to http://labs.jensimmons.com/examples/grid-content-1-rotated.html
2) Open Developer Console
3) Type in: document.getElementsByTagName("ul")[0].getBoxQuads()
4) The array has one DOMQuad in it that gives you the four points of the transformed element and thus the grid itself (I think).
5) You can do math on the DOMQuad such as finding the distance between TL and TR points in the quad, and dividing by the sum of the track and gap lengths of the grid to get a ratio of DOM coords to grid coords, and then you can interpolate points along the TR -> TL line for each grid cell and grid gap, and of course do likewise for the other boundary lines.

If this is not sufficient, or if there are boundary cases where getBoxQuads will not give you this data, please make clear what other values you need to draw the overlay. I don't think there's any benefit in me adding duplicate values as properties to the grid object.

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Brad Werth [:bradwerth] from comment #12)
> 5) You can do math on the DOMQuad such as finding the distance between TL
> and TR points in the quad, and dividing by the sum of the track and gap
> lengths of the grid to get a ratio of DOM coords to grid coords, and then
> you can interpolate points along the TR -> TL line for each grid cell and
> grid gap, and of course do likewise for the other boundary lines.
This is what I had started to do. See comment 3.
I'm totally ok doing this on DevTools side. This is just some math to do, and getBoxQuads and getGridFragments provide enough info here.
But, I did find one problem though:
(from comment 3)
> Now, this works almost perfectly, but there's a problem when the grid doesn't fully cover the element,
> or extends out of it.
The grid definition might be such that it doesn't have the same size as the grid container. And that makes it impossible to correctly calculate the rest. 
Unless I'm missing something.

Comment 14

[Brad Werth [:bradwerth]]

(In reply to Patrick Brosset <:pbro> from comment #13)

> The grid definition might be such that it doesn't have the same size as the
> grid container. And that makes it impossible to correctly calculate the
> rest. 
> Unless I'm missing something.

You're right -- that screws up the math. Let's figure this out. I think the only difference the grid could have relative to the element is one of scale (when grid dimensions have non-auto size and the element has a non-auto size). There's no grid property that could translate or rotate the grid relative to the element, for example. So we're just trying to calculate a scaling factor. That leads me to...

Assertion: The top left of the grid will always be the same as the top left of the element.

Is there any case where that's wrong?

So assuming that, then we should be able to get the getBoxQuads of the element, compute the TL -> TR distance, divide it by the clientWidth of the element, and get a scaling factor of transformed width to styled width, which I'll call transformedRatio. Then we'll get the scaling factor of the styled width to the grid width by dividing sum of grid tracks and gaps by clientWidth, which gives us gridRatio.

Now, with grid overlay origin at TL point, we can draw the width of the entire grid by taking the TL -> TR vector, multiply by gridRatio, and add it back to TL point.

Anyway, my math may be screwy in some of this analysis, but I believe that all the values are present between the getBoxQuads and clientWidth and clientHeight methods/properties. That math can be done on the JS side and probably should be done there.

Comment 15

[Brad Werth [:bradwerth]]

Attachment: gridTransformMath.html

Demonstrates the JS math described in comment 14.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Another test page http://meyerweb.com/eric/css/tests/firefox/grid-scale.html

Comment 17

[Brad Werth [:bradwerth]]

This is solvable with existing API in JS (see comment 14 and attachment 8835670 [details]), so I'm taking myself off the bug.

Comment 18

[Jen Simmons [:jensimmons]]

Here's another test case: http://labs.jensimmons.com/2016/

(Current status in FF52: https://monosnap.com/file/Mexcoc7RNX0BWQ6b1gRUt5EAkJvcRW.png)

Comment 19

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

Attachment: Screenshot of Grid's WIP

After hitting few walls, I finally got a kind of Eureka moment and understand why in all my prototypes everything worked, but put the same logic in Grid Inspector didn't. I attach a screenshot so show the current state of the things. 

As you can see the grid now is covering the element even if it's translated, rotated, scaled, or skewed; without affecting the line rendering (one of the problems of using the canvas' matrix transform function, was that with a scaled element also the lines were scaled, where we want actually scale only the coordinates where to put those lines, also to understand where to put the numbers).

As you can see, there are still few things to handle:
1. The gaps are not affected.
2. The line numbers are not affected – and should affect just partially, in term of position but not in term of size / skew / scaling.

Still few things to handle (that you can't see):
3. Currently it works only if the transformation is applied to the grid's element it self – easy to fix, but it requires walk all the ancestors of the element.

4. If we have the element and ancestors with transformations (e.g. the element is translated, the parent is rotated, and one of the ancestors is rotated as well and translated); we should applied the resulting transformation.

5. It doesn't support 3D transformation.

6. The code is a mess.

Points 1 & 2 are mandatory to fix of course. I can fix also bug 3 & 4 (but it will impact the performance, hope not too much).
I wouldn't address in this patch the point 5. We can file a bug for 3d transformation.

Another problem here is that it won't necessary works well with fragments, and still investigating here if it's that the case. However, in that case we might leave this open to fix in a follow up bug.

Comment 20

[Jen Simmons [:jensimmons]]

Gosh that attachment 8854714 [details] is exciting!! 

So exciting I had to tweet it. I'd realized of course that we can apply rotation to Grid, but I hadn't thought about Skew and such to get a layout to tip into 3D space. Wowzer! Now I need to go make another 14 demos of what Grid can do. And all that from a screenshot of a tool that doesn't exist (for me) yet. I can't wait until the world can use these tools. You are inspiring future great design. Thanks to everyone working on this. It's big!

Comment 21

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

Attachment: Bug 1297072 - added 2d matrix utility functions;

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

Comment 22

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

Attachment: Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

- Fixed a bug on `getNodeBounds` that would makes the calculation wrong in case
  of nested frames.
- Centralized all the transformation in `updateCurrentMatrix` function,
  including the scaling due the zoom and display's pixel ratio, and the
  translation to the top left corner of the node inspected.
- Added the transformation from the inspected node to the `currentMatrix`.
- Added `drawLine` and `drawRect` functions, that takes a matrix as argument.
- Position the line's number to the grid itself even when we've infinite lines
  (it's not a regression, it is intended since if a grid is transformed, we
  could have weird results otherwise, so we decided to uniform the behaviors).

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

Comment 23

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

Comment on attachment 8860449 [details]
Bug 1297072 - added 2d matrix utility functions;

Hi Greg, just a couple of note about this patch: I discovered only later that glMatrix has a `mat2d` module. Now, I don't think we should import glMatrix in our codebase just for that, since we do not use anywhere else matrices atm; and since we don't really need huge performance there we could be leave the code as is. I know we could get rid of the `0, 0, 1`, for example, and reuse the arrays instead of creating new ones (not sure if it would make sense using `Float32Array`). For that reason I was in doubt if copy & paste the functions we need, or not. I decided to push the review as is, so that we could discuss there what would be better to do.

Comment 24

[Greg Tatum [:gregtatum]]

Comment on attachment 8860449 [details]
Bug 1297072 - added 2d matrix utility functions;

https://reviewboard.mozilla.org/r/132440/#review135326

Looks good! I had a few comments, but I'll leave it up to you how/if you address them.

::: devtools/shared/layout/dom-matrix-2d.js:46
(Diff revision 1)
> +exports.translate = translate;
> +
> +/**
> + * The identity matrix.
> + */
> +exports.identity = scale();

I'd be careful with this, since it's mutable. I think I'd prefer a new copy every time to be safe.

```javascript
exports.identity = () => [
  1, 0, 0,
  0, 1, 0,
  0, 0, 1
];
```

::: devtools/shared/layout/dom-matrix-2d.js:103
(Diff revision 1)
> + * @param {DOMNode} node
> + *        The node.
> + * @return {Array}
> + *        The transformation origin point.
> + */
> +function getNodeTransformOrigin(node) {

This function isn't as general as it sounds, transform origin can have many non-numeric and percentage based values.

https://developer.mozilla.org/en-US/docs/Web/CSS/transform-origin?v=example

I'd annotate this more since it's in a shared folder and not completely owned by the code that's using it.

::: devtools/shared/layout/dom-matrix-2d.js:118
(Diff revision 1)
> + * @param {DOMNode} node
> + *        The node.
> + * @return {Array}
> + *        The transformation matrix.
> + */
> +function getNodeTransformationMatrix(node) {

Like above, I'm a tad nervous that this assumes a well-formed transformation matrix, as transform can have other values. I haven't looked at where and how you are using it though.

Comment 25

[Greg Tatum [:gregtatum]]

glMatrix is for realtime results, and limiting garbage collection. I think your approach is probably easier to maintain for our purposes. We're not building a 3d engine :)

Comment 26

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

Comment on attachment 8860449 [details]
Bug 1297072 - added 2d matrix utility functions;

https://reviewboard.mozilla.org/r/132440/#review135734

Comment 27

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

Comment on attachment 8860449 [details]
Bug 1297072 - added 2d matrix utility functions;

https://reviewboard.mozilla.org/r/132440/#review135326

> I'd be careful with this, since it's mutable. I think I'd prefer a new copy every time to be safe.
> 
> ```javascript
> exports.identity = () => [
>   1, 0, 0,
>   0, 1, 0,
>   0, 0, 1
> ];
> ```

Yeah, I wasn't sure about this myself. I added `identity` as function in the same way of `scale` and `translation`.

> This function isn't as general as it sounds, transform origin can have many non-numeric and percentage based values.
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/transform-origin?v=example
> 
> I'd annotate this more since it's in a shared folder and not completely owned by the code that's using it.

It is! :) That's the wonderful thing of this approach: since I'm accessing at the css property via computed style, the values are always computed as numeric and pixel as unit; it doesn't matter if transform-origin is set or which value are given.

> Like above, I'm a tad nervous that this assumes a well-formed transformation matrix, as transform can have other values. I haven't looked at where and how you are using it though.

The same is also applying here: since we're accessing to the computed style, we always get a `matrix`, `matrix3d` (in case of 3D transformation), or `none`. For instance, if you have in an element: `transform: translate(100em, 20%) scale(0.5)` you will always obtain the resulting matrix if you access to the computed style.

Comment 28

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

Comment on attachment 8860449 [details]
Bug 1297072 - added 2d matrix utility functions;

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

Comment 29

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

Comment on attachment 8860450 [details]
Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

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

Comment 30

[Greg Tatum [:gregtatum]]

> It is! :) That's the wonderful thing of this approach: since I'm accessing at the css property via computed style, the values are always computed as numeric and pixel as unit; it doesn't matter if transform-origin is set or which value are given.

Oh sweet, I didn't know that. That all works for me then! Thanks Matteo.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8860450 [details]
Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

https://reviewboard.mozilla.org/r/132442/#review136168

Looks great!
I made a few comments below. Mostly about code comments.

I tested the patches locally and found one issue:
Cell highlighting does not take transform into account. Go to http://labs.jensimmons.com/2016/examples/grid-content-1-rotated.html, go to the layout panel, highlight the grid, move your mouse over the grid outline that appears in the panel --> you'll see that cells become highlighted in the page, but they're not at the right place. 

Finally, what tests do you think could be written for this? Do we need unit tests for the matrix module? Do you think we could write a devtools mochitest for the feature as a whole?

::: commit-message-9bd3e:1
(Diff revision 2)
> +Bug 1297072 - The CSS Grid Inspector should take transforms into account; r=pbro

Please don't just re-state the bug title here. This first line should be a very short summary of what you fixed here.

::: devtools/server/actors/highlighters/css-grid.js:86
(Diff revision 2)
>  // the displayport API instead.
>  //
>  // Using a fixed value should also solve bug 1348293.
>  const CANVAS_SIZE = 4096;
>  
> +// This constant is used as value when the grid wants to draw infinite lines.

Please be more explicit in this comment. Explain that we need a value for when we draw infinite lines in transformed css grids, because ...

::: devtools/server/actors/highlighters/css-grid.js:115
(Diff revision 2)
> +  ctx.lineTo(Math.round(toPoint[0]), Math.round(toPoint[1]));
> +}
> +
> +/**
> + * Draws a rect to the context given, applying a transformation matrix if passed.
> + * The coordinates are related to the rectangle's diagonal.

Some rephrasing needed to make this more ovious: "The coordinates are the start and end points of the rectangle's diagonal.

::: devtools/server/actors/highlighters/css-grid.js:120
(Diff revision 2)
> + * The coordinates are related to the rectangle's diagonal.
> + *
> + * @param {CanvasRenderingContext2D} ctx
> + *        The 2d canvas context.
> + * @param {Number} x1
> + *        The x-axis of the coordinate for the begin of the rectangle's diagonal.

Some rephrasing here too (same in the next 3 params):
"The x-axis coordinate of the rectangle's diagonal start point"

::: devtools/server/actors/highlighters/css-grid.js:735
(Diff revision 2)
>  
>      // Clear the grid area highlights.
>      this.clearGridAreas();
>      this.clearGridCell();
>  
> +    // Updates the current matrix used in our canvas' rendering

s/Updates/Update

::: devtools/server/actors/highlighters/css-grid.js:954
(Diff revision 2)
>    },
>  
> +  updateCurrentMatrix() {
> +    let origin = getNodeTransformOrigin(this.currentNode);
> +    let bounds = getNodeBounds(this.win, this.currentNode);
> +    let nodeMatrix = getNodeTransformationMatrix(this.currentNode);

I don't know if this is the right place for such a comment, but we do need a comment somewhere that lists the edge cases not currently covered by this solution. In particular here, we only get the transformation matrix for the current node, not for its containers.

::: devtools/server/actors/highlighters/css-grid.js:1227
(Diff revision 2)
> +    [x, y] = apply(this.currentMatrix, [x, y]);
> +
> +    x -= boxWidth / 2;
> +    y -= boxHeight / 2;
> +
> +    let hasNodeTransformations = getNodeTransformationMatrix(this.currentNode) !== null;

Is this costly? If we wanted to avoid calling this once when we update the matrix and then once per line number, we could just store a boolean on `this` when `updateCurrentMatrix` runs.

Comment 32

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

Comment on attachment 8860450 [details]
Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

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

Comment 33

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

Comment on attachment 8860450 [details]
Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

https://reviewboard.mozilla.org/r/132442/#review138554

Hope I addressed all the comments – bug 1359522 contains the tests requested, already r+ from greg.

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8860450 [details]
Bug 1297072 - added support for matrices to handle CSS 2D transformation on grid inspector;

https://reviewboard.mozilla.org/r/132442/#review138780

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1fb6ea7a81
added 2d matrix utility functions; r=gregtatum
https://hg.mozilla.org/integration/mozilla-inbound/rev/43cf95c025b8
added support for matrices to handle CSS 2D transformation on grid inspector; r=pbro

Comment 36

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/5e1fb6ea7a81
https://hg.mozilla.org/mozilla-central/rev/43cf95c025b8