Add ability to display grid line name in css-grid highlighter

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Inspector
P3
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
We want the ability to show a grid's line name and number in the grid highlighter.
(Assignee)

Updated

9 months ago
Summary: Add ability to display grid line names and their numbers for grid highlighter → Add ability to display grid line name in css-grid highlighter

Updated

9 months ago
Blocks: 1347964

Updated

9 months ago
Priority: -- → P3
(Assignee)

Updated

9 months ago
Blocks: 1347336
Comment hidden (mozreview-request)

Comment 2

9 months ago
mozreview-review
Comment on attachment 8849849 [details]
Bug 1347338 - Add ability to grid line name in grid highlighter.

https://reviewboard.mozilla.org/r/122578/#review124948

Needs rebasing
Attachment #8849849 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8849849 [details]
Bug 1347338 - Add ability to grid line name in grid highlighter.

https://reviewboard.mozilla.org/r/122578/#review126402

::: devtools/server/actors/highlighters/css-grid.js:136
(Diff revision 3)
>   *         <span class="css-grid-cell-infobar-position">Grid Cell Position</span>
>   *         <span class="css-grid-cell-infobar-dimensions"Grid Cell Dimensions></span>
>   *       </div>
>   *     </div>
> + *   <div class="css-grid-line-infobar-container">
> + *     <div class="css-line-infobar">

This should use the css-grid-infobar class.

::: devtools/server/actors/highlighters/css-grid.js:137
(Diff revision 3)
>   *         <span class="css-grid-cell-infobar-dimensions"Grid Cell Dimensions></span>
>   *       </div>
>   *     </div>
> + *   <div class="css-grid-line-infobar-container">
> + *     <div class="css-line-infobar">
> + *       <div class="css-line-infobar-text">

This should use the css-grid-infobar-text class.

::: devtools/server/actors/highlighters/css-grid.js:139
(Diff revision 3)
>   *     </div>
> + *   <div class="css-grid-line-infobar-container">
> + *     <div class="css-line-infobar">
> + *       <div class="css-line-infobar-text">
> + *         <span class="css-grid-line-infobar-position">Grid Line Position</span>
> + *         <span class="css-grid-line-infobar-names"Grid Line Names></span>

s/"css-grid-line-infobar-names"/"css-grid-line-infobar-names">

This actually needs to be fixed for all the other infobar-dimensions

::: devtools/server/actors/highlighters/css-grid.js:342
(Diff revision 3)
>          "id": "cell-infobar-dimensions"
>        },
>        prefix: this.ID_CLASS_PREFIX
>      });
>  
> +    // Building the grid line container infobar markup

s/grid line container infobar/grid line infobar

