Imagine I'm a developer/designer working in Dev Tools, using the Grid Inspector. I see the lines and am rewriting my CSS. Once I've saved my CSS, I refresh the browser in order to see the results of what I just wrote. When I refresh, the Grid Inspector lines disappear. To make them reappear, I have to go click on the little icon again. And again. And again. And again. When we created the Grid Inspector Add On, this is how that tool behaved at first as well. We did user research, and discovered that it's actually really annoying for authors to have to keep turning the lines back on. Potch and I changed the Add-on so that it captured state, and kept the Grid lines on even upon refresh. Every time an author refreshed, the Grid Inspector stayed visible. Our test users were very happy about the change. It made for a much better workflow, and a much more satisfying tool. I'd like to make the same change to our DevTools version.

Needinfo'ing you Matteo, I think you were the one that originally fixed this, correct me if I am wrong.

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #1) > Needinfo'ing you Matteo, I think you were the one that originally fixed > this, correct me if I am wrong. I fixed the bfcache, but not the behavior for the navigation, I think this was implemented by the original writer of the css grid highlighter (Patrick, I guess): by default all our highlighters are hidden or destroyed during the navigation, to avoid issues; so this is expected. If we are going to change this, we should probably do that in a deeper level, and therefore for all our highlighters, and trap only the reload of the page. This would be a nice enhancement, but it would be easier once we refactored the highlighters using redux to keep the state IMVHO.

Matteo: can I get your feedback on this approach ? Instead of persisting the highlighter we simply keep a reference on the client to the CSS selector of the last grid that was highlighted (similar to what we do to automatically reselect the same node in the markup view). Persisting the information in the grid highlighter directly could be interesting, but we would still need the state to be up to date on the client (ie highlighters.gridHighlighterShown should still be correct). The patch is really just a prototype, I'm really not set on saving this information in the highlighters-overlay or in the inspector panel.

Just wanted to say we should try to make highlighters-overlay keep track of the state as if it was a redux store.

I agree with this, it still means keeping the state on the client (which is what I do here).

try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cee9316036028d26b1c559caf06ee169114dba1c

Comment on attachment 8842411 [details] [diff] [review] Bug 1342310 - WIP: remember selected grid container on navigate Sorry for the late feedback! I checked the latest patch you added. I think this has approach is a bit flawed. I think we should first of all ensure that we're doing that only on reloading the same document instead of general navigation. Otherwise we could end up to open the highlighters automatically every time for different pages too. Where that could be an interesting feature to add, I don't think it should be the default behavior. To be more clear: 1. Go to http://gridbyexample.com/examples/code/example11.html 2. Enable the grid on <div class="wrapper"> 3. Go to http://gridbyexample.com/examples/code/example12.html 4. The highlighter is on here too already for a different grid Additional steps that could confuse the user: 5. Press the "back" button 6. http://gridbyexample.com/examples/code/example11.html has the highlighter enabled (and could make sense, since the user left this page with the highlighter on) 7. Press the "forward" or navigate in some other examples 8. The highlighter is still on, as said in point 4 9. Turn the highlighter off 10. Press the "back" button 11. Now the http://gridbyexample.com/examples/code/example11.html has the highlighter disabled The things become complex if we have a page with frames that contains the same selector too: 1. Go to https://zer0.github.io/devtools/highlighters-test-page.html 2. Scroll down the Playground area at the left, until you find the Framed ones. 3. Select the grid (either from the new layout panel, so the 2nd one, or by Rules panel once selected) 4. Reload the page The first grid in the main document will be selected, instead of the one in the frame. Where I totally agree that it's a nice feature, and those issues are maybe not too common, I also think that if we know in advance that something is buggy and could be seen as "unpolished" from our users, their impression won't be so good. Since this is not a bug fix, but an enhancement, and introduces something we need to address soon or later, I would rather avoid to land as is; so addressing the issues above or postpone the patch until we have time to work on them. That's just to explain why my f-, Julian, I think the direction it's the good ones!

Comment on attachment 8843124 [details] Bug 1342310 - remember selected grid container on navigate; https://reviewboard.mozilla.org/r/116758/#review118734 ::: devtools/client/inspector/rules/test/browser_rules_grid-highlighter-restored-after-reload.js:7 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test that the grid highlighter is re-displayed after reloading a page s/Test/Tests ::: devtools/client/inspector/rules/test/browser_rules_grid-highlighter-restored-after-reload.js:7 (Diff revision 1) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test that the grid highlighter is re-displayed after reloading a page Add a period at the end ::: devtools/client/inspector/shared/highlighters-overlay.js:75 (Diff revision 1) > el.addEventListener("mousemove", this.onMouseMove); > el.addEventListener("mouseout", this.onMouseOut); > el.ownerDocument.defaultView.addEventListener("mouseout", this.onMouseOut); > > this.inspector.target.on("will-navigate", this.onWillNavigate); > + this.inspector.target.on("navigate", this.onNavigate); Move this above "will-navigate" ::: devtools/client/inspector/shared/highlighters-overlay.js:97 (Diff revision 1) > el.removeEventListener("click", this.onClick, true); > el.removeEventListener("mousemove", this.onMouseMove); > el.removeEventListener("mouseout", this.onMouseOut); > > this.inspector.target.off("will-navigate", this.onWillNavigate); > + this.inspector.target.off("navigate", this.onNavigate); Same as above. ::: devtools/client/inspector/shared/highlighters-overlay.js:175 (Diff revision 1) > + }), > + > + /** > + * Restore the saved highlighter states. > + * > + * @returns {Promise} that resolves when the highlighter state was restored, and the s/@returns/@return ::: devtools/client/inspector/shared/highlighters-overlay.js:191 (Diff revision 1) > + yield this.onInspectorNewRoot; > + > + let walker = this.inspector.walker; > + let rootNode = yield walker.getRootNode(); > + let nodeFront = yield walker.querySelector(rootNode, selector); > + if (nodeFront) { Add a new line before the if statement. ::: devtools/client/inspector/shared/highlighters-overlay.js:217 (Diff revision 1) > return highlighter; > }); > }, > > + _handleRejection: function (error) { > + if (this.destroyed) { We can just make this: if (!this.destroyed) { console.error(errro); }

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6fade1c29386d31aa7bf85a330f4505773022b6

