A little similarly to how we have color swatches icons in the rule-view next to colors, we should have grid icons next to 'display:grid;' declarations in the rule-view. Clicking on them would toggle the grid highlighter on the corresponding grid container element. This is super useful when making changes to grid-template-columns, grid-template-rows and grid-gaps properties. It just makes sense to be able to see the grid while you edit any of these properties, otherwise you're basically working blindly. So, simple UX, clicking on the icon would show the grid. Clicking again should hide it. But I'm going to suggest that if you select another element in the inspector, then that should also hide the grid. That's because when you do, the rules in the rule-view are refreshed, and you won't see the icon anymore, which would make it super hard to get rid of the grid highlighter.

Oh, and another thing that might make the so called "simple UX" not so simple in the end: https://bugzilla.mozilla.org/show_bug.cgi?id=1297100#c7 describes how the grid highlighter should be integrated with our usual highlighter. So, if you've toggled a grid from the rule-view, and then go and use the element picker to select another node, then you might find yourself highlighting the same grid twice. So ideally, we should really be using just one instance of the grid highlighter.

I don't think we want add this considering we are gonna working on the CSS Grid layout panel. Marking this as a closed this reason.

I'd like us to consider re-opening this. I don't think the 2 things are incompatible. In fact, when you're working in the rule-view, making changes to css properties such as grid-template-rows, having a quick way to display the grid would be really cool. This would avoid having to switch to the layout panel and toggle it from there.

Sorry Gabriel, I had completely forgotten about this. I found an old commit in my local commits that I must have worked on while traveling that does this. I don't remember how far I went and some things might not work. But at least it's rebased, and might be useful to you working on this.

