1343447 - Maintain aspect ratio of grid outline

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Micah Tigley [:micah]]

The grid outline's aspect ratio does not honour the grid that is currently highlighted on the page.

Comment 1

[Micah Tigley [:micah]]

Attachment: Bug 1343447 - Rebase maintain aspect ratio patch.

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

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review120452

::: devtools/client/inspector/grids/components/GridOutline.js:85
(Diff revision 1)
> -    const width = 10;
> -    const height = 10;
> +    let width = 20;
> +    let height = 20;

It would be great if you could moved these as constants at the top of this file. This way it's easier to know what these are, because the constant name can be self explanatory, and you can add comments.

::: devtools/client/inspector/grids/components/GridOutline.js:94
(Diff revision 1)
> +    let borderHeight = 0;
> +    let borderWidth = 0;
>  
> -    // Draw the cells within the grid outline border.
> +    // Draw the cells contained within the grid outline border.
>      for (let rowNumber = 1; rowNumber <= numberOfRows; rowNumber++) {
> +      height = 20 * (rows.tracks[rowNumber - 1].breadth / 100);

Same here, the number 20 isn't explained. It might be hard for someone reading the code for the first time to know why we have it.
Please use a constant instead here.

::: devtools/client/inspector/grids/components/GridOutline.js:114
(Diff revision 1)
> +      borderWidth += 20 * (cols.tracks[columnNumber].breadth / 100);
> +    }
> +
> +    // Store the height of the grid container in the
> +    // component state to prevent overflow issues
> +    if (borderHeight > this.state.height) {
> +      this.setState({height: borderHeight + 20});

A couple more 20 magic numbers here.

::: devtools/client/inspector/grids/components/GridOutline.js:219
(Diff revision 1)
> -    return this.state.selectedGrids.length ?
> +    const { selectedGrids } = this.state;
> +    return selectedGrids.length ?
>        dom.svg(
>          {
>            className: "grid-outline",
> -          width: "100%",
> +          width: 500,

And another one here. Should this be just VIEWPORT_WIDTH?

Also, why is this 500px in fact? If I make the sidebar smaller than 500px, then the outline disappears below the scrollable area.
Finally, the outline isn't centered in the sidebar.
It sits about 200px from the left of the panel and stays there.

It would be much better if the outline was responsive instead.
I know this is not the focus of your commit, but since you've changed 100% to 500px, I think we need to discuss this here.

Now, this is a problem because we want to preserve aspect ratio here.
But I'm saying it would be nice if the outline was responsive with the width of the sidebar panel. But if it is, and we want aspect ratio, then the height is going to change too, and that's not good, cause it might be too big in some cases.

But I think this is also a problem now. We have a hardcoded width of 500px. So in some cases, the grid may actually be too small to see in the outline, or way too big.

What if I have a grid in an element that is 50px by 50px? Then I can barely see the outline.
I don't think this is an edge case. People use flexbox on tiny elements just to get some of the alignment properties. I suspect they will do the same with grid.

What if I have a huge grid instead: 2000px by 5000px? Then the outline is so tall that it pushes the rest of the panel down, and causes a scrollbar to appear.
This, also, is not really an edge case, css grid is being publicized as a good way to handle page layouts, so very long grids will happen.

So, my point is, there are (common?) cases where the outline is going to either be hard to use at all, or will make the rest of the panel hard to use.
Preserving aspect ratio sounds like a useful feature, but only up to a point.
I think we might need to add some min/max sizes here. So that the outline is never too small or too big, but always sort of occupies the same area of the panel.

Comment 3

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

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

Comment 4

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

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

Gonna leave the review to pbro.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review123388

Thanks, this looks much better now.
I have 4 main comments about the outline:

- There's a bug that prevents the outline from nicely resizing to occupy the available space sometimes. I think this should be fixed before landing thid patch. Here's how to reproduce the bug:
1. load this URL in a tab:
data:text/html,<div test="test" class="t" style="display:grid;grid-template-rows:repeat(20, 100px);grid-template-columns:repeat(2, 100px);">
2. open the layout panel and click on the grid checkbox to display the outline
3. in the inspector, edit the style attribute on the element and change it to "display:grid;grid-template-rows:repeat(2, 100px);grid-template-columns:repeat(2, 100px);"
4. press enter

In this case, the grid updates in the page to only have 2 rows, it also updates in the outline, but it doesn't change its zoom factor, so it looks really small now.

- The second comment I have is about extreme cases. If a grid has something like 50 rows and 50 columns, then the cells in the outline are going to be so small that the outline is basically going to be unusable. Similarly, if the grid has 1 thin column, and 100 rows, it's going to be so small that it'll also be unusable.
I'm willing to not block this  bug on this however. The outline is behind a pref for now. So please file another bug that blocks bug 1347964 for this.
My suggestion for that bug would be to just hide the outline in these extreme cases. Basically, if a cell is going to be smaller than, say, 5px wide or high, then there isn't much point showing the outline. But doing this in another bug is better because then we can discuss there things like: do we need a message that explains why the outline is hidden, or should we show it partially, ...

- Also, right now the outline only shows the first grid fragment. There can be several fragments in cases where a grid is inside a column layout. Do you mind filing another bug for this too? My suggestion for this one would be: if there is more than 1 fragment (which is going to be really rare): then display prev/next navigation links to display the different fragments.

- Finally, because of the preserveAspectRatio attribute, the whole SVG is being resized, including line strokes, and that makes them hard to see. To avoid this, can you please add `vector-effect: non-scaling-stroke;` in the CSS to these 2 rules: `.grid-outline-border` and `.grid-outline-cell`.

::: devtools/client/inspector/grids/components/GridOutline.js:73
(Diff revision 2)
>      return gridArea.name;
>    },
>  
>    /**
> +   * Returns the height of the grid outline ranging between a
> +   * minimum/maximum height of 100/150, respectively.

If we change the constants in the future for some reason, there are good chances that we will forget to change this comment. So better no mention 100 and 150 in this comment.

::: devtools/client/inspector/grids/components/GridOutline.js:80
(Diff revision 2)
> +    if (height >= 150) {
> +      return VIEWPORT_MAX_HEIGHT;
> +    } else if (height <= 100) {
> +      return VIEWPORT_MIN_HEIGHT;
> +    }

You have constants for 150 and 100, so no need to hard-code them again here.

```
if (height > VIEWPORT_MAX_HEIGHT) {
  return VIEWPORT_MAX_HEIGHT;
} else if (height < VIEWPORT_MIN_HEIGHT) {
  return VIEWPORT_MIN_HEIGHT;
}
return height;
```

::: devtools/client/inspector/grids/components/GridOutline.js:234
(Diff revision 2)
>      const color = target.getAttribute("stroke");
>  
>      target.setAttribute("fill", "none");
>  
>      onShowGridAreaHighlight(grids[id].nodeFront, null, color);
> -    onShowGridCellHighlight(grids[id].nodeFront);
> +    onShowGridCellHighlight(grids[id].nodeFront, color, null, null, null);

If the 3 last parameters are null, maybe we don't need to pass them at all:
`onShowGridCellHighlight(grids[id].nodeFront, color);`

::: devtools/client/themes/layout.css:64
(Diff revision 2)
> +.grid-outline {
> +  padding: 10%;
> +}

I removed this to test what the effect was and nothing changed. Why do you need a 10% padding here? Can this be removed? If not, can you please add a comment that explains why it's needed?

Comment 6

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

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

Comment 7

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review123388

Thanks for the feedback! I figured I should explain my fix for the first comment:

The zoom factor isn't updated since the outline stores its height inside component state, so although the rows/cols are updated the height does not adjust itself. I put in a fix where upon receiving new props, if only one grid is selected we reset the height of the outline to adjust to the new dimensions. 

I choose to only let this happen for one selected grid since I imagine when multiple grids are implemented, we want them to scale with the current height already calculated. Of course, then the zoom factor will not adjust appropriately if the rows/cols are edited for a grid. Perhaps this is something that can be opened in another bug, or be handled as part of the multiple selected grids feature ?

> If the 3 last parameters are null, maybe we don't need to pass them at all:
> `onShowGridCellHighlight(grids[id].nodeFront, color);`

For consistency with the function's signature, shouldn't we still pass them even if they are null?

Comment 8

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

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

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review125396

Thanks for the new patch.
This works great. And I have very few comments in the code.
I was going to R+ this, but then I saw many errors in the browser console (ctrl+shift+J). So many in fact that the console was freezing the whole browser.
I'm going to list them below. They're all related to React warning us about things we should fix. So let's try and fix them.

- When opening the panel for the first time:
```
"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `GridList`. See https://fb.me/react-warning-keys for more information.
    in GridItem (created by GridList)
    in GridList (created by Grid)
    in div (created by Grid)
    in div (created by Grid)
    in Grid (created by bound createElement)
    in bound createElement (created by Accordion)
    in div (created by Accordion)
    in div (created by Accordion)
    in div (created by Accordion)
    in Accordion (created by App)
    in div (created by App)
    in App (created by Connect(App))
    in Connect(App)
    in Provider
    in div (created by Tabs)
    in div (created by Tabs)
    in div (created by Tabs)
    in Tabs (created by Tabbar)
    in div (created by Tabbar)
    in Tabbar"
```

And this one too: 
```
"Warning: Unknown prop `object` on <span> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop
    in span (created by ElementNode)
    in span (created by ElementNode)
    in ElementNode (created by Rep)
    in Rep (created by GridItem)
    in label (created by GridItem)
    in li (created by GridItem)
    in GridItem (created by GridList)
    in ul (created by GridList)
    in div (created by GridList)
    in GridList (created by Grid)
    in div (created by Grid)
    in div (created by Grid)
    in Grid (created by bound createElement)
    in bound createElement (created by Accordion)
    in div (created by Accordion)
    in div (created by Accordion)
    in div (created by Accordion)
    in Accordion (created by App)
    in div (created by App)
    in App (created by Connect(App))
    in Connect(App)
    in Provider
    in div (created by Tabs)
    in div (created by Tabs)
    in div (created by Tabs)
    in Tabs (created by Tabbar)
    in div (created by Tabbar)
    in Tabbar"
```

I don't know if these are related to your patch or not, but we should make sure we fix them.

- I also see errors when I click on a checkbox to highlight a grid in the layout panel:
```
Warning: setState(...): Cannot update during an existing state transition (such as within `render` or another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to `componentWillMount`.
```
Looking at the stack trace, it seems to be coming from this location: updateBoxModel/lastRequest< resource://devtools/client/inspector/boxmodel/box-model.js:172:7

- Finally, I see a lot of errors when I start hovering over cells in the outline. The stack trace tells me these come from: onHighlighterChange resource://devtools/client/inspector/grids/grid-inspector.js:301:5

Let's try and clean these up please.

::: devtools/client/inspector/grids/components/GridOutline.js:116
(Diff revision 4)
> +    let borderHeight = 0;
> +    let borderWidth = 0;

I think these would make more sense if they were named `totalHeight` and `totaleWidth` instead.
`border` suggests that this will be the size of the border around the outline, however it's the size of the rectangle that acts as the border.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Micah Tigley [:micah] from comment #7)
> The zoom factor isn't updated since the outline stores its height inside
> component state, so although the rows/cols are updated the height does not
> adjust itself. I put in a fix where upon receiving new props, if only one
> grid is selected we reset the height of the outline to adjust to the new
> dimensions. 
Thanks for the fix, works much better now!

> I choose to only let this happen for one selected grid since I imagine when
> multiple grids are implemented, we want them to scale with the current
> height already calculated. Of course, then the zoom factor will not adjust
> appropriately if the rows/cols are edited for a grid. Perhaps this is
> something that can be opened in another bug, or be handled as part of the
> multiple selected grids feature ?
Agreed, let's worry about this when we do implement support for multiple grid outlines.

> > If the 3 last parameters are null, maybe we don't need to pass them at all:
> > `onShowGridCellHighlight(grids[id].nodeFront, color);`
> 
> For consistency with the function's signature, shouldn't we still pass them
> even if they are null?
I understand the point, however JavaScript doesn't really care about this, it will pass undefined to the function for these arguments, and since the function doesn't care if arguments are passed or not (null is the same as undefined as far as it's concerned), it will work.
We don't usually pass null for unneeded arguments in other parts of devtools, so I'd prefer if we left them out, as you did in your last patch.

Comment 11

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

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

Comment 12

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review125396

We will address these issues in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1350046

Comment 13

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117992/diff/5-6/

Comment 14

[Micah Tigley [:micah]]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117992/diff/6-7/

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8844629 [details]
Bug 1343447 - Rebase maintain aspect ratio patch.

https://reviewboard.mozilla.org/r/117992/#review126728

Thank you for filing bug 1350046, let's just make sure we fix it before shipping the new layout panel however (I see it already blocks the right bug, so we're good).
I have only made a couple of minor comment. Please address them and land this commit.
Do we also have a bug for adding a test for this new feature? We do need tests to cover all new features, so we can avoid regressions in the future. It's especially important here to cover cases like the ones mentioned at the beginning of comment 5.

::: commit-message-e91de:1
(Diff revision 7)
> +Bug 1343447 - Rebase maintain aspect ratio patch. r?pbro

Your commit message should be a short summary of what the commit actuall does. Here it says what you did last on the commit, which was rebasing it.
Instead, it should say something like:
`Bug 1343447 - Make the grid outline have the same aspect ratio as the grid itself; r=pbro`

::: devtools/client/inspector/grids/components/GridOutline.js:132
(Diff revision 7)
> -  /**
> +    /**
> -   * Renders the grid outline for the given grid container object.
> +      * Renders the grid outline for the given grid container object.
> -   *
> +      *
> -   * @param  {Object} grid
> +      * @param  {Object} grid
> -   *         A single grid container in the document.
> +      *         A single grid container in the document.
> -   */
> +      */

nit: this comment did not need to be re-indented, I think it's indentation was fine before.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8985e3bb7d
Bug 1343447 - Maintain aspect ratio of grid outline. r=pbro

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce8985e3bb7d