I haven't looked at that patch yet, but based on bug 1297100 comment 7, I think we should treat cells as we treat areas: create an option in the highlighter that, once turned ON, can display a grid cell at a particular position in the grid, along with an infobar that gives information about the coordinates of the cell. We're most likely going to toggle cells from the new Layout panel only. So this needs to be something we can easily tell the highlighter to show/hide on demand, when needed.

Comment on attachment 8791705 [details] [diff] [review] Part 1: Display the grid cell in the css grid overlay [1.0] Review of attachment 8791705 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I found one bug: highlighter.show(i.selection.nodeFront, {showGridArea: "a", showGridCell: {rowNumber: 2, columnNumber: 2}}); When you do this, the area and the cell are highlighted correctly, but the nodeinfobar only appears for the cell, and is sort of weird because it both has the name of the area and the coordinates of the cell displayed. We should either not support highlighting 2 things at the same time, or really support it, but not this in between state. I suggest not supporting it (since we have just one infobar for now anyway, and because I don't think there are use cases where this would be useful). I've also not been able to highlight cells in other fragments when the grid is fragmented. See my comment below. One last question: we've skipped tests so far, primarily because the highlighter was using <canvas> and because nothing made use of it in the production code so far. But we need to start worrying about tests now I think. The markup has become more complex and we're not only drawing to a <canvas>, and we're getting closer to actually using this highlighter in the inspector. So we really need to make sure it's tested. What's your plan for this? Have you already filed bug(s) for this? ::: devtools/server/actors/highlighters/css-grid.js @@ +57,5 @@ > * @param {String} areaName > * Shows the grid area highlight for the given area name. > * - showAllGridAreas > * Shows all the grid area highlights for the current grid. > + * - showGridCell({ rowNumber: Integer, columnNumber: Integer }) Maybe rename rowNumber to row and columnNumber to column? @@ +272,5 @@ > let box = this.getElement("areas"); > box.setAttribute("d", ""); > }, > > + showGridCell({ rowNumber, columnNumber }) { All other methods have jsdoc comments, please add some here too. @@ +279,5 @@ > + for (let i = 0; i < this.gridData.length; i++) { > + let fragment = this.gridData[i]; > + let {bounds} = this.currentQuads.content[i]; > + let row = fragment.rows.tracks[rowNumber - 1]; > + let column = fragment.cols.tracks[columnNumber - 1]; This is incorrect in some cases. Say you have this grid 1 2 +---+---+ 1| | | +---+---+ 2| | | +---+---+ 3| | | +---+---+ 4| | | +---+---+ Now, say it's split in 2 columns, it becomes: 1 2 1 2 +---+---+ +---+---+ 1| | | 3 | | | +---+---+ +---+---+ 2| | | 4 | | | +---+---+ +---+---+ If I want to highlight the cell in column 1, row 3, then I pass the following options: showGridCell: {rowNumber: 3, columnNumber: 1} which will cause this algorithm to not highlight anything. The first iteration, it won't find a row object in fragment.rows.tracks, because it only has 2 items and we want to access the 3rd. And in the second iteration, the same will happen, because in the second fragment, the fragment.rows.tracks array also has just 2 rows. So you'd need to subtract the number of tracks seen in the previous iterations. But the problem is, you'd need to know which direction is the grid split along. In that case it's split in columns, so you need to subtract rows only, but maybe there are cases where it could be split in rows? Not sure about this one. Might be worth checking with Brad. @@ -657,5 @@ > _showGrid() { > this.getElement("canvas").removeAttribute("hidden"); > }, > > - _hideGridArea() { You've renamed this method but there's still an occurrence of this in _hide().

Comment on attachment 8842312 [details] Bug 1302197 - Implement the option to show grid cell highlights in the CSS Grid Highlighter. https://reviewboard.mozilla.org/r/116174/#review117860 The code changes are easy to follow, it's pretty much the same as for areas, so nothing much to say here. I'm happy to R+ on this account. Now, I think we still need to figure out how the UI for this is going to work, should cells and areas be highlighted at the same time, etc. It might make things harder to see. ::: devtools/server/actors/highlighters/css-grid.js:109 (Diff revision 1) > + * <div class="css-grid-cell-infobar-container"> > + * <div class="css-grid-infobar"> > + * <div class="css-grid-infobar-text"> > + * <span class="css-grid-cell-infobar-position">Grid Cell Position</span> > + * <span class="css-grid-cell-infobar-dimensions"Grid Cell Dimensions></span> > * </div> > * </div> This is very similar to the area infobar. Do you think we could just have 1 for both areas and cells? That is, of course, only if we intend to not show areas *and* cells at the same time. I seem to remember you told me we might want to do that, although I'm not too sure how this would work in case the cell you're highlighting is wihtin the area being highlighted too.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/51bf67737ecb Implement the option to show grid cell highlights in the CSS Grid Highlighter. r=pbro