Comment on attachment 8804378 [details] [diff] [review] 1302536.patch Review of attachment 8804378 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. A few UX comments: - If there are multiple grids in the page, you can highlight one, then find the other one and click on the icon to highlight it: what happens then is that the first grid is hidden and you have to click a second time to highlight the second grid. It would be nice, when you're clicking on a different grid icon, if the previous grid would hide and the new one would show, all in one go, without requiring an extra click. - Related: the icon of the first grid should turn back to grey when the second one is clicked. Try these STR: - open http://labs.jensimmons.com/examples/grid-content-1.html - select the <main> element - in the rule-view there are 2 display:grid declarations, one active and one overridden - click on the first one to display the grid - click on the second one --> the grid is hidden (OK), but the first grid icon is still blue (not OK). The only way to de-activate it is to click on the first icon again. - Related again: the grid icon de-activates even if the grid is still shown when you go an select other nodes. Try this: - open http://labs.jensimmons.com/examples/grid-content-1.html - select the <main> element - in the rule-view, click on the first grid icon to display the grid - now select another element, like a <li> or something - go back and select the <main> element again --> the grid icon is de-activated, even though the grid is still shown This last point reminds me of bug 1309611. Navigating in the DOM with locked highlighters makes it hard to unlock them later, because it might be hard to remember which nodes had the highlighters been locked on. Unfortunately, this isn't going to be super easy to fix, because the style-inspector-overlay thing wasn't made for this in the first place, and we don't have any piece of state that tells us which node is highlighted, etc. ::: devtools/client/inspector/rules/test/browser_rules_grid-toggle.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test toggling the grid highlighter in the rule view. You should also add another test that checks what happens when you click on a second grid icon. ::: devtools/client/shared/output-parser.js @@ +201,5 @@ > if (options.expectCubicBezier && > BEZIER_KEYWORDS.indexOf(token.text) >= 0) { > this._appendCubicBezier(token.text, options); > + } else if (options.expectDisplay && token.text === "grid" && > + text === token.text) { I think the Services.prefs.getBoolPref(CSS_GRID_ENABLED_PREF) check should be part of this condition, not the one inside _appendGrid. This way, if grids are not supported, we just default to the final else and append a textNode. @@ +302,5 @@ > + * _mergeOptions() > + */ > + _appendGrid: function (options) { > + let value = this._createNode("span", {}); > + value.textContent = "grid"; We should find a way to append the authored text here rather than the hard-coded "grid" string. I might be wrong, but since we support authored text now, we don't trim values anymore, and therefore it's important to keep the original formatting of the value. @@ +304,5 @@ > + _appendGrid: function (options) { > + let value = this._createNode("span", {}); > + value.textContent = "grid"; > + > + if (Services.prefs.getBoolPref(CSS_GRID_ENABLED_PREF)) { As mentioned before, you can remove this condition here since it's checked earlier. ::: devtools/client/themes/rules.css @@ +387,5 @@ > + height: 9px; > + margin-inline-start: .5em; > + margin-inline-end: 0; > + color: var(--theme-graphs-grey); > + background-image: I had used a linear-gradient in my initial patch because it was faster for me to do this than creating a SVG. But I think we might need a SVG here? The icon looks blurry on my monitor. See https://dl.dropboxusercontent.com/u/714210/grid-icon.png

(In reply to Patrick Brosset <:pbro> from comment #9) > Comment on attachment 8804378 [details] [diff] [review] > 1302536.patch > > Review of attachment 8804378 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/themes/rules.css > @@ +387,5 @@ > > + height: 9px; > > + margin-inline-start: .5em; > > + margin-inline-end: 0; > > + color: var(--theme-graphs-grey); > > + background-image: > > I had used a linear-gradient in my initial patch because it was faster for > me to do this than creating a SVG. But I think we might need a SVG here? The > icon looks blurry on my monitor. See > https://dl.dropboxusercontent.com/u/714210/grid-icon.png Helen, can you help provide an SVG icon for the grid toggle?

This one has a stroke color attribute that should be removed (just so that it can be downloaded and checked in a lower dpi monitor, I'm on 300 dpi for the day).

Comment on attachment 8804378 [details] [diff] [review] 1302536.patch ✨ the UI review I am not flagged for ✨. I hope that's okay. Two things I notice with the patch as it is: Currently the lines in gutters/tracks overlap each other. It causes some of the cross-sections to look like you can close them somehow: https://cl.ly/1D390m3G2D0p Is it possible to have horizontal track hashes supersede the other, and have vertical gutter track hashes not appear when they are overlapped? (I'm sure that's a nightmare of a visual problem to fix, and I apologize for that.) Second, the grid and the page, for whatever reason, seem like they're scrolling at different rates: https://cl.ly/290r1i3C251i If we can make them seem more locked, I think we'll make people feel less dizzy scrolling the page.

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #12) > Second, the grid and the page, for whatever reason, seem like they're > scrolling at different rates: https://cl.ly/290r1i3C251i If we can make them > seem more locked, I think we'll make people feel less dizzy scrolling the > page. That's bug 1312103. The page scrolls with APZ (asynchronously), but the highlighter uses JS to scroll, and so doesn't scroll at the same time. This looks really bad, especially if the highlighter is heavy in terms of size, markup, styles, it makes it all the more slow to scroll. There is a solution in bug 1312103 that we should spend time implementing. It may take some time though, but we should figure out whether it should block this bug or not.

(In reply to Patrick Brosset <:pbro> from comment #13) > That's bug 1312103. The page scrolls with APZ (asynchronously), but the > highlighter uses JS to scroll, and so doesn't scroll at the same time. This > looks really bad, especially if the highlighter is heavy in terms of size, > markup, styles, it makes it all the more slow to scroll. > There is a solution in bug 1312103 that we should spend time implementing. > It may take some time though, but we should figure out whether it should > block this bug or not. I don't think it's worth blocking on but I'd like a second opinion. Gabe, when you are ready for reviews, can you pass this by the accessibility team (:yzen would probably be happy to oblige) and make sure that there aren't any concerns for users with epilepsy?

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #14) > (In reply to Patrick Brosset <:pbro> from comment #13) > > That's bug 1312103. The page scrolls with APZ (asynchronously), but the > > highlighter uses JS to scroll, and so doesn't scroll at the same time. This > > looks really bad, especially if the highlighter is heavy in terms of size, > > markup, styles, it makes it all the more slow to scroll. > > There is a solution in bug 1312103 that we should spend time implementing. > > It may take some time though, but we should figure out whether it should > > block this bug or not. > > I don't think it's worth blocking on but I'd like a second opinion. > > Gabe, when you are ready for reviews, can you pass this by the accessibility > team (:yzen would probably be happy to oblige) and make sure that there > aren't any concerns for users with epilepsy? Will do

I removed the check for a valid node in auto-refresh#hide() since the current node will no longer exist when we will-navigate and want to hide the highlighters.

Comment on attachment 8810757 [details] [diff] [review] 1302536.patch [1.0] Review of attachment 8810757 [details] [diff] [review]: ----------------------------------------------------------------- Before you squash these 2 patches. A minor comment. ::: devtools/client/shared/output-parser.js @@ +298,5 @@ > + * Append a CSS Grid highlighter toggle icon next to the value in a > + * 'display: grid' declaration > + * > + * @param {String} grid > + * The grid text vlue to append s/vlue/value

Comment on attachment 8810788 [details] [diff] [review] 1302536.patch [2.0] Review of attachment 8810788 [details] [diff] [review]: ----------------------------------------------------------------- I wasn't able to manually test the patch unfortunately. That will take me more time to do. But I can already give you some feedback on this patch. Some manual QA is needed here, to make sure this works well, so I'll try and do this next. I understand you're going to upload another patch to add more tests, right? So, assuming this and a try build, let's R+ this change. ::: devtools/client/inspector/shared/highlighters-overlay.js @@ +100,5 @@ > + > + // Otherwise, hide any existing grid highlighter and show the grid highlighter > + // for the given node. > + highlighter.hide(); > + return highlighter.show(node); I don't think you need to call hide first. The show method does what's needed to update to the currently highlighted node anyway. Only one node can be highlighted at once. If you do need to call hide, then you might need to wait for it to resolve (maybe change this function to be a Task instead, to make it more readable). @@ +104,5 @@ > + return highlighter.show(node); > + }).then(isGridShown => { > + if (isGridShown) { > + // Turn on all the grid toggle icons in the current rule view. > + for (let gridIcon of this.view.element.querySelectorAll(".ruleview-grid")) { This loop is repeated twice in the if and in the else parts. You could move it above of the if/else and use toggle instead of add/remove: // Toggle all the grid icons in the current rule view. for (let gridIcon of this.view.element.querySelectorAll(".ruleview-grid")) { gridIcon.classList.toggle("active", isGridShown); }

I've done some manual testing and here's what I found: - If you highlight a grid, and then use the pick tool and hover over elements in the page, you'll see that the nodeinfobar appears below the grid. I remember we solved this in bug 1297100 before. It might be possible to use the fix from there. - If you have visited several pages (so they're in bfcache), go back, highlight a grid, go forward, and back again, then the grid is still visible even though devtools thinks its not. As discussed on IRC this would help: https://irccloud.mozilla.com/pastebin/O6lcr9fl/

Added the unit tests.

Comment on attachment 8811189 [details] [diff] [review] 1302536.patch [4.0] Review of attachment 8811189 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. R+ with a couple of test comment to be addressed and assuming you get a green try build with this. ::: devtools/client/inspector/rules/test/browser_rules_edit-display-grid-property.js @@ +62,5 @@ > + yield onHighlighterHidden; > + yield onDone; > + > + info("Check the grid highlighter is hidden."); > + ok(!highlighters.gridHighlighterShown, "No CSS grid highlighter is shown."); Maybe also access the DOM of the rule-view to check that the grid icon is gone. ::: devtools/client/inspector/rules/test/browser_rules_grid-toggle_03.js @@ +10,5 @@ > + <style type='text/css'> > + .grid { > + display: grid; > + } > + .cell1 { Adding CSS rules for cell1/2/3/4 in this test (and in other similar) isn't needed and makes the tests unnecessarily longer I think. Let's just leave these divs be auto-positioned in the grid.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9611f0e821c2fe892e5a42002c71f94b2662cd70 Bug 1302536 - Adds a toggleable button next to the 'display: grid' declaration to toggle the grid highlighter r=pbro

It would be great to get some QA done on this tool. But probably only after bug 1318019 has landed.

Comment on attachment 8811220 [details] [diff] [review] 1302536.patch [6.0] Approval Request Comment [Feature/regressing bug #]: 1302536 [User impact if declined]: No grid tooling shipped with CSS grid that is intended to be shipped in 52 [Describe test coverage new/current, TreeHerder]: Added extensive test coverage [Risks and why]: Minimal visible risk since the new feature is only added to a 'display: grid' declaration in the rule-view. [String/UUID change made/needed]: Added rule.gridToggle.tooltip string in devtools/shared/locales/en-US/styleinspector.properties

Some help for QA testing this tool: A short video of what the tool does and looks like is available here: https://dl.dropboxusercontent.com/u/714210/css-grid/grid3.mp4 The tool itself is really small: in the rule-view (the inspector sidebar where css rules are displayed), if an element has the display:grid; declaration applied, then a small grid icon will appear next to the value "grid". Clicking on this grid icon will toggle the grid highlighter on the page. The grid highlighter is a piece of UI that overlays an element and displays where CSS grid lines and gaps are. CSS Grids is really new and has a lot of ways to position elements and define the grid, so this tool helps with seeing the grid and that is very useful when modifying the position of elements, or changing the definition of the grid. The intended use case is for someone to find an element with display:grid, toggle the grid highlighter, then start changing other values in the rule-view, like grid-template-rows, grid-template-columns, grid-template-areas, grid-gaps, but also css properties of items within the grid, like grid-column, grid-row, grid-area, etc. As this happens, the user would see the grid highlighter update in real time and would understand better how those properties work. The user would then toggle off the grid highlighter by clicking on the icon again. In terms of QA, it would be useful to test how the rule-view behaves once the grid icon has been enabled. Editing other properties, selecting another node and coming back to the first one, refreshing the page, navigating, trying to highlight a node within an iframe, etc. We should make sure that the experience is as predictable as possible.

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #33) > [String/UUID change made/needed]: Added rule.gridToggle.tooltip string in > devtools/shared/locales/en-US/styleinspector.properties We don't normally take string changes in aurora, per https://wiki.mozilla.org/Release_Management/Uplift_rules#Aurora_Uplift_.28approval-mozilla-aurora.29 so that would need an ok from l10n.

(In reply to Julien Cristau [:jcristau] from comment #35) > (In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from > comment #33) > > [String/UUID change made/needed]: Added rule.gridToggle.tooltip string in > > devtools/shared/locales/en-US/styleinspector.properties > > We don't normally take string changes in aurora, per > https://wiki.mozilla.org/Release_Management/Uplift_rules#Aurora_Uplift_. > 28approval-mozilla-aurora.29 so that would need an ok from l10n. To help take this decision: CSS Grid has been heavily discussed in blogs and conferences this year, there is a lot of excitement for it, and Firefox is shipping it in 52, and Chrome also announced that it was shipping now. We therefore expect a lot of people will start using it in release versions of these browsers when they are available. Firefox is innovating in this area, by being the first to ship a tool with it. CSS Grid, although really powerful and cool, is also sort of hard to understand and get right. So tooling is essential. So shipping a tool (this bug) when CSS Grid is available in Firefox (in 52) would be really powerful. Also, but I don't know if this makes any difference, only one string was added in this patch: https://hg.mozilla.org/mozilla-central/diff/9611f0e821c2/devtools/shared/locales/en-US/styleinspector.properties

(In reply to Patrick Brosset <:pbro> from comment #36) > Also, but I don't know if this makes any difference, only one string was > added in this patch: Looking at the size of the patch, I can't help wondering what's the current status of this tool. Do you expect other changes to require uplifts for 52?

(In reply to Francesco Lodolo [:flod] from comment #37) > (In reply to Patrick Brosset <:pbro> from comment #36) > > Also, but I don't know if this makes any difference, only one string was > > added in this patch: > > Looking at the size of the patch, I can't help wondering what's the current > status of this tool. There are 2 main steps for this tool. Step 1 is this bug. It only adds an icon next to 'display:grid' in the CSS Rules panel in the Inspector. This is what we're trying to uplift to 52. It's simple but would make all the difference for people testing CSS Grid for the first time. Step 2 is much more ambitious, and we have no plans to ship any of it in 52. Some of it has already been implemented behind a flag, and we will continue to work on it in future releases. Step 2 adds a new sidebar panel that contains much more information about CSS Grid layouts in the page. > Do you expect other changes to require uplifts for 52? There is just one other minor bug: bug 1318019.

OK, I think that's good enough for l10n. The sooner it lands, the better.

(In reply to Julien Cristau [:jcristau] from comment #35) > (In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from > comment #33) > > [String/UUID change made/needed]: Added rule.gridToggle.tooltip string in > > devtools/shared/locales/en-US/styleinspector.properties > > We don't normally take string changes in aurora, per > https://wiki.mozilla.org/Release_Management/Uplift_rules#Aurora_Uplift_. > 28approval-mozilla-aurora.29 so that would need an ok from l10n. Julien, not sure if you're the right person to ping about this, but Francesco has replied (see comment 39) about the l10n concern. I'd really like a decision to be taken quickly here. This is an important change with a lot of excitement for, and we have some marketing prepared for it. So we need to know when it's going to start becoming available to users.

Comment on attachment 8811220 [details] [diff] [review] 1302536.patch [6.0] devtools grid layout highlighter update, l10n change acked by flod, take in aurora52 along with bug 1318019

I think this is covered by https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_grid_layouts, which was done for bug 1282726. So I'm marking this dev-doc-complete, but please let me know if you see anything else needed for this.