1431071 - Suggest line and area names in the autocomplete for grid-row/column[-start/end] and grid-area

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

There's one missing thing to our grid highlighter, and that's grid line names. Nowhere in the tool are they visible. Knowing that line names are one of the important positioning system in grid, it's a problem that we'd like to fix soon.
In bug 1359875, we're exploring ideas for displaying them somewhere in the page.

Here, I'd like to suggest another approach (not instead of bug 1359875, on top of it):

when the autocomplete popup appears for one of the grid positioning properties, then suggest line names (and area names for grid-area).

- for grid-row, grid-row-start and grid-row-end: populate the list with all of the row line names found in the parent grid container, as well as all of the grid area names found there too.

- for grid-column, grid-column-start and grid-column-end: populate the list with all of the column line names found in the parent grid container, as well as all of the grid area names found there too.

- for grid-area: populate the list with all of the area names found on the parent grid container.

This would make it a lot easier to position items onto the grid.

Comment 1

[Erica Wright [:ewright]]

Attachment: Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 2

[Erica Wright [:ewright]]

:gl As we talked about I still need to write tests. 

If I understand correctly "grid-area" is actually short hand for all the others. I have not brought in the "area" names only the column and row names.

Comment 3

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review239894


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/shared/inplace-editor.js:299
(Diff revision 2)
>    if (typeof options.selectAll == "undefined" || options.selectAll) {
>      this.input.select();
>    }
>  
> -  if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input.value == "") {
> +  if (["grid-area", "grid-row", "grid-row-start", "grid-row-end",
> +      "grid-column", "grid-column-start", "grid-column-end"]

Error: Expected indentation of 7 spaces but found 6. [eslint: indent-legacy]

::: devtools/client/shared/inplace-editor.js:304
(Diff revision 2)
> +      "grid-column", "grid-column-start", "grid-column-end"]
> +       .includes(this.property.name)) {
> +    options.gridLineNames()
> +    .then((gridLineNames) => {
> +      this.gridLineNames = gridLineNames;
> +      if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input && this.input.value == "") {

Error: Line 304 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/shared/inplace-editor.js:309
(Diff revision 2)
> +      if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input && this.input.value == "") {
> -    this._maybeSuggestCompletion(false);
> +        this._maybeSuggestCompletion(false);
> -  }
> +      }
> +    });
> +  } else {
> +    if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input && this.input.value == "") {

Error: Line 309 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/shared/inplace-editor.js:309
(Diff revision 2)
> +      if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input && this.input.value == "") {
> -    this._maybeSuggestCompletion(false);
> +        this._maybeSuggestCompletion(false);
> -  }
> +      }
> +    });
> +  } else {
> +    if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input && this.input.value == "") {

Error: Unexpected if as the only statement in an else block. [eslint: no-lonely-if]

Comment 5

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

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review239948

::: devtools/client/inspector/rules/views/text-property-editor.js:330
(Diff revision 2)
>        this.ruleView.highlighters.on("hover-shape-point", this._onHoverShapePoint);
>      }
>    },
>  
> +  getGridlineNames: function() {
> +    console.log("calling gridlines");

Remove console.log

::: devtools/client/inspector/rules/views/text-property-editor.js:334
(Diff revision 2)
> +  getGridlineNames: function() {
> +    console.log("calling gridlines");
> +    let gridLineNames = {cols: [], rows: []};
> +    // Get the one grid that the current selected element is contained in.
> +    return this.ruleView.inspector.walker.getLayoutInspector()
> +      .then((layoutInspector) => {

Can remove the () around layoutInspector since this is a single param

::: devtools/client/inspector/rules/views/text-property-editor.js:337
(Diff revision 2)
> +    // Get the one grid that the current selected element is contained in.
> +    return this.ruleView.inspector.walker.getLayoutInspector()
> +      .then((layoutInspector) => {
> +        return layoutInspector.getCurrentGrid(
> +          this.ruleView.inspector.selection.nodeFront);
> +      }).then((gridFront) => {

Same as above

::: devtools/client/inspector/rules/views/text-property-editor.js:340
(Diff revision 2)
> +        return layoutInspector.getCurrentGrid(
> +          this.ruleView.inspector.selection.nodeFront);
> +      }).then((gridFront) => {
> +        if (gridFront) {
> +          let gridFragments = gridFront.gridFragments;
> +          gridFragments.forEach((grid, i) => {

Use for (let gridFragment of gridFragments) {}

::: devtools/client/shared/inplace-editor.js:298
(Diff revision 2)
>  
>    if (typeof options.selectAll == "undefined" || options.selectAll) {
>      this.input.select();
>    }
>  
> -  if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input.value == "") {
> +  if (["grid-area", "grid-row", "grid-row-start", "grid-row-end",

Let's make a const for this array at the top of the file called GRID_PROPERTY_NAMES

::: devtools/server/actors/layout.js:135
(Diff revision 2)
>    initialize(conn, tabActor, walker) {
>      Actor.prototype.initialize.call(this, conn);
>  
>      this.tabActor = tabActor;
>      this.walker = walker;
> +    this.getCurrentDisplay = this.getCurrentDisplay.bind(this);

Does this need to be binded?

::: devtools/server/actors/layout.js:145
(Diff revision 2)
>  
>      this.tabActor = null;
>      this.walker = null;
>    },
>  
> -  /**
> +  getCurrentDisplay(node, typeToCheck) {

s/typeToCheck/type

::: devtools/server/actors/layout.js:145
(Diff revision 2)
>  
>      this.tabActor = null;
>      this.walker = null;
>    },
>  
> -  /**
> +  getCurrentDisplay(node, typeToCheck) {

Add a JSDoc for this function. It should mention that  this is a helper function to getCurrentFlexbox and getCurrentGrid

Comment 6

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 7

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

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

@jdescottes, can you review the inplace-editor/autocomplete portions?

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review240502

Thanks for the patch. As asked by Gabriel I mostly looked at the inplace-editor / text-property-editor changes. 
I have few suggestions, mostly concerned about introducing async in the InplaceEditor constructor. Otherwise it looks good!

::: devtools/client/inspector/rules/views/text-property-editor.js:322
(Diff revision 3)
>          popup: this.popup,
>          multiline: true,
>          maxWidth: () => this.container.getBoundingClientRect().width,
>          cssProperties: this.cssProperties,
>          cssVariables: this.rule.elementStyle.variables,
> +        gridLineNames: this.getGridlineNames,

Let's rename this new parameter to getGridLineNames to communicate that it is a method.

Could we also document it in the jsdoc in inplace-editor.js above editableField()

::: devtools/client/inspector/rules/views/text-property-editor.js:329
(Diff revision 3)
>  
>        this.ruleView.highlighters.on("hover-shape-point", this._onHoverShapePoint);
>      }
>    },
>  
> +  getGridlineNames: function() {

Could we rewrite this with async/await?

::: devtools/client/inspector/rules/views/text-property-editor.js:339
(Diff revision 3)
> +          for (let gridFragment of gridFragments) {
> +            for (let j = 0; j < gridFragment.rows.lines.length; j++) {
> +              gridLineNames.rows = gridLineNames.rows
> +                .concat(gridFragment.rows.lines[j].names);
> +            }
> +            for (let k = 0; k < gridFragment.cols.lines.length; k++) {
> +              gridLineNames.cols = gridLineNames.cols
> +                .concat(gridFragment.cols.lines[k].names);
> +            }
> +          }

For readability, could we replace the for (let j/k = 0) with for (let rowLine/colLine of ...). Alternatively you could also use forEach or reduce to avoid nesting loops.

::: devtools/client/shared/inplace-editor.js:53
(Diff revision 3)
>  
>  const WORD_REGEXP = /\w/;
>  const isWordChar = function(str) {
>    return str && WORD_REGEXP.test(str);
>  };
> +const GRID_PROPERTY_NAMES = ["grid-area", "grid-row", "grid-row-start",

Not ideal, but I think for maintenance it will be easier to also have const for the sub arrays:

const GRID_ROW_PROPERTY_NAMES = ["grid-area", "grid-row", "grid-row-start", "grid-row-end"];
const GRID_COL_PROPERTY_NAMES = ...

::: devtools/client/shared/inplace-editor.js:301
(Diff revision 3)
>  
>    if (typeof options.selectAll == "undefined" || options.selectAll) {
>      this.input.select();
>    }
>  
> -  if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input.value == "") {
> +  if (this.property && GRID_PROPERTY_NAMES.includes(this.property.name)) {

This implementation changes the timing of the call to _maybeSuggestCompletion depending on the options of the editor. If gridLineNames is defined, then we call it asynchronously after the constructor finishes. If not, it is called synchronously in the constructor. 

We should try to keep this consistent. If it's ok to initialize this and call maybeSuggestCompletion after everything else, then let's move this if block to an async method and call it at the end of the constructor:

  if (this.property && GRID_PROPERTY_NAMES.includes(this.property.name)) {
    this.gridLinesNames = await this.getGridLineNames();
  }
  
  if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input &&
    this.input.value == "") {
    this._maybeSuggestCompletion(false);
  }

Or you could modify `function editableField(options)` to retrieve the gridLineNames right before calling `new InplaceEditor`. This way we could keep the constructor fully synchronous.

::: devtools/client/shared/inplace-editor.js:1384
(Diff revision 3)
> +        if (["grid-area", "grid-row", "grid-row-start", "grid-row-end"]
> +             .includes(this.property.name)) {
> +          list.push.apply(list, this.gridLineNames.rows);
> +        }
> +        if (["grid-area", "grid-column", "grid-column-start", "grid-column-end"]
> +             .includes(this.property.name)) {
> +          list.push.apply(list, this.gridLineNames.cols);
> +        }

Depending on how you address my previous comment this could change a bit, but right now, nothing guarantees that this.gridLineNames is defined (eg. the async call might have failed silently). If there is still a risk that gridLineNames is not initialized, we should gard against that.

::: devtools/client/shared/inplace-editor.js:1384
(Diff revision 3)
>          } else {
>            list = ["!important",
>                    ...this._getCSSValuesForPropertyName(this.property.name)];
>          }
>  
> +        if (["grid-area", "grid-row", "grid-row-start", "grid-row-end"]

Could we move both ifs directly in _getCSSValuesForPropertyName ?

::: devtools/client/shared/inplace-editor.js:1393
(Diff revision 3)
> +        if (["grid-area", "grid-column", "grid-column-start", "grid-column-end"]
> +             .includes(this.property.name)) {
> +          list.push.apply(list, this.gridLineNames.cols);
> +        }
> +        // We need to sort again after adding gridLineNames.
> +        list.sort();

The sort happens at the end of this method, is this necessary?

Comment 9

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review240502

> This implementation changes the timing of the call to _maybeSuggestCompletion depending on the options of the editor. If gridLineNames is defined, then we call it asynchronously after the constructor finishes. If not, it is called synchronously in the constructor. 
> 
> We should try to keep this consistent. If it's ok to initialize this and call maybeSuggestCompletion after everything else, then let's move this if block to an async method and call it at the end of the constructor:
> 
>   if (this.property && GRID_PROPERTY_NAMES.includes(this.property.name)) {
>     this.gridLinesNames = await this.getGridLineNames();
>   }
>   
>   if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input &&
>     this.input.value == "") {
>     this._maybeSuggestCompletion(false);
>   }
> 
> Or you could modify `function editableField(options)` to retrieve the gridLineNames right before calling `new InplaceEditor`. This way we could keep the constructor fully synchronous.

This one has proven to be difficult. I tried calling it just before `new InplaceEditor(options, event);` like: `options.getGridLineNames().then(gridLineNames => {new InplaceEditor(options, event, gridLineNames);}` but making that function async causes a lot of problems.

I moved it to an async method and moved it to the bottom.

> The sort happens at the end of this method, is this necessary?

The check on line 1464 assumes the list is alphabetically sorted. It checks if the query is earlier in the alphabet than the next result and if so, quits. If I don't sort the list then the results stay at the bottom and often end up being cut before the final list is sorted.

Comment 10

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review240910

Looks good to me, thanks Erica.

::: devtools/client/shared/inplace-editor.js:328
(Diff revision 4)
>  
>    if (options.start) {
>      options.start(this, event);
>    }
> +
> +  this._getGridNamesBeforeCompletion(options.getGridLineNames);

Could we document this new option in the jsdoc for function editableField() ?

::: devtools/client/shared/inplace-editor.js:1603
(Diff revision 4)
> +      }
> +      if (GRID_COL_PROPERTY_NAMES.includes(this.property.name)) {
> +        gridLineList.push(...this.gridLineNames.cols);
> +      }
> +    }
> +    return gridLineList.concat(this.cssProperties.getValues(propertyName)).sort();

Can we add a comment explaining why this needs to be sorted?

Comment 12

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 13

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

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

https://reviewboard.mozilla.org/r/231662/#review241536

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:17
(Diff revision 4)
> +//    is the popup open,
> +//    is a suggestion selected in the popup,
> +//    expect ruleview-changed,
> +//  ]
> +
> +// Tests that CSS property values are autocompleted and cycled

The description of the test should really go before the format. See https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:20
(Diff revision 4)
> +//  ]
> +
> +// Tests that CSS property values are autocompleted and cycled
> +// correctly when editing an existing property in the rule view.
> +const OPEN = true, SELECTED = true, CHANGE = true;
> +let changeTestData = [

Use const for these test data variables.

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:84
(Diff revision 4)
> +  info("Selecting the test node");
> +  await selectNode("#cell2", inspector);
> +
> +  info("Focusing the css property editable field");
> +  let ruleEditor = getRuleViewRuleEditor(view, 0);
> +

Remove this new line

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:86
(Diff revision 4)
> +
> +  info("Focusing the css property editable field");
> +  let ruleEditor = getRuleViewRuleEditor(view, 0);
> +
> +  let editor = await focusNewRuleViewProperty(ruleEditor);
> +

Remove this new line

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:90
(Diff revision 4)
> +  let editor = await focusNewRuleViewProperty(ruleEditor);
> +
> +  let gridLineNamesUpdated = inspector.once("grid-line-names-updated");
> +
> +  info("Starting to test for css property completion");
> +  for (let i = 0; i < testData.length; i++) {

We can probably simply this by doing

for (let data of testData) {}

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:107
(Diff revision 4)
> +
> +  let ruleEditor = getRuleViewRuleEditor(view, 1).rule;
> +  let prop = ruleEditor.textProps[0];
> +
> +  let gridLineNamesUpdated = inspector.once("grid-line-names-updated");
> +  info("Focusing the css property editable value");

Move this before gridLineNamesUpdates

::: devtools/client/inspector/rules/test/browser_rules_gridline-names-autocomplete.js:113
(Diff revision 4)
> +  let editor = await focusEditableField(view, prop.editor.valueSpan);
> +  await gridLineNamesUpdated;
> +
> +  info("Starting to test for css property completion");
> +
> +  for (let i = 0; i < changeTestData.length; i++) {

Same here

for (let testData of changeTestData) {}

::: devtools/client/shared/inplace-editor.js:53
(Diff revision 4)
>  
>  const WORD_REGEXP = /\w/;
>  const isWordChar = function(str) {
>    return str && WORD_REGEXP.test(str);
>  };
> +const GRID_PROPERTY_NAMES = ["grid-area", "grid-row", "grid-row-start",

Add a new line before this since WORD_REGEXP and isWordChar has no relations to these constants

::: devtools/client/shared/inplace-editor.js:1014
(Diff revision 4)
> +        GRID_PROPERTY_NAMES.includes(this.property.name)) {
> +      this.gridLineNames = await getGridLineNames();
> +    }
> +
> +    if (this.contentType == CONTENT_TYPES.CSS_VALUE && this.input &&
> +      this.input.value == "") {

We can probably indent this a bit more so that this.input.value == "" is aligned within the "(". This is similar to what you have done in line 1009.

::: devtools/server/actors/layout.js:180
(Diff revision 4)
>      }
>  
> -    // Check if the current node is a flex container.
> -    if (displayType == "inline-flex" || displayType == "flex") {
> -      return new FlexboxActor(this, treeWalker.currentNode);
> +    if (type == "flex" &&
> +       (displayType == "inline-flex" || displayType == "flex")) {
> +      return new FlexboxActor(this, currentNode);
> +    } else if (type == "grid" && displayType == "grid") {

We need to check for inline-grid as well.

::: devtools/server/actors/layout.js:195
(Diff revision 4)
>  
> -      switch (displayType) {
> -        case "inline-flex":
> +      if (type == "flex" &&
> +         (displayType == "inline-flex" || displayType == "flex")) {
> -        case "flex":
> -          return new FlexboxActor(this, currentNode);
> +        return new FlexboxActor(this, currentNode);
> -        case "contents":
> +      } else if (type == "grid" && displayType == "grid") {

Check for inline-grid

Comment 14

[Erica Wright [:ewright]]

Comment on attachment 8963307 [details]
Bug 1431071 - Gridline names suggestions offered in inspector autocomplete if element is in a grid and rule applies.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ec6e74f1e1
Grid line names suggestions offered in inspector autocomplete if element is in a grid and rule applies. r=gl, jdescottes

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/62ec6e74f1e1