1308260 - Display a list of grid container elements in the css grid layout panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

Adds a checkbox list of the grid containers for the current page in the css grid layout and creates a new css grid highlighter when a grid container is checked. We won't worry about having different colours for each highlighter for this bug.

Comment 1

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

Attachment: 1308260.patch [WIP1]

Comment 2

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

Attachment: Part 1: Emit an event with the new grid layout actors on page navigations from the LayoutActor [1.0]

Breaking down my current big WIP patch into reviewable and landable pieces. Patrick, can you comment on what is the best way to go about filtering out the reflows? I saw that for nodes we have a wasDisplayed attribute that we check when the display visibility is changed. Should we be doing the same thing here when checking for reflows?

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8810294 [details] [diff] [review]
Part 1: Emit an event with the new grid layout actors on page navigations from the LayoutActor [1.0]

Review of attachment 8810294 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/layout.js
@@ +72,5 @@
>      this.tabActor = tabActor;
>      this.walker = walker;
> +
> +    this.onNavigate = this.onNavigate.bind(this);
> +    this.onReflows = this.onReflows.bind(this);

This callback isn't defined or used in the code that you changed in this patch.

@@ +84,3 @@
>      this.tabActor = null;
>      this.walker = null;
> +    this.layoutChangeObserver = null;

this.layoutChangeObserver does not seem to exist in this file. Either this needs to be removed, or you forgot to import and use it.

