1342310 - The CSS Grid Inspector should not have to be turned again over and over when refreshing the page

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Jen Simmons [:jensimmons]]

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.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1342310 - WIP: remember selected grid container on navigate

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.

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 5

[Julian Descottes [:jdescottes]]

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

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1342310 - remember selected grid container on navigate;

The highlighters-overlay now keep the state of the grid highlighter
in memory. On "navigate", the state is restored.

Review commit: https://reviewboard.mozilla.org/r/116758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116758/

Comment 7

[Julian Descottes [:jdescottes]]

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

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

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 9

[Gabriel Luong [:gl] (ΦωΦ)]

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);
}

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1342310 - read grid color from highlighters-overlay state;

- emit highlighter options when emitting grid highlighter shown/hidden
events
- read color for highlighted grid container when building initial grids
array in grid-inspector
- explicitly set color when toggling grid highlighter from the rule view

Review commit: https://reviewboard.mozilla.org/r/117642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117642/

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1342310 - preserve grid highlighter only when reloading the page;

Store url in highlighters-overlay grid state to avoid reapplying a state
after navigating on a different page.

Review commit: https://reviewboard.mozilla.org/r/117644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117644/

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8843124 [details]
Bug 1342310 - remember selected grid container on navigate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116758/diff/1-2/

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8843124 [details]
Bug 1342310 - remember selected grid container on navigate;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116758/diff/2-3/

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8844146 [details]
Bug 1342310 - read grid color from highlighters-overlay state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117642/diff/1-2/

Comment 15

[Julian Descottes [:jdescottes]]

Comment on attachment 8844147 [details]
Bug 1342310 - preserve grid highlighter only when reloading the page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117644/diff/1-2/

Comment 16

[Julian Descottes [:jdescottes]]

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

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

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 18

[Gabriel Luong [:gl] (ΦωΦ)]

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 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8844146 [details]
Bug 1342310 - read grid color from highlighters-overlay state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117642/diff/2-3/

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8844147 [details]
Bug 1342310 - preserve grid highlighter only when reloading the page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117644/diff/2-3/

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8844146 [details]
Bug 1342310 - read grid color from highlighters-overlay state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117642/diff/3-4/

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8844147 [details]
Bug 1342310 - preserve grid highlighter only when reloading the page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117644/diff/3-4/

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

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 24

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8844147 [details]
Bug 1342310 - preserve grid highlighter only when reloading the page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117644/diff/4-5/

Comment 26

[Julian Descottes [:jdescottes]]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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