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

RESOLVED FIXED in Firefox 53

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(7 attachments, 22 obsolete attachments)

3.66 KB, patch
pbro
: review+
Details | Diff | Splinter Review
4.67 KB, patch
Details | Diff | Splinter Review
5.23 KB, patch
Details | Diff | Splinter Review
8.87 KB, patch
Details | Diff | Splinter Review
3.77 KB, patch
jryans
: review+
Details | Diff | Splinter Review
35.71 KB, patch
Details | Diff | Splinter Review
9.47 KB, patch
Details | Diff | Splinter Review
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.
Depends on: 1308257
Assignee: nobody → gl
Status: NEW → ASSIGNED
Posted patch 1308260.patch [WIP1] (obsolete) — Splinter Review
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?
Attachment #8810294 - Flags: review?(pbrosset)
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
Attachment #8810294 - Flags: review?(pbrosset)
Attachment #8810345 - Flags: review?(pbrosset) → review+
Keywords: leave-open
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
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?
Flags: needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset)
(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 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.
Attachment #8811787 - Flags: review?(jryans) → review+
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.
Attachment #8811800 - Flags: review?(jryans) → review+
Attachment #8810345 - Flags: checkin+
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.
Attachment #8812063 - Flags: review?(pbrosset)
Attachment #8812110 - Flags: review?(pbrosset)
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.
Attachment #8812113 - Flags: review?(pbrosset) → review+
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
Posted patch 1308260-5.patch [WIP1] (obsolete) — Splinter Review
Attachment #8809725 - Attachment is obsolete: true
Depends on: 1318835
Posted patch 1308260.patch [WIP2] (obsolete) — Splinter Review
Attachment #8814171 - Attachment is obsolete: true
Posted patch 1308260.patch [WIP3] (obsolete) — Splinter Review
Attachment #8814306 - Attachment is obsolete: true
Blocks: 1308263
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.
Attachment #8814529 - Flags: review?(pbrosset)
Attachment #8814323 - Attachment is obsolete: true
Attachment #8812089 - Flags: checkin+
Attachment #8812837 - Flags: checkin+
Attachment #8812838 - Flags: checkin+
Attachment #8814529 - Flags: review?(pbrosset)
Posted patch 1308260-7.patch [WIP1] (obsolete) — Splinter Review
Attachment #8814524 - Attachment is obsolete: true
Attachment #8814524 - Flags: review?(jryans)
Attachment #8814656 - Flags: review?(jryans)
Attachment #8814706 - Attachment description: 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. → 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 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.
Attachment #8814725 - Flags: review?(pbrosset) → review+
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.
Attachment #8814656 - Flags: review?(jryans) → review+
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.
Attachment #8814719 - Flags: review?(jryans) → review+
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
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
Pushed by gabriel.luong@gmail.com:
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
Keywords: leave-open
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
Attachment #8815228 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c54418718a5a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.