1297100 - Display CSS Grid layouts when highlighting elements in the page

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

Elements in the page often get highlighted in devtools:

- when you click on the element picker icon in the toolbox and then hover over elements in the page,
- or you move your mouse over elements in the inspector tree,
- or move your mouse over elements in the animation inspector, or console,
- or click on the inspector icon in the rule-view next to selectors,
- ...

All of these end up toggling the box-model highlighter on the hovered/selected elements, which is useful because you see how big elements are, how big their margins are, etc.

Now that we are working on a new css grid tool, it would be useful to integrate the css grid highlighter with the box-model highlighter, so that as you hover over elements that define (or are in) grids, you'd also see the grid itself.

There are several possible options, but first some vocabulary:
- grid container = an element that has display:grid
- grid item = an element that is a direct child of a grid container

So the options I see would be:
- highlight the grid when hovering grid containers only,
- or highlight the grid when hovering grid containers and grid items,
- or highlight the grid when hovering grid containers and any of their descendants.

We should also choose whether the box-model regions (margin, padding, border) are also highlighted when the grid is highlighted.

Comment 1

[Patrick Brosset <:pbro>]

Option 2 in the above list would look something like this:
https://dl.dropboxusercontent.com/u/714210/css-grid/highlighter/container-and-items-only.gif

As the mouse enters the grid container, the lines are displayed, and when the mouse moves over grid items, the lines stay visible, but the grid items also get highlighted at the same time.

Now this was a very simple example where there was only 2 levels of elements: the container and the items.

Here's how it would behave with more elements:
https://dl.dropboxusercontent.com/u/714210/css-grid/highlighter/container-and-items-only-2.gif

Same as before, when the mouse is over the grid container or over a grid item, then the grid is displayed (you can see the lines). But as the mouse moves to deeper elements (the text paragraphs for instance), only they get highlighted, the grid disappears.

I think this is the first thing this bug should resolve. Should we highlight the grid more permanently, or is highlighting it only on containers and items enough?

Unfortunately, I think the answer might depend on the use case:
- if you use a grid only as your global page layout (to position the header, sidebar, content, footer), then showing the grid lines all the time would be confusing if all you're trying to do is select a paragraph or a link in the the main content area,

- however, if the grid is only used to align a few close-by elements, like labels and inputs in a form for instance, you might want to see it as long as your mouse is over any element in the form, even if those elements aren't grid items or containers.

Comment 2

[Patrick Brosset <:pbro>]

And the second question this bug should answer is how should the tool highlight areas, cells, lines and gaps.

The current devtools highlighter only cares about DOM elements right now. So only nodes that exist and are visible.
The grid highlighter however also cares about things that don't exist in the DOM: grid lines, areas, gaps, cells. These things are only defined in CSS, and can be used by grid items to position themselves at specific positions, but the grid really doesn't exist, it just serves as a set of coordinates that grid items can refer to to occupy a particular place in the page.

Nevertheless, it'd be useful to have these things become highlighted as you move your mouse. Like, moving over a line could display its names and numbers. Moving over an area could display the area itself and give its name, etc.

But, because we use hovering for finding DOM elements in the page we have a conflict. If the mouse moves over an area that contains a DOM element, should we highlight the DOM element, or the area, or both?
Or, if the mouse moves over a line over which a DOM element spans, should the DOM element be highlight, or the line, or both?

Comment 3

[Patrick Brosset <:pbro>]

Helen, Gabriel, Jen: what do you think about the above?
comment 0 and comment 1 are about displaying the grid when selecting elements.
comment 2 is about potentially being able to highlight grid lines, areas, gaps, cells.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d9d337c97a contains links to the prototype used to take the screenshots in comment 1.

Comment 4

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

Clearing needinfo since we already chatted about this today. I like the option of highlighting the grid container and items. Possibly also displaying the grid area/name info as well.

