In bug 1181227, we're adding support for CSS Grids in devtools. Part of this means we need a way to highlight a grid on the page. This bug is a first step for this, and only should focus on: Displaying the lines of the grid for a given DOM element.

Here's a first draft for the CSS Grid highlighter. The patch I just uploaded only contains the highlighter itself, nothing uses it for now. For this we'd need bug 1282730. So, to see it in action, I use the following steps: - open a page that has a css grid (e.g. http://gridbyexample.com/examples/code/layout4.html ) - open the inspector and select the node with class 'wrapper' in the markup-view - open scratchpad, switch it to browser environment - execute the following code in scratchpad: /* -sp-context:browser */ var t = [...gDevTools._toolboxes][0][1]; var i = t.getPanel("inspector"); t.highlighterUtils.getHighlighterByType("CssGridHighlighter").then(h => { h.show(i.selection.nodeFront); }); This bug is only about display grid lines, which this does. So this shouldn't be too hard to land. There's a few rough edges, and of course we'd need some tests (although the whole highlighter UI is in <canvas>, so we can't test it).

Inspector bug triage (filter on CLIMBING SHOES).

Comment on attachment 8780630 [details] [diff] [review] 1282726.patch [3.0] Review of attachment 8780630 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Gabriel, this looks good. The code changes look good to me, but I'm not giving R+ now based on the following comments (which are, in my opinion, blockers unless noted otherwise): - Could you check with Mats Palmgren and Brad Werth what's the state of the CSS Grid implementation? I forget if it's behind a flag and enabled in nightly only. We need to make sure we don't ship devtools code that is based on a feature only enabled in nightly or something. - [not blocking R+] When there are many grid fragments (e.g. when a grid is inside a css columns layout like here: https://dl.dropboxusercontent.com/u/714210/filter-editor.html ): the grid highlighter inside the last column has a weird vertical offset. I think this is a platform issue with the getGridFragments method, but I'll let you check and discuss with Brad if needed. See this screenshot: https://dl.dropboxusercontent.com/u/714210/grid-highlighter-vertical-offset.png - [not blocking R+] The lines aren't always crisp. I've tested on windows 10, with a non-retina external monitor, and most of the times the lines aren't "pixel-snapped" so they appear a little bit blurry. See this screenshot: https://dl.dropboxusercontent.com/u/714210/grid-highlighter-blurry-lines.png - The highlighter doesn't handle browser zoom: - first of all, the lines get bigger as you zoom in, or thinner as you zoom out, making them hard to see - secondly, the highlighter position is wrong. See this screenshot: https://dl.dropboxusercontent.com/u/714210/grid-highlighter-zoom-offset.png - The new css-grid.js file seems to be clean of ESLint errors, so please make sure it isn't ignored in .eslintignore so we don't introduce new errors. Right now all actors are ignored. You can filter this one out of the ignore rule by adding: !devtools/server/actors/highlighters/css-grid.js ::: devtools/server/actors/highlighters/css-grid.js @@ +35,5 @@ > + "class": "highlighter-container" > + } > + }); > + > + // The highlighter is drawn on a single canvas element. This comment could use some more details. Like: We use a <canvas> element so we can draw an arbitrary number of lines, which we wouldn't be able to do with HTML or SVG without having to remove and insert the whole markup at every update (because we use the document.insertAnonymousContent API). @@ +103,5 @@ > + > + /** > + * Update the highlighter on the current highlighted node (the one that was > + * passed as an argument to show(node)). > + * Should be called whenever node size or attributes change This last comment line should be changed to something like: Should be called whenever the node's geometry or grid change. @@ +129,5 @@ > + let ratio = this.win.devicePixelRatio || 1; > + let width = this.win.innerWidth; > + let height = this.win.innerHeight; > + > + // Resize the canvas taking the dpr into account so as to have crisp lines. Based on my earlier comment about lines blurry, this obviously doesn't seem to work fine. A few years ago when I was working with <canvas> I used to employ the +.5 hack to make lines crisp. That is, offsetting all lines by 0.5px to prevent aliasing. It seems like this is already done in the renderLine method, so I don't know what's causing the lines to still be blurry. I've tried making a few changes to the code without finding a good solution. Maybe ping jdescottes, he has a lot of experience with canvas, he might be able to find a solution. @@ +205,5 @@ > + this.ctx.beginPath(); > + this.ctx.translate(.5, .5); > + this.ctx.moveTo(x1, y1); > + this.ctx.lineTo(x2, y2); > + this.ctx.strokeStyle = "#483D88"; Please move this up top as a constant. @@ +237,5 @@ > +}); > +exports.CssGridHighlighter = CssGridHighlighter; > + > +/** > + * Stringify CSS Grid data as returned by node.getGridFragments. Maybe worth asking Brad if there is something we can do on the platform side to avoid this?

(In reply to Patrick Brosset <:pbro> from comment #9) > Comment on attachment 8780630 [details] [diff] [review] > 1282726.patch [3.0] > > Review of attachment 8780630 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks Gabriel, this looks good. > The code changes look good to me, but I'm not giving R+ now based on the > following comments (which are, in my opinion, blockers unless noted > otherwise): > > - Could you check with Mats Palmgren and Brad Werth what's the state of the > CSS Grid implementation? I forget if it's behind a flag and enabled in > nightly only. We need to make sure we don't ship devtools code that is based > on a feature only enabled in nightly or something. > > - [not blocking R+] When there are many grid fragments (e.g. when a grid is > inside a css columns layout like here: > https://dl.dropboxusercontent.com/u/714210/filter-editor.html ): the grid > highlighter inside the last column has a weird vertical offset. I think this > is a platform issue with the getGridFragments method, but I'll let you check > and discuss with Brad if needed. See this screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-vertical-offset. > png > > - [not blocking R+] The lines aren't always crisp. I've tested on windows > 10, with a non-retina external monitor, and most of the times the lines > aren't "pixel-snapped" so they appear a little bit blurry. See this > screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-blurry-lines.png > > - The highlighter doesn't handle browser zoom: > - first of all, the lines get bigger as you zoom in, or thinner as you > zoom out, making them hard to see > - secondly, the highlighter position is wrong. See this screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-zoom-offset.png > > - The new css-grid.js file seems to be clean of ESLint errors, so please > make sure it isn't ignored in .eslintignore so we don't introduce new > errors. Right now all actors are ignored. You can filter this one out of the > ignore rule by adding: > !devtools/server/actors/highlighters/css-grid.js > > ::: devtools/server/actors/highlighters/css-grid.js > @@ +35,5 @@ > > + "class": "highlighter-container" > > + } > > + }); > > + > > + // The highlighter is drawn on a single canvas element. > > This comment could use some more details. Like: > We use a <canvas> element so we can draw an arbitrary number of lines, which > we wouldn't be able to do with HTML or SVG without having to remove and > insert the whole markup at every update (because we use the > document.insertAnonymousContent API). > > @@ +103,5 @@ > > + > > + /** > > + * Update the highlighter on the current highlighted node (the one that was > > + * passed as an argument to show(node)). > > + * Should be called whenever node size or attributes change > > This last comment line should be changed to something like: > > Should be called whenever the node's geometry or grid change. > > @@ +129,5 @@ > > + let ratio = this.win.devicePixelRatio || 1; > > + let width = this.win.innerWidth; > > + let height = this.win.innerHeight; > > + > > + // Resize the canvas taking the dpr into account so as to have crisp lines. > > Based on my earlier comment about lines blurry, this obviously doesn't seem > to work fine. > A few years ago when I was working with <canvas> I used to employ the +.5 > hack to make lines crisp. That is, offsetting all lines by 0.5px to prevent > aliasing. It seems like this is already done in the renderLine method, so I > don't know what's causing the lines to still be blurry. I've tried making a > few changes to the code without finding a good solution. Maybe ping > jdescottes, he has a lot of experience with canvas, he might be able to find > a solution. > > @@ +205,5 @@ > > + this.ctx.beginPath(); > > + this.ctx.translate(.5, .5); > > + this.ctx.moveTo(x1, y1); > > + this.ctx.lineTo(x2, y2); > > + this.ctx.strokeStyle = "#483D88"; > > Please move this up top as a constant. > > @@ +237,5 @@ > > +}); > > +exports.CssGridHighlighter = CssGridHighlighter; > > + > > +/** > > + * Stringify CSS Grid data as returned by node.getGridFragments. > > Maybe worth asking Brad if there is something we can do on the platform side > to avoid this? @bradwerth, as mentioned briefly on irc, just forwarding a needinfo to you to address some of the review comments that pbro made

Mainly, focusing on landing the css grid highlighter for the time being. (In reply to Patrick Brosset <:pbro> from comment #9) > Comment on attachment 8780630 [details] [diff] [review] > 1282726.patch [3.0] > > Review of attachment 8780630 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks Gabriel, this looks good. > The code changes look good to me, but I'm not giving R+ now based on the > following comments (which are, in my opinion, blockers unless noted > otherwise): > > - Could you check with Mats Palmgren and Brad Werth what's the state of the > CSS Grid implementation? I forget if it's behind a flag and enabled in > nightly only. We need to make sure we don't ship devtools code that is based > on a feature only enabled in nightly or something. > Waiting on the needinfo from brad, but I know css grid are only enabled in Nightly and DevEd. So, I don't believe it is riding the train yet. > - [not blocking R+] When there are many grid fragments (e.g. when a grid is > inside a css columns layout like here: > https://dl.dropboxusercontent.com/u/714210/filter-editor.html ): the grid > highlighter inside the last column has a weird vertical offset. I think this > is a platform issue with the getGridFragments method, but I'll let you check > and discuss with Brad if needed. See this screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-vertical-offset. > png > Waiting on needinfo from Brad regarding this. Can you provide a jsfiddle/test case to reproduce this? > - [not blocking R+] The lines aren't always crisp. I've tested on windows > 10, with a non-retina external monitor, and most of the times the lines > aren't "pixel-snapped" so they appear a little bit blurry. See this > screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-blurry-lines.png > Can you also provide a test case to reproduce this? I will not be focusing on pixel-snapping in this bug. Will file a follow up upon landing. > - The highlighter doesn't handle browser zoom: > - first of all, the lines get bigger as you zoom in, or thinner as you > zoom out, making them hard to see I tried setting lineWidth to 1, but I am not quite sure how to properly resolve this issue. Any suggestions? > - secondly, the highlighter position is wrong. See this screenshot: > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-zoom-offset.png > This should be fixed in patch 4.0 > - The new css-grid.js file seems to be clean of ESLint errors, so please > make sure it isn't ignored in .eslintignore so we don't introduce new > errors. Right now all actors are ignored. You can filter this one out of the > ignore rule by adding: > !devtools/server/actors/highlighters/css-grid.js > Added item to .estlintignore > ::: devtools/server/actors/highlighters/css-grid.js > @@ +35,5 @@ > > + "class": "highlighter-container" > > + } > > + }); > > + > > + // The highlighter is drawn on a single canvas element. > > This comment could use some more details. Like: > We use a <canvas> element so we can draw an arbitrary number of lines, which > we wouldn't be able to do with HTML or SVG without having to remove and > insert the whole markup at every update (because we use the > document.insertAnonymousContent API). > Fixed. Added additional comment detail. > @@ +103,5 @@ > > + > > + /** > > + * Update the highlighter on the current highlighted node (the one that was > > + * passed as an argument to show(node)). > > + * Should be called whenever node size or attributes change > > This last comment line should be changed to something like: > > Should be called whenever the node's geometry or grid change. > Fixed comment. > @@ +129,5 @@ > > + let ratio = this.win.devicePixelRatio || 1; > > + let width = this.win.innerWidth; > > + let height = this.win.innerHeight; > > + > > + // Resize the canvas taking the dpr into account so as to have crisp lines. > > Based on my earlier comment about lines blurry, this obviously doesn't seem > to work fine. > A few years ago when I was working with <canvas> I used to employ the +.5 > hack to make lines crisp. That is, offsetting all lines by 0.5px to prevent > aliasing. It seems like this is already done in the renderLine method, so I > don't know what's causing the lines to still be blurry. I've tried making a > few changes to the code without finding a good solution. Maybe ping > jdescottes, he has a lot of experience with canvas, he might be able to find > a solution. > Will work on pixel-snapping in a file up if this does not block r+. > @@ +205,5 @@ > > + this.ctx.beginPath(); > > + this.ctx.translate(.5, .5); > > + this.ctx.moveTo(x1, y1); > > + this.ctx.lineTo(x2, y2); > > + this.ctx.strokeStyle = "#483D88"; > > Please move this up top as a constant. > Fixed. Moved to a constant. > @@ +237,5 @@ > > +}); > > +exports.CssGridHighlighter = CssGridHighlighter; > > + > > +/** > > + * Stringify CSS Grid data as returned by node.getGridFragments. > > Maybe worth asking Brad if there is something we can do on the platform side > to avoid this? Waiting on needinfo from Brad.

Since it is only available on DevEd and Nightly, would there be anything specific I should do before this can land? (eg, adding a pref?)

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #13) > Since it is only available on DevEd and Nightly, would there be anything > specific I should do before this can land? (eg, adding a pref?) Is the CSS Grid implementation not riding the train yet? Or just getGridFragments? Or both? I doubt it but if, by chance, getGridFragments rides the train now (and just returns an empty array all the time on releases where CSS Grid hasn't shipped yet), then we have nothing to worry about. I think CSS Grid might be developed behind the flag layout.css.grid.enabled and this flag might be true by default on nightly/aurora and false by default on beta/release. I'm unsure how we deal with this in devtools usually. I guess we could just check the flag in our code. If it's false, then just don't show the UI for the grid inspector, and bail out of the grid highlighter code.

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #12) > > - [not blocking R+] When there are many grid fragments (e.g. when a grid is > > inside a css columns layout like here: > > https://dl.dropboxusercontent.com/u/714210/filter-editor.html ): the grid > > highlighter inside the last column has a weird vertical offset. I think this > > is a platform issue with the getGridFragments method, but I'll let you check > > and discuss with Brad if needed. See this screenshot: > > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-vertical-offset. > > png > Waiting on needinfo from Brad regarding this. Can you provide a > jsfiddle/test case to reproduce this? I already did, see the filter-editor.html link above, try to display the grid highlighter on node .functions and you should see the problem (make sure you resize your browser window so that there are at least 3 columns in the lasout). > > - [not blocking R+] The lines aren't always crisp. I've tested on windows > > 10, with a non-retina external monitor, and most of the times the lines > > aren't "pixel-snapped" so they appear a little bit blurry. See this > > screenshot: > > https://dl.dropboxusercontent.com/u/714210/grid-highlighter-blurry-lines.png > Can you also provide a test case to reproduce this? I will not be focusing > on pixel-snapping in this bug. Will file a follow up upon landing. There's no need for a specific test case for this, this happens all the time. You can use the same html file I linked above, open it on a non-retina display and then show the grid highlighter. You'll see the lines aren't crisp. > > - The highlighter doesn't handle browser zoom: > > - first of all, the lines get bigger as you zoom in, or thinner as you > > zoom out, making them hard to see > > I tried setting lineWidth to 1, but I am not quite sure how to properly > resolve this issue. Any suggestions? The problem is that when the page zooms in/out, so does the anonymous content in the CanvasFrame, because that's part of the page. And since we use a <canvas> element to draw the highlighter, that just ends up zooming in/out the canvas element, and its content. The only way to properly fix this is to take the current page zoom level into account when drawing and adapting the lineWidth with it. The nice thing about this highlighter is that it's an auto-refresh one, so there's no need to listen for an event to do this, as soon as the user zooms the page, _update will be called. So, in update, you'll need to retrieve the zoom level (use shared/layout/utils.js' getCurrentZoom function), and then try and experiment with this. I can't guaranty anything, I've never tried this.

Comment on attachment 8782795 [details] [diff] [review] 1282726.patch [4.0] Review of attachment 8782795 [details] [diff] [review]: ----------------------------------------------------------------- Nice. I see all of my comments have been addressed. The only thing left before we can land this is figuring out how we deal with CSS Grid not being enabled on beta/release. See comment 14, but we should wait Brad's answer before we land. R+ assuming this is done and you have a green try build.

GetGridFragments has landed (excepting the grid areas properties in bug 1289200) but the css grid feature itself will remain behind a flag until bug 1217086 is resolved. The strategy in comment 14 seems sound to me.

https://hg.mozilla.org/integration/fx-team/rev/6b1f62a32be285d0d8956629b2860fb5212bc8de Bug 1282726 - Simple CSS Grid highlighter that displays grid lines r=pbro

Did this land in Nightly? It would be great to see it running. In response to the questions above about the status of CSS Grid -- it is currently enabled in Nightly and Dev Edition without requiring the user to flip a flag. Work is still underway, so Nightly has a more complete implementation than Dev Edition, but both implementations are very close. The majority of basic functionality is ready. It's down to bug fixing and working out details of advanced stuff. For sure, we don't want to accidentally ship the dev tools for Grid into Firefox release before Grid ships enabled in Release. But meanwhile, I would really like to see the Grid tools enabled in Nightly as they are ready. We could then try it out. Discuss what's happening. Test it with real users. All with the ability to still make big changes as we learn things. If we wait to enable it for the first time in Release, when CSS Grid ships, we will be stuck with our first draft. Is anything ready to be looked at? It looks from Comment 20 like something landed. Can I use it without building a special version of Firefox? Can I show it to others, and ask for feedback?

(In reply to Jen Simmons [:jensimmons] from comment #22) > Did this land in Nightly? It would be great to see it running. > > In response to the questions above about the status of CSS Grid -- it is > currently enabled in Nightly and Dev Edition without requiring the user to > flip a flag. Work is still underway, so Nightly has a more complete > implementation than Dev Edition, but both implementations are very close. > The majority of basic functionality is ready. It's down to bug fixing and > working out details of advanced stuff. > > For sure, we don't want to accidentally ship the dev tools for Grid into > Firefox release before Grid ships enabled in Release. But meanwhile, I would > really like to see the Grid tools enabled in Nightly as they are ready. We > could then try it out. Discuss what's happening. Test it with real users. > All with the ability to still make big changes as we learn things. If we > wait to enable it for the first time in Release, when CSS Grid ships, we > will be stuck with our first draft. > > Is anything ready to be looked at? It looks from Comment 20 like something > landed. Can I use it without building a special version of Firefox? Can I > show it to others, and ask for feedback? Hey Jen, this is landed in Nightly. However, this only provides us with a simple grid highlighter that would still require some integration with the DevTools front end to be able to see the highlighter. The integration is happening in Bug 1297100 and Bug 1302536. Currently, Bug 1297100 is prioritize and will probably land some time this week. The second priority that I am working on is getting the Grid Layout sidepanel up and going. To catch all the action, I would follow Bug 1181227. This bug will catch everything we are trying to do to land a CSS Grid Inspector MVP. I will ping you separately when there is something more to look at and present to others, but probably the first thing that will be available for feedback to the general public will be from Bug 1297100.

Thanks for the update Gabriel. And for all your work on this. Congrats on landing this piece. I'm excited to see what comes next.

I've written a short page introducing the grid highlighter: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_grid_layouts. Please let me know if we need anything else here.

(In reply to Will Bamberg [:wbamberg] from comment #25) > I've written a short page introducing the grid highlighter: > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/ > Examine_grid_layouts. > > Please let me know if we need anything else here. Thanks Will. This looks great. I'll forward the NI? to Gabriel since he was the one who worked on this, I think he should take a look.

Looks good Will