Add mouseover interaction for the grid outline to display grid area highlights

RESOLVED FIXED in Firefox 54

Status

P3
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: micah, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We need to implement mouseover interactions for the grid outline. This bug should:

1. Figure out the row/col of the mouseover using the grid fragment data.
2. Figure out if there is a grid area, if yes then show the grid highlight.
(Assignee)

Updated

2 years ago
Blocks: 1181227
Assignee: nobody → tigleym
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
(Assignee)

Updated

2 years ago
Depends on: 1341498
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review116294
Attachment #8840275 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8840275 - Flags: review?(gl)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8840322 [details]
Bug1337235 - Mouseover interactions for grid outline.

https://reviewboard.mozilla.org/r/114828/#review116972

::: devtools/client/inspector/layout/components/GridOutline.js:63
(Diff revision 1)
>        }
>      );
>    },
>  
>    /**
> +  * getGridAreaName - Determines if grid cell is part of a grid area and

Apply the JSDoc formatting that you should be used to from the grid outline patch.

::: devtools/client/inspector/layout/components/GridOutline.js:63
(Diff revision 1)
>        }
>      );
>    },
>  
>    /**
> +  * getGridAreaName - Determines if grid cell is part of a grid area and

Comment should be:
Returns the grid area name if the given grid cell is part of a grid area.

::: devtools/client/inspector/layout/components/GridOutline.js:101
(Diff revision 1)
> -      highlightedGrids.push(grid.gridFragments[0]);
>      }
>      return highlightedGrids;
>    },
>  
> -  renderCell(fragment, columnNumber, rowNumber) {
> +  renderCell(fragment, columnNumber, rowNumber, areas, nodeId) {

This is mostly a rebasing thing since it differs from the latest grid outline patch, but I don't think we need the fragment here.

::: devtools/client/inspector/layout/components/GridOutline.js:102
(Diff revision 1)
>      }
>      return highlightedGrids;
>    },
>  
> -  renderCell(fragment, columnNumber, rowNumber) {
> +  renderCell(fragment, columnNumber, rowNumber, areas, nodeId) {
> +    const name = this.getGridAreaName(fragment, areas);

The grid fragment object is different from {row, col} as an object. I would just pass in the individual row and col to not confuse things.

::: devtools/client/inspector/layout/components/GridOutline.js:107
(Diff revision 1)
> +    const name = this.getGridAreaName(fragment, areas);
>      return dom.rect(
>        {
>          className: "grid-outline-cell",
> +        "data-nodeId": nodeId,
> +        "data-name": name,

Put data-name before data-nodeId

::: devtools/client/inspector/layout/components/GridOutline.js:119
(Diff revision 1)
>      );
>    },
>  
> -  renderFragment(gridFragment) {
> -    const { rows, cols } = gridFragment;
> +  renderFragment({gridFragments, id}) {
> +    // TODO: We are drawing the first fragment since only one is currently being stored.
> +    // In future we will need to iterate over all fragments of a grid.

In the future,*

::: devtools/client/inspector/layout/components/GridOutline.js:151
(Diff revision 1)
>  
>      return rectangles;
>    },
>  
>    onMouseOverCell({ target }) {
> -    // TODO: We will want onMouseOver interactions with the grid cells, which is addressed
> +    const { grids, onShowGridAreaHighlight } = this.props;

const {
  grids,
  ...
} = this.props;

::: devtools/client/inspector/layout/components/GridOutline.js:153
(Diff revision 1)
>    },
>  
>    onMouseOverCell({ target }) {
> -    // TODO: We will want onMouseOver interactions with the grid cells, which is addressed
> -    // in Bug 1337235
> +    const { grids, onShowGridAreaHighlight } = this.props;
> +    const nodeId = target.getAttribute("data-nodeId");
> +    const name = target.getAttribute("data-name");

Move this above nodeId

::: devtools/client/inspector/layout/layout.js:229
(Diff revision 1)
>  
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a change in the grid area being highlighted.

We first describe when this handler is called:
Handler for a mouseover in the grid outline of a grid area. Highlights the grid area in the CSS Grid Highlighter for the given grid area.

::: devtools/client/inspector/layout/layout.js:231
(Diff revision 1)
>        },
>  
>        /**
> +      * Handler for a change in the grid area being highlighted.
> +      * Shows the grid area currently being highlighted.
> +      * @param  {NodeFront} node

Format the JSDoc similar to what we have done before.
Attachment #8840322 - Flags: review?(gl)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8840322 - Attachment is obsolete: true

Comment 8

2 years ago
mozreview-review
Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117004

::: devtools/client/inspector/layout/components/GridOutline.js:44
(Diff revision 4)
>        selectedGrids: grids.filter(grid => grid.highlighted),
>      });
>    },
>  
> -  renderGridCell(columnNumber, rowNumber, color) {
> +  /**
> +  * Returns the grid area name if the given grid cell is part of a grid area.

Each * needs 1 more space indentation. Should align with /*

::: devtools/client/inspector/layout/components/GridOutline.js:52
(Diff revision 4)
> +  *         The column number of the grid cell.
> +  * @param  {Number} row
> +  *         The row number of the grid cell.
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment
> +  * @returns  {String}

s/@returns/@return.

{String} should align with {Array} so remove one space. after @return.

::: devtools/client/inspector/layout/components/GridOutline.js:73
(Diff revision 4)
> +  *
> +  * @param  {Number} x
> +  *         The x-position of the grid cell.
> +  * @param  {Number} y
> +  *         The y-position of the grid cell.
> +  * @param  {Number} col

s/col/columnNumber

::: devtools/client/inspector/layout/components/GridOutline.js:75
(Diff revision 4)
> +  *         The x-position of the grid cell.
> +  * @param  {Number} y
> +  *         The y-position of the grid cell.
> +  * @param  {Number} col
> +  *         The column number of the grid cell.
> +  * @param  {Number} row

s/row/rowNumber

::: devtools/client/inspector/layout/components/GridOutline.js:78
(Diff revision 4)
> +  * @param  {Number} col
> +  *         The column number of the grid cell.
> +  * @param  {Number} row
> +  *         The row number of the grid cell.
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment

Add with a period.
s/List/Array
s/grid areas/grid areas data
s/on/in the

::: devtools/client/inspector/layout/components/GridOutline.js:82
(Diff revision 4)
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment
> +  */
> +  renderGridCell(x, y, columnNumber, rowNumber, color, areas) {
> +    const name = this.getGridAreaName(columnNumber, rowNumber, areas);
>      return dom.rect(

I think we have a grid key that we can store in a data-id attribute to make getting the grid and its nodefront easier in onShowGridAreaHighlight so we don't have to traverse through the list of grids as well.

::: devtools/client/inspector/layout/components/GridOutline.js:85
(Diff revision 4)
> +  renderGridCell(x, y, columnNumber, rowNumber, color, areas) {
> +    const name = this.getGridAreaName(columnNumber, rowNumber, areas);
>      return dom.rect(
>        {
>          className: "grid-outline-cell",
> -        x: columnNumber,
> +        "data-name": name,

s/data-name/data-grid-area-name

::: devtools/client/inspector/layout/components/GridOutline.js:164
(Diff revision 4)
> +    const name = target.getAttribute("data-name");
> +    const color = target.getAttribute("stroke");
> +
> +    target.setAttribute("fill", color);
> +
> +    onShowGridAreaHighlight(name, color);

We need to make sure we aren't still highlighting the grid area when we leave a cell that has a grid area to a cell that doesn't have a grid area.

::: devtools/client/inspector/layout/layout.js:105
(Diff revision 4)
>          return this.swatchColorPickerTooltip;
>        },
>  
>        /**
>         * Set the inspector selection.
>         * @param {NodeFront} nodeFront

Add a new line before @param here while you are making fixes.

::: devtools/client/inspector/layout/layout.js:153
(Diff revision 4)
>  
>        onShowBoxModelEditor,
>        onShowBoxModelHighlighter,
>  
>       /**
>         * Shows the box-model highlighter on the element corresponding to the provided

Ah, I see the indentation for this is also messed up. We need to remove a space before the *. Can you fix this while we are at it.

::: devtools/client/inspector/layout/layout.js:167
(Diff revision 4)
>          let toolbox = this.inspector.toolbox;
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a mouseover in the grid outline of a grid area.

* needs 1 more space indented.

::: devtools/client/inspector/layout/layout.js:167
(Diff revision 4)
>          let toolbox = this.inspector.toolbox;
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a mouseover in the grid outline of a grid area.

This isn't really a handler for any typical events. So, we should just be describing its functionality which is stated in the second sentence of the comments. We can remove this handler sentence.
Attachment #8840275 - Flags: review?(gl)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117246
Attachment #8840275 - Flags: review?(gl)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117556
Attachment #8840275 - Flags: review?(gl) → review+

Comment 14

2 years ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a15dfecc9fa
Mouseover interaction for grid outline. r=gl

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a15dfecc9fa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.