Comment 5

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset <:pbro> from comment #1)
> Same as before, when the mouse is over the grid container or over a grid
> item, then the grid is displayed (you can see the lines). But as the mouse
> moves to deeper elements (the text paragraphs for instance), only they get
> highlighted, the grid disappears.
> 
> I think this is the first thing this bug should resolve. Should we highlight
> the grid more permanently, or is highlighting it only on containers and
> items enough?
> 
> Unfortunately, I think the answer might depend on the use case:
We'll still need to choose a default. I think we might want to display the grid more permanently; otherwise, it will be challenging to highlight lines and grid cells to inspect them. I understand the argument that it's more noise, but I suspect that if you have a grid, and are using grid, you're more concerned with grid information than traditional box-model issues, as a rule. (Not always true, and not saying that we shouldn't display box-model information, just that I think that box-model shouldn't trump grid when a grid element is selected).

Jen, I think this discussion might be helpful in the csslayoutclub slack, since the people in there have been building lots of demos and have needed to inspect them. Going to cross-reference it.

Comment 7

[Patrick Brosset <:pbro>]

I had a chat about this with Rachel Andrew and Jen Simmons at ViewSource today and, based on this, here's what we should do with this bug:

- When we highlight an element in the page, if this element is a grid container OR a grid item, then we should also show the grid lines and gaps.
- By highlighting elements in the page I mean:
  - either we're hovering over elements in the page with the picker mode ON
  - or we're hovering over nodes in DOM tree in the inspector
  - or we've locked the highlighter on a rule's selector in the rule-view

We can do all sorts of cool stuff with the grid tool like show line names and numbers or areas, or even cells, but that belongs to a more dedicated panel in the sidebar. 90% of what people need when inspecting elements or making CSS changes to elements in the rule-view is 
a) know that a grid is here
b) see the grid lines because that's only a CSS construct that has no representation in the DOM

Also, we still want our "normal" highlighter to appear, with its different colors and node infobar, so we want the grid highlighter to remain light, and therefore showing the lines and gaps only is probably a good idea.

Comment 8

[Patrick Brosset <:pbro>]

Gabriel, as discussed on IRC, here's a simple/hacky patch I used to demo this:
https://hg.mozilla.org/try/rev/5e5dd73b4bdba26ed987cccf5e12fea6718345c4

1 question we should answer: do we want to keep on showing guides around the box-model highlighter. They get confusing when a grid is also being highlighted. But I wouldn't want to just remove them for good as this patch suggests. They can be useful in other situations, and in fact, with our new box-model region colors, they are super important right now (see bug 1301676).

And 1 other thing I think we should do differently than this patch: I think we should instantiate the grid highlighter on the front-end (maybe in toolbox-highlighter-utils.js, or maybe just in inspector-panel.js with getHighlighterByType), instead of having box-model.js do it on the server. 
If we do it on the front-end, then we can pass the actorID of the grid highlighter to the box-model highlighter, and that allows us to make sure we're only ever using just one instance.

Say you've shown the grid from the rule-view (as bug 1302536 suggests), and then you move your mouse over elements in the markup-view. So, instead of using 2 grid highlighters to do this (and potentially highlighting twice the same node), if you just had this one instance of the grid highlighter actor on the front-end, you'd just be able to share it.
One way this could work is when we call is when we call
yield toolbox.highlighter.pick();
in toolbox-highlighter-utils.js for instance, then we could add an optional param to this:
yield toolbox.highlighter.pick(actorIDOfTheGridHighlighter);
which the box-model highlighter would then use on the server to also highlight the grid when needed.

Anyway, this is more of an idea at this stage, I encourage you to try the code in this patch and experiment with different ways of doing this.

Comment 9

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

Attachment: 1297100.patch [1.0]

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8793459 [details] [diff] [review]
1297100.patch [1.0]

Review of attachment 8793459 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. A few general comments first:

- The box-model nodeinfobar appears below grid lines, making it sometimes harder to see. It should be on top of everything else.

- Sometimes the box-model highlighter won't highlight certain elements. I was able to reproduce this on this page:
http://codepen.io/rachelandrew/full/QKwvxJ/
Moving my mouse over elements inside .home-hero in the inspector doesn't highlight them in the page, and the following exception are thrown:
  TypeError: this.currentNode is null
  Stack trace:
  BoxModelHighlighter.prototype<._show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/box-model.js:302:1
  AutoRefreshHighlighter.prototype.show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters/auto-refresh.js:79:17
  exports.HighlighterActor<.showBoxModel@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/highlighters.js:185:19
