1317102 - Display multiple grid containers in the CSS Grid Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

We should be able to display multiple grid containers using a single CSS grid highlighter. I found that creating multiple grid highlighters to really slow down scrolling and we should probably not create multiple grid highlighters as the proper solution for displaying multiple grid overlays. This bug is to add support for displaying multiple grid containers in our current css grid highlighter implementation.

Comment 1

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

@pbro, needinfoing to make sure this decision is sound.

Comment 2

[Patrick Brosset <:pbro>]

As discussed on IRC: I believe solving the APZ scrolling issue might make this performance problem go away (bug 1312103). So we should work on this first and re-evaluate.
If we must make the grid highlighter able to draw more than 1 grid at once, it might make the API a lot harder to use.
Right now, all of our highlighters implement a show(domEl, options) method that is used to highlight a single element.
Of course, we have an options object, so we could simply make domEl be some sort of rootNode, and then use the options object instead to pass information about which grid container to highlight and which options it should use.

So, it's possible to go down this route, but I'd prefer it if we fixed the APZ scrolling problem first and checked again if the performance problem still exists after.

Comment 3

[Jen Simmons [:jensimmons]]

This isn't a priority yet (because so much of our attention is on FF57), but once we have resources again, I'd like to bump this back up the top of the list. Everything about the Grid Inspector makes the ability to see more than one Grid at once just make sense. 

Perhaps we should cap it at 3 Grids (or even 2 if necessary as a first step for performance reasons). Being able to see three Grids at once would be amazing, and would likely cover 80% of people's needs. We don't need an unlimited number. I can't image anything more than 10 being at all useful. (Statement subject to further research ;)

Comment 5

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

Attachment: Bug 1317102 - Display multiple grid containers in the CSS Grid Inspector.

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

Comment 6

[Patrick Brosset <:pbro>]

Thanks Gabe. I gave this a quick test on 2 machines: one really powerful desktop PC, and one very slow laptop Mac.

I tested on meyerweb.com which uses grids for each header and each <pre> code area.
I also tested on stripe.com/connect

Up to 3 grids it works pretty well!
There is some noticeable jank when scrolling fast, and sometimes you can see that the canvas takes a while to re-render (missing lines for some time after I scrolled really fast).
But as long as the number of grids stays low, it seems acceptable to me, at least on my local tests.

With *many* grids visible at the same time (say more than 10), the browser really struggles. Scrolling is very janky, and lines do take a long time to appear after you've scrolled.

We need more people to test, on a variety of hardware, but given the right UX, I'm confident we can land this. It's very useful being able to see more than 1 grid at a time.

In terms of UX, here are the things worth considering:
- the grid outline doesn't make sense anymore, perhaps it should be hidden, perhaps there should be 1 per grid
- grid numbers can overlap, and right now, every grid overlay is completely independent, so offsetting numbers in a way that they wouldn't overlap anymore isn't currently possible. Simply because these overlays don't even know other overlays are being drawn too. And we already struggle to show numbers for 1 grid, so finding a spot where *all* numbers from all grids are visible at the same time is going to be hard.
- should the settings be per grid? The ability to show line numbers, extend lines infinitely, etc.
- should we cap the number of grids at somewhere around 3? If so, how do we explain users that they can't select more? (maybe gray out/disable the other checkboxes and add a tooltip message that says "please unselect another grid first")
- perhaps we need a "unselect all" button now.

I think we would want to have answers to these, and perhaps more before landing this.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Patrick Brosset <:pbro> from comment #6)
> With *many* grids visible at the same time (say more than 10), the browser
> really struggles. Scrolling is very janky, and lines do take a long time to
> appear after you've scrolled.

That sounds like a bug to me.  Can you produce a perf log for that?

A problem that I've noticed before is that as soon as an overlay
is displayed nsGridContainerFrame::GetGridFrameWithComputedInfo
is called repeatedly, multiple times per second, for as long as
it is displayed.  This seems like something you should try to avoid.

Comment 8

[Mats Palmgren (inactive)]

(FYI, GetGridFrameWithComputedInfo is called from Element::GetGridFragments)

Comment 9

[Patrick Brosset <:pbro>]