Comment on attachment 8843124 [details] Bug 1342310 - remember selected grid container on navigate; https://reviewboard.mozilla.org/r/116758/#review119932 ::: devtools/client/inspector/shared/highlighters-overlay.js:138 (Diff revision 3) > return; > } > > this._toggleRuleViewGridIcon(node, true); > > + node.getUniqueSelector().then(selector => { Since we use Task.async here. We can probably just call yield node.getUniqueSelector() instead. We should try to then use promises.then() in this case.

Comment on attachment 8844146 [details] Bug 1342310 - read grid color from highlighters-overlay state; https://reviewboard.mozilla.org/r/117642/#review119936 ::: devtools/client/inspector/grids/actions/grids.js:41 (Diff revision 2) > * @param {Boolean} highlighted > * Whether or not the grid highlighter is highlighting the grid. > + * @param {String} color > + * The color to use for this nodeFront's grid highlighter. > */ > - updateGridHighlighted(nodeFront, highlighted) { > + updateGridHighlighted(nodeFront, highlighted, color) { The function name would be wrong given the current context here of what we are updating in the grid's state. I think I would rather see 2 dispatches one to update the highlighted state and one to update the color. ::: devtools/client/inspector/grids/grid-inspector.js:228 (Diff revision 2) > let grid = gridFronts[i]; > let nodeFront = yield this.walker.getNodeFromActor(grid.actorID, ["containerEl"]); > > - let fallbackColor = GRID_COLORS[i % GRID_COLORS.length]; > - let color = this.getGridColorForNodeFront(nodeFront) || fallbackColor; > + let highlighted = nodeFront == this.highlighters.gridHighlighterShown; > + > + let color; Let's create a getGridColor(highlighted) function that returns the grid color from one of these 3 options that we have know since updateGridPanel is getting fairly complex now. ::: devtools/client/inspector/grids/grid-inspector.js:280 (Diff revision 2) > + * The highlighter options used for the highlighter being shown/hidden. > */ > - onHighlighterChange(event, nodeFront) { > + onHighlighterChange(event, nodeFront, options) { > let highlighted = event === "grid-highlighter-shown"; > - this.store.dispatch(updateGridHighlighted(nodeFront, highlighted)); > + let { color } = options; > + this.store.dispatch(updateGridHighlighted(nodeFront, highlighted, color)); We should make 2 dispatch calls one to updateGridHighlighted and one to updateGridColor. ::: devtools/client/inspector/shared/highlighters-overlay.js:13 (Diff revision 2) > > const promise = require("promise"); > const {Task} = require("devtools/shared/task"); > const EventEmitter = require("devtools/shared/event-emitter"); > const { VIEW_NODE_VALUE_TYPE } = require("devtools/client/inspector/shared/node-types"); > +const DEFAULT_GRID_COLOR = "#4B0082"; Add a new line to separate requires and constant values.

Comment on attachment 8844147 [details] Bug 1342310 - preserve grid highlighter only when reloading the page; https://reviewboard.mozilla.org/r/117644/#review120512 ::: devtools/client/inspector/shared/highlighters-overlay.js:141 (Diff revision 4) > this._toggleRuleViewGridIcon(node, true); > > - node.getUniqueSelector().then(selector => { > + try { > + let selector = yield node.getUniqueSelector(); > + > // Save grid highlighter state. Nit: we should probably move this comment after the `let { url }…` and before `this.state.gid`, or put `let selector` after this comment and before `let { url }`. ::: devtools/client/inspector/shared/highlighters-overlay.js:385 (Diff revision 4) > - }, > + yield this.restoreState(); > + } catch (e) { > + this._handleRejection(e); > + } > + // Used by tests. > + this.emit("highlighters-overlay-navigated"); I don't like too much having this event there specifically for the tests, plus the name itself doesn't make too much sense to me. Would it works as well, for the tests, if we `emit` an event once we actually restore the state? Too me seems a more natural thing, and it could be used not only from the tests or the grid highlighter.

Comment on attachment 8844147 [details] Bug 1342310 - preserve grid highlighter only when reloading the page; https://reviewboard.mozilla.org/r/117644/#review120512 Forgot to add the overall comment: Looks good to me overall, there are a couple of things maybe to address – I would like to have a better event as mentioned, but I wouldn't stop landing for that if you disagree, Julian.

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ceef1c3164ca remember selected grid container on navigate;r=gl https://hg.mozilla.org/integration/autoland/rev/a43a98cc70bd read grid color from highlighters-overlay state;r=gl https://hg.mozilla.org/integration/autoland/rev/66227bcf53be preserve grid highlighter only when reloading the page;r=zer0

https://hg.mozilla.org/mozilla-central/rev/ceef1c3164ca https://hg.mozilla.org/mozilla-central/rev/a43a98cc70bd https://hg.mozilla.org/mozilla-central/rev/66227bcf53be