In fact, after looking at this more, I think it only happens if I have navigated first. If I restart my browser and try again on this page, then it works, but if I came to that page after having navigated a few times (or refreshed the same page), then it fails. Maybe this is a DeadWrapper problem? Maybe we're keeping a reference to a dead node.

- This will need to be rebased.

- As I mentioned in an earlier bug, we need to start thinking about tests. I haven't been mentioning tests in my reviews so far because the feature was not available through the UI. With this patch it now is, so we must make sure we don't regress it. At the very least here you should be adding a test that checks that the grid highlighter is shown along with the box-model highlighter when needed, and hidden when needed.

::: devtools/server/actors/highlighters/box-model.js
@@ +283,5 @@
>    _show: function () {
>      if (BOX_MODEL_REGIONS.indexOf(this.options.region) == -1) {
>        this.options.region = "content";
>      }
> +    // Show the CSS grid highlighter if the current node is a grid container

Please insert a newline to separate these 2 blocks.
Also, our line length limit is now 90 chars, and this comment can fit on 90 chars, no need to wrap it.

@@ +371,5 @@
>      setIgnoreLayoutChanges(true);
>      this._untrackMutations();
>      this._hideBoxModel();
>      this._hideInfobar();
> +    this.cssGridHighlighter.hide();

You should wrap this in `if (this.cssGridHighlighter) {...}` since the property doesn't always exist.

::: devtools/server/actors/highlighters/css-grid.js
@@ +46,5 @@
>   * Cached used by `CssGridHighlighter.getGridGapPattern`.
>   */
> +const gCachedGridPattern = new WeakMap();
> +// WeakMap key for the Row grid pattern.
> +const ROW = {};

Maybe rename ROW_KEY to make it harder to confuse with ROWS (same for COLUMN).

Comment 11

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

Attachment: 1297100.patch [2.0]

Comment 12

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

Attachment: 1282726-2.patch

Comment 13

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

Need some help with the z-index and DeadWrapper problem

Comment 14

[Patrick Brosset <:pbro>]

For the z-index problem, you could simply do this (this way the grid is always inserted before the box-model and therefore appears below):

--- a/devtools/server/actors/highlighters/box-model.js
+++ b/devtools/server/actors/highlighters/box-model.js
@@ -91,28 +91,28 @@ const GRID_ENABLED_PREF = "layout.css.gr
  *       <div class="box-model-infobar-arrow box-model-infobar-arrow-bottom"/>
  *     </div>
  *   </div>
  * </div>
  */
 function BoxModelHighlighter(highlighterEnv) {
   AutoRefreshHighlighter.call(this, highlighterEnv);

+  if (Services.prefs.getBoolPref(GRID_ENABLED_PREF)) {
+    this.cssGridHighlighter = new CssGridHighlighter(this.highlighterEnv);
+  }
+
   this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv,
     this._buildMarkup.bind(this));

   /**
    * Optionally customize each region's fill color by adding an entry to the
    * regionFill property: `highlighter.regionFill.margin = "red";
    */
   this.regionFill = {};
-
-  if (Services.prefs.getBoolPref(GRID_ENABLED_PREF)) {
-    this.cssGridHighlighter = new CssGridHighlighter(this.highlighterEnv);
-  }
 }

Comment 15

[Patrick Brosset <:pbro>]

And I found the cause for the DeadWrapper problem: the cached canvas pattern can't be reused after a page navigation, because it was created in the previous page.
This should fix it:

--- a/devtools/server/actors/highlighters/css-grid.js
+++ b/devtools/server/actors/highlighters/css-grid.js
@@ -93,16 +93,19 @@ const COLUMN = {};
  *   </div>
  * </div>
  */
 function CssGridHighlighter(highlighterEnv) {
   AutoRefreshHighlighter.call(this, highlighterEnv);

   this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv,
     this._buildMarkup.bind(this));
