Display multiple grid containers in the CSS Grid Inspector

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks: 4 bugs, {DevAdvocacy})

unspecified
Firefox 64
DevAdvocacy
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 64+, firefox64 fixed)

Details

(Whiteboard: [DevRel:P1])

Attachments

(7 attachments, 6 obsolete attachments)

59 bytes, text/x-review-board-request
Details
94.26 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
12.07 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
24.09 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
25.85 KB, patch
Details | Diff | Splinter Review
4.23 KB, patch
pbro
: review+
Details | Diff | Splinter Review
13.35 KB, patch
pbro
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
@pbro, needinfoing to make sure this decision is sound.
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
Blocks: 1333358
Depends on: 1312103
Keywords: DevAdvocacy
Whiteboard: [DevRel:P1]
Summary: Display multiple grid containers in the CSS Grid highlighter → Display multiple grid containers in the CSS Grid Inspector
(Assignee)

Updated

2 years ago
Assignee: gl → nobody
Status: ASSIGNED → NEW
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 ;)
(Assignee)

Updated

10 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1458605
Comment hidden (mozreview-request)
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.
(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.
(FYI, GetGridFrameWithComputedInfo is called from Element::GetGridFragments)
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.
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.)
(Assignee)

Comment 11

10 months ago
(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.
(Assignee)

Updated

10 months ago
Attachment #8981173 - Flags: review?(pbrosset)
(Assignee)

Updated

10 months ago
Depends on: 1468002
(Assignee)

Updated

10 months ago
Depends on: 1456680
Blocks: 1464440
Blocks: 1468402

Updated

9 months ago
Product: Firefox → DevTools
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8981173 - Attachment is obsolete: true
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

9 months ago
mozreview-review
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.
Attachment #8988369 - Flags: review?(pbrosset)
Forgot about one thing, sorry: can we add 1 test for checking that highlighting multiple grids does work :)
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?
Attachment #9007666 - Flags: review?(jdescottes) → review+
(Assignee)

Updated

6 months ago
Keywords: leave-open
(Assignee)

Comment 21

6 months ago
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

6 months ago
Pushed by gabriel.luong@gmail.com:
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
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?
(Assignee)

Updated

6 months ago
Flags: needinfo?(gl)

Comment 25

6 months ago
Pushed by gabriel.luong@gmail.com:
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 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).
Attachment #9008736 - Flags: review?(jdescottes) → review+
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() ?
Attachment #9008862 - Flags: review?(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.
Attachment #9008736 - Flags: review+
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?)
Attachment #9008992 - Flags: review?(jdescottes) → review+
Depends on: 1492516
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!
Attachment #9011338 - Flags: review?(jdescottes) → review+
(Assignee)

Comment 37

6 months ago
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.
(Assignee)

Updated

6 months ago
Attachment #9007666 - Flags: checkin+
(Assignee)

Updated

6 months ago
Attachment #9011338 - Flags: checkin+
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.
Attachment #9011569 - Flags: review?(jdescottes) → review+

Comment 41

6 months ago
Pushed by gabriel.luong@gmail.com:
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

6 months ago
Pushed by gabriel.luong@gmail.com:
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
(Assignee)

Updated

6 months ago
Attachment #9015062 - Flags: review?(pbrosset)
(Assignee)

Comment 45

6 months ago
Attachment #9015062 - Attachment is obsolete: true
Attachment #9015065 - Flags: review?(pbrosset)
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.
Attachment #9015065 - Flags: review?(pbrosset)
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.
Attachment #9015066 - Flags: review?(pbrosset)
(Assignee)

Comment 49

6 months ago
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
Attachment #9015066 - Flags: review?(pbrosset)
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.
Attachment #9015066 - Flags: review?(pbrosset) → review+

Comment 51

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/327ce5456680
Part 5: Increase the max grid highlighters shown to 3. r=pbro
(Assignee)

Comment 52

6 months ago
Attachment #9015065 - Attachment is obsolete: true
Attachment #9015298 - Flags: review?(pbrosset)
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.
Attachment #9015298 - Flags: review?(pbrosset) → review+

Comment 54

6 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e30423fbf14c
Part 6: Hide the grid outline when multiple grids are highlighted. r=pbro
(Assignee)

Updated

5 months ago
Keywords: leave-open

Comment 56

5 months ago
Pushed by gabriel.luong@gmail.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c0be70aed86
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
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".
relnote-firefox: --- → 64+
You need to log in before you can comment on or make changes to this bug.