That's because the CSS Grid Highlighter right now just runs on a requestAnimationFrame loop and at every step, just calls getGridFragments again to check whether anything has changed.
If nothing has changed, the grid is not re-displayed. If anything did change, we redraw the highlighter.

We do this because we lack other events that would tell us when a grid changed.

Comment 10

[Jen Simmons [:jensimmons]]

Would everything get faster if we were to remove the grid outline altogether? (And by "grid outline" I assume you mean the mini diagram of the grid that provides a way to reveal tooltips with sizing in pixels.)

Comment 11

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

(In reply to Jen Simmons [:jensimmons] from comment #10)
> Would everything get faster if we were to remove the grid outline
> altogether? (And by "grid outline" I assume you mean the mini diagram of the
> grid that provides a way to reveal tooltips with sizing in pixels.)

No, we are really looking at the scrolling of the page which is not affected by the grid outline.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0042f0b3349ecace6151849710eed59fb8af6db4

Comment 13

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

Attachment: Bug 1317102 - Display multiple grid containers in the CSS Grid Inspector.

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

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a05120e1b4eacfb0efd09f28f39c64394623f9

Comment 15

[Patrick Brosset <:pbro>]

I am currently testing your latest patch locally, and I have a few non-code comments to share first:

1. When the max number of grids has been reached, I believe we should gray out all other checkboxes until one of the checked ones has been unchecked. Otherwise you're left with a list of perfectly good looking boxes that just don't do anything when you click them. This will be confusing. Disabling the other ones doesn't explain much to users though, so maybe on top of this, hovering over a disabled checkbox should display a tooltip like "please unselect a grid first".
(in fact, if we do this, we really should also do it in the rule-view (the waffle icon), and in the markup-view (the badges)).

2. When >=2 grids are selected, I believe we should just hide the outline. It's just confusing at this stage.

3. I'm fairly sure we'll need to move our settings to be per grid, but we can think about this in a separate bug, since the default value for the max pref is 1. That gives us some time.

4. Here's one problem I don't know how to solve: grids always get painted in the order you select them. So if you select grid1 first and then grid2, then the lines for grid2 will be painted last, and if they overlap the lines from grid1, they will be hiding them. Now if you unselect grid1 and re-select again, then its lines will be hiding those from grid2.
So, basically, the problem is when there are overlapping lines (which there will be, because the whole point of selecting multiple grids is for when those grids are similar/aligned), then it isn't possible to see the overlapping lines at the same time, and you may be seeing one grid, or the other depending on the order you selected them.

Now, setting the pref to 1 in this patch is good, because it allows us to preserve today's behavior, and therefore fix those incrementally and only "ship" when we're satisfied with the overall experience.
Having said this, I would fix the following in this patch before landing:
1 (only the part about the checkboxes for now)
2
and I think 3 and 4 can be follow-up material.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8988369 [details]
Bug 1317102 - Display multiple grid containers in the CSS Grid Inspector.

https://reviewboard.mozilla.org/r/253638/#review261228

::: devtools/client/inspector/grids/grid-inspector.js:116
(Diff revision 1)
>      this.document.addEventListener("mousemove", () => {
>        this.highlighters.on("grid-highlighter-hidden", this.onHighlighterHidden);
>        this.highlighters.on("grid-highlighter-shown", this.onHighlighterShown);
>      }, { once: true });
>  
> +    if (flags.testing) {
> +      this.highlighters.on("grid-highlighter-hidden", this.onHighlighterHidden);
> +      this.highlighters.on("grid-highlighter-shown", this.onHighlighterShown);
> +    }

Although unlikely, mousemove can happen from a test (I've seen this in the past where a previous test moved the mouse somewhere, and the next one which simulated a click somewhere else, therefore also simulated a move first).
Anyway, nothing bad would happen then, but I just think it would make more sense if these 2 blocks were wrapped in an if/else with a nice comment like so:

```
if (!flags.testing) {
  // Only start listening if a mousemove is detected (lazy).
  this.document.addEventListener("mousemove", ...
} else {
  // In tests, we start listening immediately to avoid having to simulate a mousemove.
  this.highlighters.on...
}
```

::: devtools/client/inspector/grids/grid-inspector.js:233
(Diff revision 1)
>      return this.swatchColorPickerTooltip;
>    }
>  
>    /**
> -   * Given a list of new grid fronts, and if we have a currently highlighted grid, check
> +   * Given a list of new grid fronts, and if there are highlighted grids, check
>     * if its fragments have changed.

typo: if their fragments have changed

::: devtools/client/inspector/grids/grid-inspector.js:453
(Diff revision 1)
>      if (grids.length && grids.some(grid => !grid.nodeFront.actorID)) {
>        this.updateGridPanel(newGridFronts);
>        return;
>      }
>  
> -    // Otherwise, continue comparing with the new grids.
> +    // Get the node front(s) from the current grid(s) so we can compare them to them to

typo: compare them to the node(s)...

::: devtools/client/inspector/grids/grid-inspector.js
(Diff revision 1)
> -    const { grids } = this.store.getState();
> -    const grid = grids.find(g => g.nodeFront === node);
> -    this.store.dispatch(updateGridHighlighted(node, !grid.highlighted));

Can you please explain to me why this isn't needed anymore.

::: devtools/client/inspector/grids/reducers/grids.js:35
(Diff revision 1)
>      });
>  
>      return newGrids;
>    },
>  
>    [UPDATE_GRID_HIGHLIGHTED](grids, { nodeFront, highlighted }) {

It would make more sense to me if this function also delt with MAX_GRID_HIGHLIGHTERS values >= 2. That is, it should mark all grid as selected/unselected depending on MAX and which grids are already selected/unselected.
The reducer is where the data model logic happens, so it would make sense to me if it also handled that.

I'm guessing we could also introduce a "disabled" property for each grid, so that we can use it to mark checkboxes as disabled when we've reached MAX.

::: devtools/client/inspector/grids/reducers/grids.js:37
(Diff revision 1)
>      return newGrids;
>    },
>  
>    [UPDATE_GRID_HIGHLIGHTED](grids, { nodeFront, highlighted }) {
>      return grids.map(g => {
> +      if (MAX_GRID_HIGHLIGHTERS === 1) {

Could you please add a comment here to explain the difference of behavior for when MAX is 1 vs. when it's more.

::: devtools/client/inspector/grids/reducers/grids.js:46
(Diff revision 1)
> +        g = Object.assign({}, g, {
> +          highlighted: !g.highlighted,
> +        });

`highlighted` is passed as an argument to the reducer, shouldn't we use this value instead of just flipping the current value?

::: devtools/client/inspector/shared/highlighters-overlay.js:640
(Diff revision 1)
> +   * Get a grid highlighter front for a given node. It will initialize a new grid
> +   * highlighter for every unique node.

We could totally reuse highlighters we've created before, they can be shown on any node. Right now we finalize them on hide, and we instantiate new ones every time.

Reusing would avoid 1 round-trip to the server to create the highlighter, and probably be better for memory too.

For this, we'd need a highlighter pool from which we can pick an unused highlighter when needed.
When getting a highlighter, we would first check in the pool, if there's one we use it, and remove it from the pool. Otherwise we instantiate a new one.
And then on hide, we return the highlighter to the pool.

::: devtools/client/inspector/shared/highlighters-overlay.js:658
(Diff revision 1)
> +    let highlighter;
> +
> +    try {
> +      highlighter = await utils.getHighlighterByType("CssGridHighlighter");
> +    } catch (e) {
> +      // Ignore any error

We silently swallow errors, but then we return null, and that causes the no highlighter to appear for the user. I think it might be better to just let the error bubble up, we do want to know when that happens, right? 
This way, users will have a stacktrace in the browser console, that we can make use of to debug the issue.
Unless you already know what kind of error can occur and you do want to hide it. In which case, please add a comment that explains this.

Comment 17

[Patrick Brosset <:pbro>]

Forgot about one thing, sorry: can we add 1 test for checking that highlighting multiple grids does work :)

Comment 18

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

Attachment: Part 1: Add a pref to enable displaying multiple grid containers in the CSS Grid Inspector [1.0]

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ae1bd057dd9100eee2a79bac97aebff2712245b

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 9007666 [details] [diff] [review]
Part 1: Add a pref to enable displaying  multiple grid containers in the CSS Grid Inspector [1.0]

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

Patch looks very good, just the try failure linked to hiding grid highlighters on will-navigate.

::: devtools/client/inspector/grids/grid-inspector.js
@@ +446,1 @@
>        // Same list of containers, but let's check if the geometry of the current grid has

This comment should say "grids" and it is a bit out of place: "let's check" right before the `return`. Reformulate?

"Same list of containers, and the geometry of all the displayed grids remained the same, we can safely abort."

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +597,5 @@
>     * Restores the saved grid highlighter state.
>     */
>    async restoreGridState() {
> +    // The NodeFronts that are used as the keys in the grid state Map are no longer in the
> +    // tree after a reload. To clean up the grid state, we create an copy of the values of

nit: an copy -> a copy

@@ +606,2 @@
>      try {
> +      for (const state of values) {

Could we rename state to gridState here? "values" is very generic, so without hint it is hard to know what this "state" might refer to.

@@ +1053,5 @@
>  
> +    // Hide and store all the grid highlighters into the pool of reusable grid
> +    // highlighters, and clear the Map of grid highlighters.
> +    for (const highlighter of this.gridHighlighters.values()) {
> +      await highlighter.hide();

We don't go through hideGridHighlighter here, so we don't emit "grid-highlighter-hidden", which makes the reload test timeout on try.

Should we really hide the grid highlighters here, since we don't do that for any of the other highlighters? They seem to normally be hidden via the onMarkupMutation > _hideHighlighterIfDeadNode > hideGridHighlighter , so I don't know what motivates the extra code here?

Comment 21

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

Comment on attachment 9007666 [details] [diff] [review]
Part 1: Add a pref to enable displaying  multiple grid containers in the CSS Grid Inspector [1.0]

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

::: devtools/client/inspector/grids/grid-inspector.js
@@ +446,1 @@
>        // Same list of containers, but let's check if the geometry of the current grid has

Fixed.

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +606,2 @@
>      try {
> +      for (const state of values) {

Fixed.

@@ +1053,5 @@
>  
> +    // Hide and store all the grid highlighters into the pool of reusable grid
> +    // highlighters, and clear the Map of grid highlighters.
> +    for (const highlighter of this.gridHighlighters.values()) {
> +      await highlighter.hide();

Fixed.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89abb776e1e
Part 1: Add a pref to enable displaying  multiple grid containers in the CSS Grid Inspector. r=jdescottes

Comment 23

[Tiberius Oros[:tiberius_oros]]

Backed out changeset c89abb776e1e (bug 1317102) for failures on devtools/client/inspector/grids/test/browser_grids_grid-list-on-iframe-reloaded.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e2a5e110fd37294924c1b4d0b93636def41f7b

Push with the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c89abb776e1e5a4482e0ab6572aaa5ee32b61c9d&selectedJob=198487650

Failure log file: https://treeherder.mozilla.org/logviewer.html#?job_id=198487650&repo=mozilla-inbound&lineNumber=9122

Comment 24

[Julian Descottes [:jdescottes]]

Only fails intermittently in --verify locally as well. It looks like `await enableTheFirstGrid(doc, inspector);` doesn't work as expected. In particular the screenshot shows that the checkbox is still unchecked?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97ebeafdd14
Part 1: Add a pref to enable displaying  multiple grid containers in the CSS Grid Inspector. r=jdescottes

Comment 26

[Daniel Varga [:dvarga]]

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

Comment 27

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

Attachment: Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. [1.0]

Comment 28

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=354b2aecdec38f60e5ef28653ea874b395995ba2

Comment 29

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

Attachment: Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. [1.0]

Comment 30

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

Attachment: Part 4: Hide grid highlighter toggles in the rules when the max highlighter is reached. [1.0]

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 9008736 [details] [diff] [review]
Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. [1.0]

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

Looks good, and thanks for the detailed commit message (however I don't reproduce the issue on my machine).

::: devtools/client/inspector/markup/views/element-container.js
@@ -65,1 @@
>      MarkupContainer.prototype.destroy.call(this);

We can remove this destroy now that we only call the parent class destroy.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +733,2 @@
>    /**
>     * Called when the display badge is clicked. Toggles on the grid highlighter for the

I realize this JSDoc is no longer up to date, it should mention flex as well (or not mention grid at all).

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 9008862 [details] [diff] [review]
Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. [1.0]

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

Thanks, I would like to see another version with the bug mentioned fixed.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +185,5 @@
>  
>      close.appendChild(this.doc.createTextNode(">"));
>    },
>  
> +  get cssProperties() {

What is the added value of this lazy getter? It seems we always access `this.cssProperties` in the constructor?

@@ +358,5 @@
> +  },
> +
> +  _updateGridDisplayBadgeInteractive: function() {
> +    if (this.maxGridHighlighters === 1) {
> +      this._displayBadge.classList.toggle("interactive");

No sure I understand this one, if this.maxGridHighlighters === 1 then the badge should always be interactive I think. Here we are toggling it every time the method is called.

STRs:
- open https://labs.jensimmons.com/2017/01-003.html
- open inspector (max grid highlighters set to 1)
- show the first .grid-container in markup view (badge is interactive)
- click on grid badge
- uncheck corresponding checkbox in layout panel
- grid badge is no longer interactive

Can probably be merged together with the conditions of the toggle in the else (which will then look very similar to the duplicated code mentioned a bit below).

@@ +766,5 @@
>        // Stop tracking highlighter events to avoid flickering of the active class.
>        this.stopTrackingFlexboxHighlighterEvents();
>  
>        this._displayBadge.classList.toggle("active");
> +      await this.highlighters.toggleFlexboxHighlighter(this.node, "markup");

toggleFlexboxHighlighter takes an option object as second paramater, and doesn't support the "trigger" parameter from toggleGridHighlighter. We should file a follow up to fix that.

@@ +776,5 @@
> +      // Don't toggle the grid highlighter if the max number of new grid highlighters
> +      // allowed has been reached.
> +      if (this.maxGridHighlighters > 1 &&
> +          !this.highlighters.gridHighlighters.has(this.node) &&
> +          this.maxGridHighlighters === this.highlighters.gridHighlighters.size) {

This logic gets duplicated in highlighters-overlay in part4. It feels like this should be a helper method in highlighters-overlay. Or maybe delegate this completely to highlighters-overlay, and update the class depending on whether or not the node is now "grid-highlighted":

  this.stopTrackingGridHighlighterEvents();

  await this.highlighters.toggleGridHighlighter(this.node, "markup");
  const isHighlighted = this.highlighters.gridHighlighters.has(this.node);
  this._displayBadge.classList.toggle("active", isHighlighted);

  this.startTrackingGridHighlighterEvents();

@@ +784,5 @@
>        // Stop tracking highlighter events to avoid flickering of the active class.
>        this.stopTrackingGridHighlighterEvents();
>  
>        this._displayBadge.classList.toggle("active");
> +      await this.highlighters.toggleGridHighlighter(this.node, "markup");

"markup" is not a supported trigger for showGridHighlighter. 

https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/devtools/client/inspector/shared/highlighters-overlay.js#450-454

Should we add it to telemetry in a followup?

@@ +827,4 @@
>      this._displayBadge.classList.toggle("active",
>        this.highlighters.gridHighlighters.has(this.node));
> +
> +    this._updateGridDisplayBadgeInteractive();

maybe we could simply call updateDisplayBadgeContent() ?

Comment 33

[Julian Descottes [:jdescottes]]

Comment on attachment 9008736 [details] [diff] [review]
Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. [1.0]

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

Sorry I initially missed the events issue! Would like to see another version with a fix for that.

Note that ideally it should be safe to simply call updateDisplayBadge as soon as anything might have changed regarding the display property of the node.
In the next patches you have been extracting some parts of this method and we are calling them one by one in different spots.
I think the whole thing will be much more maintainable if we just go through updateDisplayBadge() everytime.

::: devtools/client/inspector/markup/views/element-editor.js
@@ +309,3 @@
>      if (this._displayBadge && !showDisplayBadge) {
>        this._displayBadge.remove();
> +      this.stopTrackingFlexboxHighlighterEvents();

I read this one a bit too fast. 

So we call this everytime update() is called.

- If we should not have a badge and currently have one, we remove the node and stop listening, good! Actually we should also nullify _displayBadge, this is not related to your change, but it's a bug from my initial implementation about lazy badges, sorry about that.
- If we should display a badge, we always startTrackingSomethingHighlighterEvents. What happens if we switch the display form grid to inline-grid to flex, then back to grid etc... We will then add several listeners

I think it would be easier if you:
- always add all events when creating the badge
- always remove them when removing the badge 

It doesn't matter so much that we are listening to both events at the same time I think and it will simplify the logic.

Comment 34

[Julian Descottes [:jdescottes]]

Comment on attachment 9008992 [details] [diff] [review]
Part 4: Hide grid highlighter toggles in the rules when the max highlighter is reached. [1.0]

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

Thanks! 

See comments in part3 about duplicated logic between highlighters-overlay and the editors.
Would be nice to address this.

I have a comment about the UX for this change. Maybe this was already discussed, otherwise, up to you to discuss it/do it/move it to a followup.

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +546,5 @@
> +
> +      if (this.maxGridHighlighters > 1 &&
> +          gridHighlighters.size === this.maxGridHighlighters &&
> +          !gridHighlighters.has(nodeFront)) {
> +        gridToggle.setAttribute("hidden", true);

Should we hide or disable (with title explaining why it is disabled?)

Comment 35

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

Attachment: Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. [2.0]

Comment 36

[Julian Descottes [:jdescottes]]

Comment on attachment 9011338 [details] [diff] [review]
Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. [2.0]

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

Thanks!

Comment 37

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

Comment on attachment 9008862 [details] [diff] [review]
Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. [1.0]

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

::: devtools/client/inspector/markup/views/element-editor.js
@@ +185,5 @@
>  
>      close.appendChild(this.doc.createTextNode(">"));
>    },
>  
> +  get cssProperties() {

Removed this.

@@ +358,5 @@
> +  },
> +
> +  _updateGridDisplayBadgeInteractive: function() {
> +    if (this.maxGridHighlighters === 1) {
> +      this._displayBadge.classList.toggle("interactive");

Fixed.

@@ +766,5 @@
>        // Stop tracking highlighter events to avoid flickering of the active class.
>        this.stopTrackingFlexboxHighlighterEvents();
>  
>        this._displayBadge.classList.toggle("active");
> +      await this.highlighters.toggleFlexboxHighlighter(this.node, "markup");

Followup.

@@ +776,5 @@
> +      // Don't toggle the grid highlighter if the max number of new grid highlighters
> +      // allowed has been reached.
> +      if (this.maxGridHighlighters > 1 &&
> +          !this.highlighters.gridHighlighters.has(this.node) &&
> +          this.maxGridHighlighters === this.highlighters.gridHighlighters.size) {

Ended up using a helper method since I prefer not going through all these function calls if we can avoid it.

@@ +784,5 @@
>        // Stop tracking highlighter events to avoid flickering of the active class.
>        this.stopTrackingGridHighlighterEvents();
>  
>        this._displayBadge.classList.toggle("active");
> +      await this.highlighters.toggleGridHighlighter(this.node, "markup");

To add as a followup.

@@ +827,4 @@
>      this._displayBadge.classList.toggle("active",
>        this.highlighters.gridHighlighters.has(this.node));
> +
> +    this._updateGridDisplayBadgeInteractive();

Done.

Comment 38

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

Attachment: Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. [2.0]

Comment 39

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

Attachment: Part 4: Hide grid highlighter toggles in the rules when the max highlighter is reached. [2.0]

Comment 40

[Julian Descottes [:jdescottes]]

Comment on attachment 9011569 [details] [diff] [review]
Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. [2.0]

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

Thanks, just one small comment!

::: devtools/client/inspector/markup/views/element-editor.js
@@ +346,5 @@
> +      this._displayBadge.classList.toggle("interactive",
> +        Services.prefs.getBoolPref("devtools.inspector.flexboxHighlighter.enabled"));
> +    } else if (displayType === "grid" || displayType === "inline-grid") {
> +      this._displayBadge.classList.toggle("interactive",
> +        this.maxGridHighlighters === 1 ||

Isn't this 
  !this.highlighters.canGridHighlighterToggle(this.node)

? If it is we no longer need this.maxGridHighlighters in this file.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3bd3d1a83f
Part 2: Refactor the highlighter event handlers into ElementEditor and prevent flickering of the display badge when toggled from a click. r=jdescottes

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0994cc1133a5
Part 3: Disable grid highlighter toggles in the markup when the max highlighter is reached. r=jdescottes

Comment 43

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2b3bd3d1a83f
https://hg.mozilla.org/mozilla-central/rev/0994cc1133a5

Comment 44

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

Attachment: Part 6: Hide the grid outline when multiple grids are highlighted. [1.0]

Comment 45

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

Attachment: Part 6: Hide the grid outline when multiple grids are highlighted. [2.0]

Comment 46

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

Attachment: Part 5: Increase the max grid highlighters shown to 3. [1.0]

Comment 47

[Patrick Brosset <:pbro>]

Comment on attachment 9015065 [details] [diff] [review]
Part 6: Hide the grid outline when multiple grids are highlighted. [2.0]

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

Looks good, but please add a simple mochitest that sets the pref to, say, 2, and checks that the outline is not visible.

Comment 48

[Patrick Brosset <:pbro>]

Comment on attachment 9015066 [details] [diff] [review]
Part 5: Increase the max grid highlighters shown to 3. [1.0]

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

Same here, we need a test for this new feature. Or maybe several tests.
Are you intending to write them in another patch? If so, please land it within 64 as well, so the feature goes out with tests from the start.
At least, I'd expect a test that simulates clicking on multiple checkboxes, checking that each time the corresponding grid gets toggled, and then checking that no more checkboxes can be clicked on, etc.

Comment 49

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

Comment on attachment 9015066 [details] [diff] [review]
Part 5: Increase the max grid highlighters shown to 3. [1.0]

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

We actually have multiple unit tests for the rules (not landed yet), markup and grid panel for multiple grid highlighting. They were all landed in the previous parts of this bug or will be.

browser_grids_grid-list-toggle-multiple-grids.js
browser_grids_restored-multiple-grids-after-reload.js
browser_markup_grid_display_badge_02.js

Comment 50

[Patrick Brosset <:pbro>]

Comment on attachment 9015066 [details] [diff] [review]
Part 5: Increase the max grid highlighters shown to 3. [1.0]

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

Same here, we need a test for this new feature. Or maybe several tests.
Are you intending to write them in another patch? If so, please land it within 64 as well, so the feature goes out with tests from the start.
At least, I'd expect a test that simulates clicking on multiple checkboxes, checking that each time the corresponding grid gets toggled, and then checking that no more checkboxes can be clicked on, etc.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/327ce5456680
Part 5: Increase the max grid highlighters shown to 3. r=pbro

Comment 52

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

Attachment: Part 6: Hide the grid outline when multiple grids are highlighted. [3.0]

Comment 53

[Patrick Brosset <:pbro>]

Comment on attachment 9015298 [details] [diff] [review]
Part 6: Hide the grid outline when multiple grids are highlighted. [3.0]

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

Great, thank you. Just one comment about browser.ini that you'll need to address before landing.

::: devtools/client/inspector/grids/test/browser.ini
@@ +31,5 @@
>  [browser_grids_grid-outline-cannot-show-outline.js]
>  [browser_grids_grid-outline-highlight-area.js]
>  skip-if = (verify && (os == 'win'))
>  [browser_grids_grid-outline-highlight-cell.js]
> +[browser_grids_grid-outline-multiple-grids.js]

Looks like you should move this new test down 1 line, since the previous test had a skip-if condition that now does not apply anymore.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e30423fbf14c
Part 6: Hide the grid outline when multiple grids are highlighted. r=pbro

Comment 55

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/327ce5456680
https://hg.mozilla.org/mozilla-central/rev/e30423fbf14c

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0be70aed86
Part 4: Disable grid highlighter toggles in the rules when the max highlighter is reached. r=jdescottes

Comment 57

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4c0be70aed86

Comment 58

[Liz Henry (:lizzard) (relman/hg->git project)]

This is in the 64 release notes as "You may now overlay multiple CSS grids at the same time (up to 3) in the CSS Grid Inspector".