+
+  this.onNavigate = this.onNavigate.bind(this);
+  this.highlighterEnv.on("navigate", this.onNavigate);
 }

 CssGridHighlighter.prototype = extend(AutoRefreshHighlighter.prototype, {
   typeName: "CssGridHighlighter",

   ID_CLASS_PREFIX: "css-grid-",

   _buildMarkup() {
@@ -203,18 +206,19 @@ CssGridHighlighter.prototype = extend(Au
       },
       prefix: this.ID_CLASS_PREFIX
     });

     return container;
   },

   destroy() {
+    this.highlighterEnv.off("navigate", this.onNavigate);
+    this.markup.destroy();
     AutoRefreshHighlighter.prototype.destroy.call(this);
-    this.markup.destroy();
   },

   getElement(id) {
     return this.markup.getElement(this.ID_CLASS_PREFIX + id);
   },

   get ctx() {
     return this.canvas.getCanvasContext("2d");
@@ -258,16 +262,25 @@ CssGridHighlighter.prototype = extend(Au
     ctx.strokeStyle = GRID_GAP_PATTERN_STROKE_STYLE;
     ctx.stroke();

     let pattern = ctx.createPattern(canvas, "repeat");
     gCachedGridPattern.set(dimension, pattern);
     return pattern;
   },

+  /**
+   * Called when the page navigates. Used to clear the cached gap patterns and avoid
+   * using DeadWrapper objects as gap patterns the next time.
+   */
+  onNavigate() {
+    gCachedGridPattern.delete(COLUMN);
+    gCachedGridPattern.delete(ROW);
+  },
+

Comment 16

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

Attachment: Part 1: Display CSS Grid layouts when highlighting elements in the page [1.0]

Unit tests will come in part 2

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8800113 [details] [diff] [review]
Part 1: Display CSS Grid layouts when highlighting elements in the page [1.0]

Review of attachment 8800113 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good to me now. R+ on the code changes.
Please add "leave-open" if you intend to push this before having the test patch ready.
Note that I haven't manually tested this change. I did so last time, and it looked good apart from the couple of problems I mentioned at the start of comment 10. And since those issues appear to be fixed in the code, I'm assuming the feature works well now. But I suggest doing intensive testing on a variety of grid example pages, just to make sure there are no edge cases we missed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/183b9d08d264
Part 1: Display CSS Grid layouts when highlighting elements in the page r=pbro

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/183b9d08d264

Comment 20

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

https://hg.mozilla.org/integration/fx-team/rev/75c306c21c0f719b590d4ebd63e1ca7ad8981adc
Backed out changeset 183b9d08d264 (Bug 1297100) reverting feature r=gl

Comment 21

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

We are reverting this feature since our feedback with Rachel Andrews and Jen Simmons did not find that displaying the grid highlighter along with the box highlighter was useful compare to implementing Bug 1302536 which would allow for the grid highlighter to locked onto the grid container.

Comment 22

[Sebastian Zartner [:sebo]]

Thanks for the hint and removing the ddn keyword, Gabriel! You just missed to set it on the other bug. :-)

Sebastian

Comment 23

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

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #21)
> We are reverting this feature since our feedback with Rachel Andrews and Jen
> Simmons did not find that displaying the grid highlighter along with the box
> highlighter was useful compare to implementing Bug 1302536 which would allow
> for the grid highlighter to locked onto the grid container.

Seems to me then that this bug would be closed as WONTFIX or INVALID. Gabriel, could you confirm that?