::: devtools/server/actors/highlighters/css-grid.js:719
(Diff revision 3)
> +   * @param  {Number} y
> +   *         The y-coordinate of the grid line.
> +   */
> +  _updateGridLineInfobar(gridLineNames, gridLineNumber, x, y) {
> +    let position = `${gridLineNumber}`;
> +    let names = `${gridLineNames}`;

gridLineNames is an array so we should be probably generate a proper string from the array.

I would probably generate a comma separated string containing all the grid line names.

::: devtools/server/actors/highlighters/css-grid.js:1141
(Diff revision 3)
>  
>      this._updateGridCellInfobar(rowNumber, columnNumber, x1, x2, y1, y2);
>      this._showGridCellInfoBar();
>    },
>  
> +  renderGridLineNames(gridFragmentIndex, lineNumber, names, dimensionType) {

Add a jsdoc for this function

::: devtools/server/actors/highlighters/css-grid.js:1150
(Diff revision 3)
> +      return;
> +    }
> +
> +    let linePos;
> +
> +    if (dimensionType === "row") {

dimensionType should point to one of the 2 constants we have for ROW and COLUMNS

::: devtools/server/actors/highlighters/css-grid.js:1166
(Diff revision 3)
> +    let { bounds } = this.currentQuads.content[gridFragmentIndex];
> +
> +    const rowYPosition = fragment.rows.lines[0];
> +    const colXPosition = fragment.rows.lines[0];
> +
> +    let x = dimensionType === "col"

Same as above

::: devtools/server/actors/highlighters/css-grid.js:1167
(Diff revision 3)
> +
> +    const rowYPosition = fragment.rows.lines[0];
> +    const colXPosition = fragment.rows.lines[0];
> +
> +    let x = dimensionType === "col"
> +    ? linePos.start + (bounds.left / currentZoom)

I would give at least one indent

::: devtools/server/actors/highlighters/css-grid.js:1171
(Diff revision 3)
> +    let x = dimensionType === "col"
> +    ? linePos.start + (bounds.left / currentZoom)
> +    : colXPosition.start + (bounds.left / currentZoom);
> +
> +    let y = dimensionType === "row"
> +    ? linePos.start + (bounds.top / currentZoom)

Add an indent
Attachment #8849849 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8849849 [details]
Bug 1347338 - Add ability to grid line name in grid highlighter.

https://reviewboard.mozilla.org/r/122578/#review128678

So, I am gonna propose a change to showGridLineNames options. I think we shoud remove the names that we get from the client and simply grab it from the server's grid fragment data object using the grid fragment index, grid dimension type (col/row), and grid line number. Also, make sure this comma+space separated when we construct the list of grid line names. The reason for this is that we want to avoid storing so much state information on the client side, and we are already accessing this information on the server.

Also, we should style the line numbers and line names in the infobar. I think the line numbers can be purple, and if we have line names we should have a border-left to separate the line number and line names. See :-moz-native-anonymous [class$=infobar-dimensions] in devtools/server/actors/highlighters.css as an example. Finally, we should center the text.

Also, you will see that moveInfobar has changed for the box model and breaks the current grid outline highlights. To temporarily fix this, replace moveInfobar with this in devtools/server/actors/highlighters/utils/markup.js:
function moveInfobar(container, bounds, win) {
  let winHeight = win.innerHeight * getCurrentZoom(win);
  let winWidth = win.innerWidth * getCurrentZoom(win);
  let winScrollY = win.scrollY;

  // Ensure that containerBottom and containerTop are at least zero to avoid
  // showing tooltips outside the viewport.
  let containerBottom = Math.max(0, bounds.bottom) + INFOBAR_ARROW_SIZE;
  let containerTop = Math.min(winHeight, bounds.top);

  // Can the bar be above the node?
  let top;
  if (containerTop < INFOBAR_HEIGHT) {
    // No. Can we move the bar under the node?
    if (containerBottom + INFOBAR_HEIGHT > winHeight) {
      // No. Let's move it inside. Can we show it at the top of the element?
      if (containerTop < winScrollY) {
        // No. Window is scrolled past the top of the element.
        top = 0;
      } else {
        // Yes. Show it at the top of the element
        top = containerTop;
      }
      container.setAttribute("position", "overlap");
    } else {
      // Yes. Let's move it under the node.
      top = containerBottom;
      container.setAttribute("position", "bottom");
    }
  } else {
    // Yes. Let's move it on top of the node.
    top = containerTop - INFOBAR_HEIGHT;
    container.setAttribute("position", "top");
  }

  // Align the bar with the box's center if possible.
  let left = bounds.right - bounds.width / 2;
  // Make sure the while infobar is visible.
  let buffer = 100;
  if (left < buffer) {
    left = buffer;
    container.setAttribute("hide-arrow", "true");
  } else if (left > winWidth - buffer) {
    left = winWidth - buffer;
    container.setAttribute("hide-arrow", "true");
  } else {

    container.removeAttribute("hide-arrow");
  }

  let style = "top:" + top + "px;left:" + left + "px;";
  container.setAttribute("style", style);
}

::: devtools/server/actors/highlighters/css-grid.js:99
(Diff revision 5)
>   *     @param  {Object} { gridFragmentIndex: Number, rowNumber: Number,
>   *                        columnNumber: Number }
>   *     An object containing the grid fragment index, row and column numbers to the
>   *     corresponding grid cell to highlight for the current grid.
> + * - showGridLineNames({ gridFragmentIndex: Number, lineNumber: Number,
> + *                        lineNames: Array, type: String })

gridFragmentIndex and lineNames indent should line up.

::: devtools/server/actors/highlighters/css-grid.js:561
(Diff revision 5)
>    showGridCell({ gridFragmentIndex, rowNumber, columnNumber }) {
>      this.renderGridCell(gridFragmentIndex, rowNumber, columnNumber);
>    },
>  
>    /**
> +   * Shows the grid cell highlight for the given grid cell options.

Shows the grid line highlight*

::: devtools/server/actors/highlighters/css-grid.js:567
(Diff revision 5)
> +   *
> +   * @param  {Number} options.gridFragmentIndex
> +   *         Index of the grid fragment to render the grid cell highlight.
> +   * @param  {Number} options.lineNumber
> +   *         Line number of the grid cell to highlight.
> +   * @param  {String} options.names

Remove options.names

::: devtools/server/actors/highlighters/css-grid.js:572
(Diff revision 5)
> +   * @param  {String} options.names
> +   *         Comma-separated string of names for the grid line.
> +   * @param  {String} options.type
> +   *         The dimension type of the grid line.
> +   */
> +  showGridLineNames({ gridFragmentIndex, lineNumber, names, type }) {

Remove names

::: devtools/server/actors/highlighters/css-grid.js:573
(Diff revision 5)
> +   *         Comma-separated string of names for the grid line.
> +   * @param  {String} options.type
> +   *         The dimension type of the grid line.
> +   */
> +  showGridLineNames({ gridFragmentIndex, lineNumber, names, type }) {
> +    this.renderGridLineNames(gridFragmentIndex, lineNumber, names, type);

Remove names

::: devtools/server/actors/highlighters/css-grid.js:1144
(Diff revision 5)
>      this._showGridCellInfoBar();
>    },
>  
>    /**
> +   * Render the grid line name highlight for the given grid fragment index, lineNumber,
> +   * names, and dimensionType.

Remove names

::: devtools/server/actors/highlighters/css-grid.js:1150
(Diff revision 5)
> +   *
> +   * @param  {Number} gridFragmentIndex
> +   *         Index of the grid fragment to render the grid cell highlight.
> +   * @param  {Number} lineNumber
> +   *         Line number of the grid line to highlight.
> +   * @param  {String} names

Remove names

::: devtools/server/actors/highlighters/css-grid.js:1155
(Diff revision 5)
> +   * @param  {String} names
> +   *         Comma-separated string of names belonging to the grid line.
> +   * @param  {String} dimensionType
> +   *         The dimension type of the grid line.
> +   */
> +  renderGridLineNames(gridFragmentIndex, lineNumber, names, dimensionType) {

Remove names

::: devtools/server/actors/highlighters/css-grid.js:1155
(Diff revision 5)
> +   * @param  {String} names
> +   *         Comma-separated string of names belonging to the grid line.
> +   * @param  {String} dimensionType
> +   *         The dimension type of the grid line.
> +   */
> +  renderGridLineNames(gridFragmentIndex, lineNumber, names, dimensionType) {

Construct the names in here.
Attachment #8849849 - Flags: review?(gl)

Comment 9

8 months ago
mozreview-review
Comment on attachment 8849849 [details]
Bug 1347338 - Add ability to grid line name in grid highlighter.

https://reviewboard.mozilla.org/r/122578/#review128716
Comment hidden (mozreview-request)

Comment 11

8 months ago
mozreview-review
Comment on attachment 8849849 [details]
Bug 1347338 - Add ability to grid line name in grid highlighter.

https://reviewboard.mozilla.org/r/122578/#review131458

::: devtools/server/actors/highlighters.css:195
(Diff revision 6)
>    padding-bottom: 1px;
>    display: flex;
>  }
>  
>  :-moz-native-anonymous .box-model-infobar-tagname {
>    color: hsl(285,100%, 75%);

Add a space after "285," while we are at it.

::: devtools/server/actors/highlighters.css:218
(Diff revision 6)
>    border-inline-start: 1px solid #5a6169;
>    margin-inline-start: 6px;
>    padding-inline-start: 6px;
>  }
>  
> +:-moz-native-anonymous [class$=infobar-names] {

So, we don't want to do [class$=infobar-names] selector since this isn't reused. 

Simply, we name this to :-moz-native-anonymous .css-grid-line-infobar-names { .. }

Notice, we only want the the border to show if we have grid line names. I think :not(:empty) should take care of this, but please try it out.

Also, this should be moved into the CSS Grid Highlighter section with the changes mentioned.

::: devtools/server/actors/highlighters.css:251
(Diff revision 6)
> -:-moz-native-anonymous .css-grid-cell-infobar-position {
> +:-moz-native-anonymous .css-grid-cell-infobar-position, {
>    color: hsl(285, 100%, 75%);
>  }
>  
> +
> +:-moz-native-anonymous .css-grid-line-infobar-position {

We should actually be putting this with the rule on top.

::: devtools/server/actors/highlighters.css:252
(Diff revision 6)
>    color: hsl(285, 100%, 75%);
>  }
>  
> +
> +:-moz-native-anonymous .css-grid-line-infobar-position {
> +  color: #C653FF;

This should be hsl(285, 100%, 75%)

::: devtools/server/actors/highlighters/css-grid.js:133
(Diff revision 6)
>   *     @param  {Object} { gridFragmentIndex: Number, rowNumber: Number,
>   *                        columnNumber: Number }
>   *     An object containing the grid fragment index, row and column numbers to the
>   *     corresponding grid cell to highlight for the current grid.
> + * - showGridLineNames({ gridFragmentIndex: Number, lineNumber: Number,
> + *                       lineNames: Array, type: String })

Remove lineNames

::: devtools/server/actors/highlighters/css-grid.js:135
(Diff revision 6)
>   *     An object containing the grid fragment index, row and column numbers to the
>   *     corresponding grid cell to highlight for the current grid.
> + * - showGridLineNames({ gridFragmentIndex: Number, lineNumber: Number,
> + *                       lineNames: Array, type: String })
> + *     @param  {Object} { gridFragmentIndex: Number, lineNumber: Number,
> + *                        lineNames: String }

Remove lineNames

::: devtools/server/actors/highlighters/css-grid.js:136
(Diff revision 6)
>   *     corresponding grid cell to highlight for the current grid.
> + * - showGridLineNames({ gridFragmentIndex: Number, lineNumber: Number,
> + *                       lineNames: Array, type: String })
> + *     @param  {Object} { gridFragmentIndex: Number, lineNumber: Number,
> + *                        lineNames: String }
> + *     An object containing the grid fragment index, line number, and line names to the

Remove line names

::: devtools/server/actors/highlighters/css-grid.js:758
(Diff revision 6)
> +   *         The y-coordinate of the grid line.
> +   */
> +  _updateGridLineInfobar(gridLineNames, gridLineNumber, x, y) {
> +    let position = `${gridLineNumber}`;
> +
> +    this.getElement("line-infobar-position").setTextContent(position);

We can probably just remove position variable and do setTextContent(gridLineNumber). I think setTextContent can take a number and automatically convert it.

::: devtools/server/actors/highlighters/css-grid.js:1291
(Diff revision 6)
> +
> +    if (!fragment || !lineNumber || !dimensionType) {
> +      return;
> +    }
> +
> +    let linePos;

Move this below const { names } to be closer to the if statement since that is where we will be assigning the value.

::: devtools/server/actors/highlighters/css-grid.js:1318
(Diff revision 6)
> +
> +    let y = dimensionType === ROWS
> +      ? linePos.start + (bounds.top / currentZoom)
> +      : rowYPosition.start + (bounds.top / currentZoom);
> +
> +    this._updateGridLineInfobar(names.join(), lineNumber, x, y);

names.join(", ")
Attachment #8849849 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)

Comment 13

8 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5069379089a
Add ability to grid line name in grid highlighter. r=gl
Notice that, due to my work on bug 1297072 (the grid transformation), this part would likely to be broken, and will need to be changed.

Gabriel, do you have a specific test page I can use to see this patch working – with edge cases too maybe – so that I can be sure I will port the same behavior across bug 1297072 ?
Flags: needinfo?(gl)
Any page with grid lines and grid line names and hovering over the grid outline. I think this really only affects the bounds where you place the infobar for the grid line number and names. http://gridbyexample.com/examples/code/example7.html.
Flags: needinfo?(gl)

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5069379089a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.