In bug 1181227, we're building a CSS grid overlay tool. It'd be very helpful if this tool would also show grid areas as colorized rectangles with names on the grid.

Part 1 only adds the grid area highlight overlay. The fill color is a color that is used as placeholder until Helen has an updated spec with all the styles (consider it landable for now). I purposely separated the patch into multiple parts so the next one would contain the grid info bar since it was getting into a big/complex patch.

Comment on attachment 8786055 [details] [diff] [review] Part 1: Display the grid area highlight in the css grid overlay [1.0] Review of attachment 8786055 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I've made a few minor comments in the code, but more importantly, I have a high level comment: I'd prefer it if we hold off of handling mouse events before we figure out what we do with bug 1297100. I think there are 2 main use cases for this highlighter: - integrated with the Box-Model highlighter so that when you select elements in the page, you see the grid being highlighted when needed --> handling mouse over to highlight areas in this mode would conflict with selecting elements that may be inside these areas, - and from the new layout panel --> in this mode, I don't see much use for handling mouse events, since you'd instead have a ON/OFF toggle button to show/hide the grid and possibly checkboxes to decide if you want to see things like line names, numbers, areas, gaps, etc... So, so far, I tend to think that we should instead have an option to show areas or not, and when set to true, we should show them all at once (the nice thing with using an SG <path> element is that it can be used to draw as many rectangles as we want without having to add extra elements). This means that I would remove the event handling from this patch altogether, at least for now. Maybe we should have 2 options: - showAreas that, when set to true, draws rectangles for all areas in the grid, - showArea that, when set to the name of an area, draw only one rectangle for just the one area with the corresponding name in the grid. Also, if we add methods in the class like showArea(name) and showAllAreas() then it'd be easy enough later to call them from mouse event handlers if we do decide to have those. ::: devtools/server/actors/highlighters.css @@ +96,5 @@ > stroke: var(--highlighter-guide-color); > stroke-dasharray: 5 3; > shape-rendering: crispEdges; > } > +:-moz-native-anonymous .css-grid-area { I'd prefer to have all css-grid highlighter rules grouped together, so please move this down to where you've put the .css-grid.canvas rule. ::: devtools/server/actors/highlighters/css-grid.js @@ +56,5 @@ > + * Structure: > + * <div class="highlighter-container"> > + * <canvas id="css-grid-canvas" class="css-grid-canvas"> > + * <svg class="css-grid-elements" hidden="true"> > + * <g class="css-grid-regions"> What is the purpose of this extra <g> element? Looking at the code it doesn't seem to be used, apart for setting an opacity of 0.6, which could equally be set on the css-grid-elements element or the css-grid-area element instead. @@ +123,5 @@ > + createSVGNode(this.win, { > + nodeType: "path", > + parent: regions, > + attributes: { > + "class": "area", I would call this "areas" instead of "area" as per my initial comment that we should have an option that shows all areas at once. @@ +147,5 @@ > }, > get canvas() { > return this.getElement("canvas"); > }, > + /** Please add a newline between method definitions. Also I see there is no newline before destroy, getElement, get ctx, get canvas. There should be.

Comment on attachment 8786452 [details] [diff] [review] Part 2: Display the grid info bar for a grid area highlight [1.0] Review of attachment 8786452 [details] [diff] [review]: ----------------------------------------------------------------- Just a quick review because I couldn't test this. First of all, this patch needs to be rebased, and secondly (maybe I rebased it wrong locally), I'm not seeing the infobar in the highlighter when testing. ::: devtools/server/actors/highlighters.css @@ +109,3 @@ > > +:-moz-native-anonymous .box-model-nodeinfobar-container, > +:-moz-native-anonymous .css-grid-gridinfobar-container { We unfortunately can't use the same classname here because of the prefix we use to scope highlighters, but maybe we could simplify all these selectors by doing: - rename nodeinfobar to infobar in the box-model highlighter - rename gridinfobar to infobar in the css-grid highlighter - and use selectors like: :-moz-native-anonymous [class$=infobar-container] This way it's shorter, and can be reused by any other highlighter we have in the future that will need infobars that share a consistent look. This means that something like: :-moz-native-anonymous .box-model-nodeinfobar-container[hide-arrow] > .box-model-nodeinfobar, :-moz-native-anonymous .css-grid-gridinfobar-container[hide-arrow] > .css-grid-gridinfobar would also become: :-moz-native-anonymous [class$=infobar-container][hide-arrow] > [class$=infobar] ::: devtools/server/actors/highlighters/box-model.js @@ +687,5 @@ > > /** > * Move the Infobar to the right place in the highlighter. > + * > + * This logic is reused in css-grid.js#_updateInfobar. Well, it's copied, not reused, right? Can you think of ways we could share the code instead? I think a simple helper function in devtools\server\actors\highlighters\utils\ would be nice.

Comment on attachment 8787322 [details] [diff] [review] Part 1: Display the grid area highlight in the css grid overlay [2.0] Review of attachment 8787322 [details] [diff] [review]: ----------------------------------------------------------------- Very close to R+, I just would like to review this one last time after you've added the options. Also, what about tests? We should add one that makes sure that when you set the right options, then the right svg path is created. ::: devtools/server/actors/highlighters/css-grid.js @@ +55,3 @@ > * Available Options: > + * - hideGridAreas {Boolean} > + * Hide the grid areas highlight hideGridAreas isn't actually implemented, please remove this option doc. Adding the 2 public methods showGridArea and showAllGridAreas is a good idea, but I think we should *also* add them as options. So I would add in this options doc: - showGridArea {String} Show a given area highlight - showAllGridAreas {Boolean} Show all the grid area highlights in the grid This way it will be very easy to customize what we want to see in the highlighter from the front-end (remember the front-end can't call other methods than show and hide on a highlighter, highlighters are wrapped into the CustomHighlighterActor actors that expose show/hide as actor methods). I think that inside the new Layout sidebar panel, we may want to have checkboxes or other things that show/hide areas or 1 area in particular. So having these options available will prove useful. @@ +232,5 @@ > for (let i = 0; i < this.gridData.length; i++) { > let fragment = this.gridData[i]; > let quad = this.currentQuads.content[i]; > this.renderFragment(fragment, quad); > } If you add the options that I'm suggesting you should add, then here you'd have something like: // And the areas if needed. if (this.options.showAllGridAreas) { this.showAllGridAreas(); } else if (this.options.showGridArea) { this.showGridArea(this.options.showGridArea); } @@ +419,5 @@ > + * Render the grid area highlight for the given area name or for all the grid areas. > + * > + * @param {String} areaName > + * Grid area name. > + * @param {Boolean} options.renderAllGridAreas Why did you write options.renderAllGridAreas here? renderAllGridAreas isn't an exposed option. In fact I think this method should just take 1 argument: renderGridArea(areaName) { ... } and if it's falsy, then all grid areas are drawn? Or something like this. @@ +442,5 @@ > + let columnEnd = fragment.cols.lines[area.columnEnd - 1]; > + > + let x1 = columnStart.start + columnStart.breadth + > + (bounds.left / getCurrentZoom(this.win)); > + let x2 = columnEnd.start + (bounds.left / getCurrentZoom(this.win)); Using bounds.left here (and bounds.top further below) will make this not work for different writing directions. I think this should depend on the horizontal and vertical writing directions, which you can get from the computed style of the currentnode I guess. Note that RTL,etc. support could be done later. @@ +445,5 @@ > + (bounds.left / getCurrentZoom(this.win)); > + let x2 = columnEnd.start + (bounds.left / getCurrentZoom(this.win)); > + let y1 = rowStart.start + rowStart.breadth + > + (bounds.top / getCurrentZoom(this.win)); > + let y2 = rowEnd.start + (bounds.top / getCurrentZoom(this.win)); I don't know how costly getCurrentZoom is in practice, but it does a number of stuff that might take time, so I suggest calling it just once at the start of this function (it's not going to change during the function execution anyway). This is because you call it 4 times inside 2 nested for loops, so let's be careful here.

Comment on attachment 8787538 [details] [diff] [review] Part 2: Display the grid info bar for a grid area highlight [2.0] Review of attachment 8787538 [details] [diff] [review]: ----------------------------------------------------------------- Only a couple of comments. But please make sure test pass. Also, we need new tests for this new css-grid infobar. R+ assuming you'll work on them in a later patch. ::: devtools/server/actors/highlighters/box-model.js @@ +67,5 @@ > * <line class="box-model-guide-right" x1="..." y1="..." x2="..." y2="..." /> > * <line class="box-model-guide-bottom" x1="..." y1="..." x2="..." y2="..." /> > * <line class="box-model-guide-left" x1="..." y1="..." x2="..." y2="..." /> > * </svg> > + * <div class="box-model-infobar-container"> Thanks for changing these classes as I requested. However I think that will break tests in devtools/client/inspector/test. If you plan to fix them in a later patch, than you will need to eventually fold these 2 patches together to avoid landing a patch that has failing tests. ::: devtools/server/actors/highlighters/utils/markup.js @@ +522,5 @@ > }; > exports.CanvasFrameAnonymousContentHelper = CanvasFrameAnonymousContentHelper; > + > +/** > + * Move the infobar to the right place in the highlighter. Now that the concept of "infobar" has been moved to this helper file, I think it requires some documentation for future readers. When it was only in the box-model.js file, it was fine because it was obvious what it was for. Now that it's shared, it's sort of a reusable widget, it needs doc. So please add a few lines here to simply say what is even an infobar, what we use it for, and what is its behavior.

(In reply to Patrick Brosset <:pbro> from comment #7) > Comment on attachment 8787322 [details] [diff] [review] > Part 1: Display the grid area highlight in the css grid overlay [2.0] > > Review of attachment 8787322 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very close to R+, I just would like to review this one last time after > you've added the options. > Also, what about tests? We should add one that makes sure that when you set > the right options, then the right svg path is created. > Tests will be in part 3. > ::: devtools/server/actors/highlighters/css-grid.js > @@ +55,3 @@ > > * Available Options: > > + * - hideGridAreas {Boolean} > > + * Hide the grid areas highlight > > hideGridAreas isn't actually implemented, please remove this option doc. > > Adding the 2 public methods showGridArea and showAllGridAreas is a good > idea, but I think we should *also* add them as options. So I would add in > this options doc: > > - showGridArea {String} > Show a given area highlight > - showAllGridAreas {Boolean} > Show all the grid area highlights in the grid > > This way it will be very easy to customize what we want to see in the > highlighter from the front-end (remember the front-end can't call other > methods than show and hide on a highlighter, highlighters are wrapped into > the CustomHighlighterActor actors that expose show/hide as actor methods). > I think that inside the new Layout sidebar panel, we may want to have > checkboxes or other things that show/hide areas or 1 area in particular. So > having these options available will prove useful. > I decided to remove the public methods since we can only call show and hide. Added the new options. > @@ +232,5 @@ > > for (let i = 0; i < this.gridData.length; i++) { > > let fragment = this.gridData[i]; > > let quad = this.currentQuads.content[i]; > > this.renderFragment(fragment, quad); > > } > > If you add the options that I'm suggesting you should add, then here you'd > have something like: > > // And the areas if needed. > if (this.options.showAllGridAreas) { > this.showAllGridAreas(); > } else if (this.options.showGridArea) { > this.showGridArea(this.options.showGridArea); > } > > @@ +419,5 @@ > > + * Render the grid area highlight for the given area name or for all the grid areas. > > + * > > + * @param {String} areaName > > + * Grid area name. > > + * @param {Boolean} options.renderAllGridAreas > > Why did you write options.renderAllGridAreas here? renderAllGridAreas isn't > an exposed option. > In fact I think this method should just take 1 argument: > > renderGridArea(areaName) { ... } > > and if it's falsy, then all grid areas are drawn? Or something like this. > Fixed. Only takes 1 option. > @@ +442,5 @@ > > + let columnEnd = fragment.cols.lines[area.columnEnd - 1]; > > + > > + let x1 = columnStart.start + columnStart.breadth + > > + (bounds.left / getCurrentZoom(this.win)); > > + let x2 = columnEnd.start + (bounds.left / getCurrentZoom(this.win)); > > Using bounds.left here (and bounds.top further below) will make this not > work for different writing directions. > I think this should depend on the horizontal and vertical writing > directions, which you can get from the computed style of the currentnode I > guess. > Note that RTL,etc. support could be done later. > TODO: RTL support in a new bug. > @@ +445,5 @@ > > + (bounds.left / getCurrentZoom(this.win)); > > + let x2 = columnEnd.start + (bounds.left / getCurrentZoom(this.win)); > > + let y1 = rowStart.start + rowStart.breadth + > > + (bounds.top / getCurrentZoom(this.win)); > > + let y2 = rowEnd.start + (bounds.top / getCurrentZoom(this.win)); > > I don't know how costly getCurrentZoom is in practice, but it does a number > of stuff that might take time, so I suggest calling it just once at the > start of this function (it's not going to change during the function > execution anyway). This is because you call it 4 times inside 2 nested for > loops, so let's be careful here. Fixed. Added currentZoom as a variable outside the loop

Fixed tests and added more docs.

Comment on attachment 8787708 [details] [diff] [review] Part 1: Display the grid area highlight in the css grid overlay [3.0] Review of attachment 8787708 [details] [diff] [review]: ----------------------------------------------------------------- Only a few final cleanups of extra parameters and jsdoc. Thanks! ::: devtools/server/actors/highlighters/css-grid.js @@ +168,5 @@ > + * @param {String} areaName > + * Grid area name. > + */ > + showGridArea(areaName) { > + this.renderGridArea(areaName, { renderAllGridAreas: false }); The second parameter here is not needed. @@ +176,5 @@ > + /** > + * Shows all the grid area highlights for the current grid. > + */ > + showAllGridAreas() { > + this.renderGridArea(null, { renderAllGridAreas: true }); Same here, in fact the first either. @@ +423,5 @@ > + /** > + * Render the grid area highlight for the given area name or for all the grid areas. > + * > + * @param {String|Null} areaName > + * Name of the grid area to be highlighted. Null if all the grid areas should To highlight all grid areas 'null' is not required, it could be undefined, false, 0, ... anything falsy. So please rephrase as: @param {String} areaName Name of the grid area to be highlighted. If missing, all grid areas are highlighted.

https://hg.mozilla.org/integration/fx-team/rev/443263a8af4effc5c55b806d9fc87263061bb085 Bug 1249558 - Part 1: Display the grid area highlight in the css grid overlay r=pbro https://hg.mozilla.org/integration/fx-team/rev/f46da98e6eddfc6df2b5e0aa565cc47fd752c0a7 Bug 1249558 - Part 2: Display the grid info bar for a grid area highlight r=pbro

https://hg.mozilla.org/integration/fx-team/rev/32a0c3b7b9923044adc93dde8c4debc8569ff840 Backed out changeset f46da98e6edd (Bug 1249558) r=backout

https://hg.mozilla.org/integration/fx-team/rev/fcd122b8e0035342fce993bfa5a75fe1a4b8eb56 Bug 1249558 - Part 2: Display the grid info bar for a grid area highlight r=pbro

https://hg.mozilla.org/mozilla-central/rev/443263a8af4e https://hg.mozilla.org/mozilla-central/rev/fcd122b8e003

Gabriel: should this remain open? I think we should close it.

(In reply to Patrick Brosset <:pbro> from comment #19) > Gabriel: should this remain open? I think we should close it. We should keep this open until I write unit tests for grid area.