Comment 24

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

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #23)
> (In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from
> comment #21)
> > We are reverting this feature since our feedback with Rachel Andrews and Jen
> > Simmons did not find that displaying the grid highlighter along with the box
> > highlighter was useful compare to implementing Bug 1302536 which would allow
> > for the grid highlighter to locked onto the grid container.
> 
> Seems to me then that this bug would be closed as WONTFIX or INVALID.
> Gabriel, could you confirm that?

Despite removing it back then, it was basically because we thought it was a poorly timed feature compare to having the toggles to keep the grid highlighter on for a grid container. It is still a valid functionality that we should potentially add.

Comment 25

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

Attachment: Part 1: Display CSS Grid layouts when highlighting elements in the page [2.0]

Comment 26

[Jean-Yves Perrier [:teoli]]

So I'm re-adding dev-doc-needed (it will appear on our doc tracker only once this lands or is otherwise RESOLVED) [This is a way for the writing team to flag new features long before they land and to be automatically notified when they are ready to be documented]

Comment 27

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

Removing this from MVP of the Grid Inspector in the layout panel

Comment 28

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

Attachment: Bug 1297100 - Display CSS Grid layouts when highlighting elements in the page

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

Comment 29

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

Comment on attachment 8994094 [details]
Bug 1297100 - Display CSS Grid layouts when highlighting elements in the page

Carrying over the r+ since this is the same patch just rebased

Comment 30

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

Patrick, we discussed this last week about reviewing this work since it fits in with what we wanted to do with the flexbox highlighter as well.

I believed you suggested to remove the "content" region fill. In the current patch, we also hide the guide lines since they would otherwise interfere with the grid lines. Just wanted to get any additional feedback on what we should do to the box model highlighter before making this landable.

Comment 31

[Martin Balfanz [:mbalfanz]]

Attachment: Screen Shot 2018-07-23 at 10.34.03.png

When we remove both guides and the content highlight we might loose a lot of information and end up in cases where we show nothing but the grid/flex highlighter.  See the attached screenshot as an example from https://gridbyexample.com/examples/example1/ : I'm highlighting the grid container, while the grid itself takes only a portion of the elements size.

My suggestions would be to:
1. Make sure the grid/flex highlighter are at the very top.  Right now they are covered by the content highlighter
2. Keep the guides, but if possible show them only outside of the element when combined with a grid/flex highlighter
3. Make the content highlighter more transparent.

Comment 32

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

Comment on attachment 8994094 [details]
Bug 1297100 - Display CSS Grid layouts when highlighting elements in the page

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

Comment 33

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab5fad4f235f13bb15f660464752804477afbdf

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8994094 [details]
Bug 1297100 - Display CSS Grid layouts when highlighting elements in the page

https://reviewboard.mozilla.org/r/258710/#review266278

I have a general comment about this first: if the grid is already displayed for a given node (maybe you enabled it from the sidebar), then this patch will add one more grid overlay on top.
Now, the lines will match up, so it won't be too visible. On my PC it just ends up looking a bit more bold, a bit more contrasty. Which isn't bad in itself. It's just different.

::: devtools/server/actors/highlighters/box-model.js:388
(Diff revision 2)
> +    } else if (this.currentNode.parentNode &&
> +               this.currentNode.parentNode.getGridFragments &&
> +               this.currentNode.parentNode.getGridFragments().length) {
> +      gridNode = this.currentNode.parentNode;

Unfortunately not entirely correct, because of `display:contents` nodes.
Can we do the same thing we did in the past for flexbox, where we walk back up to find the actual grid container?
(note, we shouldn't just walk up the tree until we find one, but only skip direct `display:contents` ancestors).

::: devtools/server/actors/highlighters/css-grid.js:144
(Diff revision 2)
> + * - showBoldGridBorders(isShown)
> + *     Shows a bold grid border for the first and last lines of the grid. This option
> + *     is used in the box-model highlighter to show a bold grid border when overlapping
> + *     with the box-model highlight.

I have a few concerns here:

- The grid may be smaller than its container, which means that we'd be drawing bold lines even though they don't overlap the box-model guides.
Here is an example data:text/html,<div style="width:500px;height:300px;margin:1em;display:grid;grid-template-rows:100px 100px;grid-template-columns:100px 100px;">test</div>

- When I tested it, the bottom and right lines did not exactly overlap with the guides. So you could still see both lines, with a small offset. It would be great to try and fix this so they properly align, and the grid lines do hide the box-model guides.

- Also, if these lines are implicit, then they are dashed, and you can therefore still see the box-model guides below. I think it might be confusing.

So, overall, I think the right thing to do, instead of making the grid lines bold, is to hide the portion of the box-model guides that are inside the element (only show the portions that extend out to the edges of the viewport).
*But* only if the grid is as tall/wide as the element itself I guess. Otherwise it would look weird.

Comment 35

[Patrick Brosset <:pbro>]

Attachment: overlapping-lines.png

In this screenshot you can see what I was talking about.
1. the lines don't overlap exactly, so you can still see the 2 sets of lines
2. the grid lines may be dashed/dotted, which means you can still see the box-model guides beneath, and that may be confusing.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:rcaliman, maybe it's time to close this bug?