Last Comment Bug 1297100 - Display CSS Grid layouts when highlighting elements in the page
: Display CSS Grid layouts when highlighting elements in the page
Status: ASSIGNED
: dev-doc-needed, leave-open
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: Unspecified Unspecified
P3 enhancement (vote)
: ---
Assigned To: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors:
Depends on:
Blocks: 1181227 devtools/highlighters
  Show dependency treegraph
 
Reported: 2016-08-22 08:18 PDT by Patrick Brosset <:pbro>
Modified: 2017-01-27 02:12 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1297100.patch [1.0] (10.02 KB, patch)
2016-09-21 12:36 PDT, Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
no flags Details | Diff | Splinter Review
1297100.patch [2.0] (10.35 KB, patch)
2016-09-30 11:18 PDT, Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
no flags Details | Diff | Splinter Review
1282726-2.patch (2.40 KB, patch)
2016-09-30 11:21 PDT, Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
no flags Details | Diff | Splinter Review
Part 1: Display CSS Grid layouts when highlighting elements in the page [1.0] (11.45 KB, patch)
2016-10-11 23:03 PDT, Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
pbrosset: review+
Details | Diff | Splinter Review
Part 1: Display CSS Grid layouts when highlighting elements in the page [2.0] (4.95 KB, patch)
2017-01-26 20:26 PST, Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
gl: review+
Details | Diff | Splinter Review

User Story
Current try-build with Patrick's demo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d9d337c97a      
Description User image Patrick Brosset <:pbro> 2016-08-22 08:18:26 PDT
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 User image Patrick Brosset <:pbro> 2016-08-22 08:57:50 PDT
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 User image Patrick Brosset <:pbro> 2016-08-22 09:03:58 PDT
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 User image Patrick Brosset <:pbro> 2016-08-22 09:11:20 PDT
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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-08-23 12:17:55 PDT
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 User image Patrick Brosset <:pbro> 2016-08-26 07:22:48 PDT
Inspector bug triage (filter on CLIMBING SHOES).
Comment 6 User image Helen V. Holmes (:helenvholmes) (:✨) 2016-09-01 07:03:02 PDT
(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 User image Patrick Brosset <:pbro> 2016-09-13 13:08:08 PDT
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 User image Patrick Brosset <:pbro> 2016-09-13 13:44:22 PDT
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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-09-21 12:36:46 PDT
Created attachment 8793459 [details] [diff] [review]
1297100.patch [1.0]
Comment 10 User image Patrick Brosset <:pbro> 2016-09-26 12:02:07 PDT
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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-09-30 11:18:29 PDT
Created attachment 8796641 [details] [diff] [review]
1297100.patch [2.0]
Comment 12 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-09-30 11:21:12 PDT
Created attachment 8796643 [details] [diff] [review]
1282726-2.patch
Comment 13 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-10-05 08:08:28 PDT
Need some help with the z-index and DeadWrapper problem
Comment 14 User image Patrick Brosset <:pbro> 2016-10-10 00:45:33 PDT
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 User image Patrick Brosset <:pbro> 2016-10-10 00:53:29 PDT
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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-10-11 23:03:35 PDT
Created attachment 8800113 [details] [diff] [review]
Part 1: Display CSS Grid layouts when highlighting elements in the page [1.0]

Unit tests will come in part 2
Comment 17 User image Patrick Brosset <:pbro> 2016-10-12 00:09:51 PDT
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 User image Pulsebot 2016-10-13 13:36:08 PDT
Pushed by gabriel.luong@gmail.com:
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 User image Carsten Book [:Tomcat] 2016-10-14 03:04:19 PDT
https://hg.mozilla.org/mozilla-central/rev/183b9d08d264
Comment 20 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-10-20 23:31:34 PDT
https://hg.mozilla.org/integration/fx-team/rev/75c306c21c0f719b590d4ebd63e1ca7ad8981adc
Backed out changeset 183b9d08d264 (Bug 1297100) reverting feature r=gl
Comment 21 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2016-10-20 23:33:48 PDT
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 User image Sebastian Zartner [:sebo] 2016-10-20 23:54:18 PDT
Thanks for the hint and removing the ddn keyword, Gabriel! You just missed to set it on the other bug. :-)

Sebastian
Comment 23 User image Matteo Ferretti [:zer0] [:matteo] 2017-01-24 02:18:55 PST
(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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2017-01-24 08:05:20 PST
(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 User image Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) 2017-01-26 20:26:00 PST
Created attachment 8831025 [details] [diff] [review]
Part 1: Display CSS Grid layouts when highlighting elements in the page [2.0]
Comment 26 User image Jean-Yves Perrier [:teoli] 2017-01-27 02:12:28 PST
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]

Note You need to log in before you can comment on or make changes to this bug.