@@ +131,5 @@
>        grids = [...grids, ...this.getGrids(document.documentElement)];
>      }
>      return grids;
>    },
> +  onNavigate: function () {

Splinter says that there's no new line between the getAllGrids method and the onNavigate method. I'm sure there is one because this already happened in other patches I reviewed for you in the past, but maybe your editor is configured to enter new lines in a way that splinter doesn't recognizes them.
We recently made a change to our .eslintrc config to enforce unix-style line breaks: http://searchfox.org/mozilla-central/source/devtools/.eslintrc.js#148-149

Comment 4

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

Attachment: Part 1: Emit an event with the new grid layout actors on page navigations from the LayoutActor [2.0]

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fef1eecca5bf6e3afa3b379974c4fe14c0342289
Bug 1308260 - Part 1: Emit an event with the new grid layout actors on page navigations from the LayoutActor. r=pbro

Comment 6

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

Patrick, can you comment on what is the best way to go about filtering out the reflows? I saw that for nodes we have a wasDisplayed attribute that we check when the display visibility is changed. Should we be doing the same thing here when checking for reflows?

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/fef1eecca5bf

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #6)
> Patrick, can you comment on what is the best way to go about filtering out
> the reflows? I saw that for nodes we have a wasDisplayed attribute that we
> check when the display visibility is changed. Should we be doing the same
> thing here when checking for reflows?
There is a important difference between the 2 things. In the Walker, we only care about the nodes we know about to check whether their visibility has changed on reflow, because only the nodes we know about in the Walker are visible in the inspector front-end.

In contrast, the grid layout sidebar panel will display a list of nodes that may or may not be known yet. So we have no choice but to actually walk the DOM to find out the whole list of grids.

So, this is down to a performance optimization problem. Here, what we're saying is that we want to update the complete list of grids anytime that list changes in the page.

Our immediate solution is to listen to reflows (of which there could be many), and when that happens, walk the whole DOM (which could be deep), and on each node run getGridFragments() to test if a grid exist (which could be slow).

For the first problem (many reflows), I think we should debounce or throttle execution of the callback to avoid unnecessary work.

For the second problem (deep DOM), I don't see any way around this. I'm assuming that the treeWalker we use is the fastest thing we can use, and I don't see any way of filtering here. Obviously, if a node is inline or hidden, there's no need to walk down within it, but getting the node's computed style might make things slower than just walking its descendants. We should test this though.
One small optimization though, when calling getDocumentWalker, we should pass a second argument set to SHOW_ELEMENT from dom-node-filter-constants.js so at least the walker only goes into element nodes.

For the last problem (calling getGridFragments), my assumption so far was that this was really fast. The platform code seems to firs test the type of the first frame of the element. So I don't think there's any way to make this faster.

Comment 9

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

Attachment: Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be disaptched. [1.0]

Comment 10

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

Attachment: Part 3: Dispatch the new grid data and update the store with the new grid state.

Comment 11

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

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #10)
> Created attachment 8811787 [details] [diff] [review]
> Part 3: Dispatch the new grid data and update the store with the new grid
> state.

@jryans, main concern here is the duplication of enum.js from rdm in devtools/client/inspector/layout/utils. 
Otherwise, it is a very simple redux state update that can be reviewed by someone else if you don't have time. Feel free to pass the review to pbro or zer0 if that is the case.

Comment 12

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

Attachment: Part 4: Pass the grids props into the layout components and only display the layout-no-grid element when no grids are present. [1.0]

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8811787 [details] [diff] [review]
Part 3: Dispatch the new grid data and update the store with the new grid state.

Review of attachment 8811787 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/layout/utils/enum.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I'd say move the RDM one to devtools/client/shared/enum.js.

Seems unnecessary to have two copies.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8811800 [details] [diff] [review]
Part 4: Pass the grids props into the layout components and only display the layout-no-grid element when no grids are present. [1.0]

Review of attachment 8811800 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/layout/components/App.js
@@ -10,1 @@
>  const App = createClass({

Consider adding

mixins: [ addons.PureRenderMixin ],

to all components that only change when props change to improve perf.

Comment 15

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

Attachment: Part 3: Dispatch the new grid data and update the store with the new grid state. [2.0]

Comment 16

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

Attachment: Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [2.0]

Comment 17

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

Attachment: Part 4: Pass the grids props into the layout components and only display the layout-no-grid element when no grids are present. [2.0]

Comment 18

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

Attachment: Part 4: Pass the grids props into the layout components and only display the layout-no-grid element when no grids are present. [3.0]

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8812063 [details] [diff] [review]
Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [2.0]

Review of attachment 8812063 [details] [diff] [review]:
-----------------------------------------------------------------

Generally OK with this patch, but would like to maybe move the layout inspector construction to init.

::: devtools/client/inspector/layout/layout.js
@@ +101,5 @@
> +        console.error(e);
> +      }
> +
> +      grids.push({
> +        id: i,

Can you explain why an ID is needed here? Maybe that's for rendering a list with React later?
If it's not needed then you could use
for (let grid of gridFronts) above instead of defining i

@@ +129,5 @@
> +   * visible. Otherwise, stop listening for grid layout changes.
> +   */
> +  onSidebarSelect: Task.async(function* () {
> +    if (!this.layoutInspector) {
> +      this.layoutInspector = yield this.inspector.walker.getLayoutInspector();

I find it a little odd that the layout inspector would be created in this function. It shouldn't really have to deal with this. This is something init should really be doing instead.
I think you should move this to init, and after its done, call onSidebarSelect instead of calling it from the constructor.

Comment 20

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

Attachment: Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [3.0]

Comment 21

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

Attachment: Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [4.0]

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8812113 [details] [diff] [review]
Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [4.0]

Review of attachment 8812113 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few more comments, but don't need to re-review once you've addressed them.

::: devtools/client/inspector/layout/layout.js
@@ +32,5 @@
>  
>  LayoutView.prototype = {
>  
> +  init: Task.async(function* () {
> +    this.layoutInspector = yield this.inspector.walker.getLayoutInspector();

Since this is async, there might be cases where destroy was called before this resolved.
So below that line, you have no guaranty that the LayoutView (or the toolbox for that matter) still exists.
So after this line, you should test this.inspector and if falsy, then bail out.

@@ +60,1 @@
>      this.inspector = null;

You also need to nullify this.layoutInspector here.

@@ +88,5 @@
> +    // Get all the GridFront from the server if no gridFronts were provided.
> +    if (!gridFronts) {
> +      gridFronts = yield this.layoutInspector.getAllGrids(this.walker.rootNode);
> +    }
> +

Unfortunately, the fact that many things are async here (and in our tools in general) leads to races hiding in every corner. Here, refresh could be called as a result of a grid change on the server, but that grid change could happen while the toolbox is being closed. So there is a possibility where you're reaching this line but the panel has been destroyed already. So you'll also need to bail out here.

@@ +98,5 @@
> +      // Get the NodeFront of the grid container element.
> +      try {
> +        nodeFront = yield this.walker.getNodeFromActor(grid.actorID, ["containerEl"]);
> +      } catch (e) {
> +        console.error(e);

Why catch the error if all you're doing is raising it again with console.error?

Is there a case where calling getNodeFromActor may fail but we do want to continue?
If there is, then you should have:
try { ... } catch (e) { continue; }
instead, so you just skip this grid.

If there isn't, then just remove the try/catch altogether.

Comment 23

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

Attachment: Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. [5.0]

Comment 24

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

Attachment: Part 3: Dispatch the new grid data and update the store with the new grid state. [3.0]

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8423ee15c2f0d080a152454082863d8246d33adc
Bug 1308260 - Part 2: Handle grid layout changes and layout sidebar selection, and prepares the grid data to be dispatched. r=pbro

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca96b9d71ca33da24f09310c8df84125dbacd1b9
Bug 1308260 - Part 3: Dispatch the new grid data and update the store with the new grid state. r=jryans

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5541adf77a4eeb1f237828e1bd6b8076bafd1f3
Bug 1308260 - Part 4: Pass the grids props into the layout components and only display the layout-no-grid element when no grids are present. r=jryans

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8423ee15c2f0
https://hg.mozilla.org/mozilla-central/rev/ca96b9d71ca3
https://hg.mozilla.org/mozilla-central/rev/b5541adf77a4

Comment 27

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

Attachment: 1308260-5.patch [WIP1]

Comment 28

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

Attachment: 1308260.patch [WIP2]

Comment 29

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

Attachment: 1308260.patch [WIP3]

Comment 30

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

Attachment: Part 5: Add a highlighted state to the grid state and a method to update it. [1.0]

Comment 31

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [1.0]

Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state.

- Rearranged the order of the functions in the highlighters-overlay.js.
- Rewrote the grid highlighter functions using Task.
- _toggleRuleViewGridIcon does not need to be called unless the actual inspector selection is the actual grid container element since we would be able to highlight different grid containers from the LayoutView.
- Separated the grid highlighter show and hide functions so it can be called individually in different situations such as multiple overlay or from the text property editor edits.

Comment 32

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

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

Comment 33

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

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

Comment 34

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [2.0]

Comment 35

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

Attachment: 1308260-7.patch [WIP1]

Comment 36

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [3.0]

Comment 37

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

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

Comment 38

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

Attachment: Part 5: Add a highlighted state to the grid state and a method to update it. [2.0]

Comment 39

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [4.0]

Comment 40

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

Attachment: Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [1.0]

Comment 41

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

Attachment: Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [2.0]

Comment 42

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [5.0]

Comment 43

[Patrick Brosset <:pbro>]

Comment on attachment 8814725 [details] [diff] [review]
Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [5.0]

Review of attachment 8814725 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/layout/layout.js
@@ +151,5 @@
> +   *         The NodeFront of the grid container element for which the grid highlighter
> +   *         is shown for.
> +   */
> +  onHighlighterChange(event, nodeFront) {
> +    let highlighted = event === "grid-highlighter-shown" ? true : false;

The === already evaluates to a boolean, so this can be simply:

let highlighted = event === "grid-hgihighter-shown";

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +159,5 @@
> +   * @param  {String} type
> +   *         The highlighter type. One of this.highlighters.
> +   * @return {Promise} that resolves to the highlighter
> +   */
> +  _getHighlighter: function (type) {

While you're at it, you could also make this one a Task.async.

Comment 44

[Patrick Brosset <:pbro>]

Comment on attachment 8814719 [details] [diff] [review]
Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [2.0]

Review of attachment 8814719 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/layout/components/GridList.js
@@ +54,5 @@
> +          let gridName = displayName;
> +
> +          let idIndex = attributes.findIndex(({ name }) => name === "id");
> +          if (idIndex > -1 && attributes[idIndex].value) {
> +            gridName += "#" + attributes[idIndex].value;

Drive-by comment (as discussed on IRC): this is duplicated code from a few different places in DevTools:
- the markup-view has some code to preview DOM nodes
- the new console uses a Rep for this: devtools\client\shared\components\reps\element-node.js
- the animationinspector has a reusable component for this too: devtools\client\inspector\shared\dom-node-preview.js
- and the variable-views also has some code for this
So, while this is fine as a stopgap to start landing these patches, I'd like one of the your next parts or bugs to be about reusing one of these shareable components.
We should really be using the Rep everywhere we can. It's a good fit here because it's React.

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8814719 [details] [diff] [review]
Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [2.0]

Review of attachment 8814719 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please file a follow up for :pbro's comments.

Comment 46

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8814719 [details] [diff] [review]
Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [2.0]

Review of attachment 8814719 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but please file a follow up for :pbro's comments.

::: devtools/client/inspector/layout/components/GridList.js
@@ +63,5 @@
> +            gridName += "." + attributes[classIndex].value.split(" ").join(".");
> +          }
> +
> +          return dom.li(
> +            {},

You should set a `key` attribute for better vdom diffing performance with lists.

https://facebook.github.io/react/docs/lists-and-keys.html

Comment 47

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c54418718a5a6f41f60992a896e301cf1664270d
Bug 1308260 - Part 5: Add a highlighted state to the grid state and a method to update it. r=jryans

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54418718a5a
Part 5: Add a highlighted state to the grid state and a method to update it. r=jryans

Comment 49

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

Attachment: Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. [6.0]

Comment 50

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

Attachment: Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. [3.0]

Comment 51

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/892a0318ffbe197aa1342c93b5ff78715fab2a7b
Bug 1308260 - Part 6: Toggling the grid highlighter should emit the node that the grid highlighter was shown or hidden for and update the grid's highlighted state. r=pbro

https://hg.mozilla.org/integration/mozilla-inbound/rev/7055689115a4ba815bdfca60af0cf1958939001b
Bug 1308260 - Part 7: Display a list of grid container elements and checkbox to toggle the grid highlighter for each grid in the CSS grid layout panel. r=jryans

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c54418718a5a

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/892a0318ffbe
https://hg.mozilla.org/mozilla-central/rev/7055689115a4