1282716 - Create a new CSS Shapes highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

In bug 1242029, we're trying to support CSS shapes in devtools.
Part of this means displaying an overlay in the page that highlights where the shape is exactly.

Let's create a new highlighter for this.

This bug is only about creating a static highlighter, I'll file another one to make the highlighter interactive so users can drag the points and the shape to move them around.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Mike Park [:mparkms]]

After discussion with Gabriel, I've decided to work on the shape editor, starting with this. What would be the best approach to parse the property and render the highlighter? Would drawing lines on a canvas be good enough?

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Mike Park [:mparkms] from comment #2)
> After discussion with Gabriel, I've decided to work on the shape editor, starting with this.
Awesome! Very happy to see this picked up.
> What would be the best approach to parse the property
> and render the highlighter? Would drawing lines on a canvas be good enough?
I have spent some time thinking/prototyping this a bit, so I can help you get started.
Here are the various points I can think of:

1. You need to create a new Highlighter class for this.

First, register it at the bottom of devtools\server\actors\highlighters.js
Then actually create the file in devtools\server\actors\highlighters\
We have many other highlighters in this folder, it's easy to copy/paste one to get started.

2. Make this highlighter an auto-refresh one.

Some of our highlighters are mostly static, they don't move much. Some others follow nodes around. For instance, the box-model highlighter (which the main one you see when you use the inspector panel) displays itself around a given node. But what if this node is changed through JS, or animated through CSS? Then the highlighter actually adapts to the new position.
This is done by sub classing the AutoRefreshHighlighter parent class which provides a lot of the complex code to do this.
This shapes highlighter will need this because nodes with clip-path or shape-outside css properties can still be animated. In fact, even those properties can be animated, so the path itself can change while the highlighter is visible on the page.

3. Build a markup that will be used to render the shapes.

I would advise against using canvas here. For 2 reasons:
- we're using canvas on the grid highlighter, and it's very complex to deal with very large pages, because canvas has a max size
- shapes are usually simple to draw and SVG can be used for this very effectively.
For instance, fill-box, stroke-box, view-box, margin-box, border-box, padding-box, content-box, and inset() just describe rectangles within or around the HTML element. Of course this element may be transformed, so it's not always going to be a rectangle, but a quadrilateral at least.
We know how to draw those already in the box-model highlighter: devtools\server\actors\highlighters\box-model.js
Then for values like ellipse() or circle() or polygon(), SVG is very adapted too because it has elements to draw them already.

So, I would go for SVG here.
One thing you should know about however: the way highlighters insert elements into the page is by using a special API that Firefox only has (and that is not available to normal web pages): we have some docs here: https://github.com/mozilla/gecko-dev/blob/master/devtools/docs/tools/highlighters.md
This API makes it impossible to use the elements you insert in the page like you would normal DOM in a web page. For instance, once you've inserted elements, you can change their attributes, but can't easily append/remove nodes dynamically. Doing this requires to remove the content altogether and insert new one. It's feasible, but not very convenient, so what we usually end up doing is we insert all the nodes we need when the highlighter starts and then we just modify their attributes to show/hide them or move them around.
In this case, I think we should have a polygon, circle and ellipse ready to be used, hidden, and then we just change their sizes and coordinates and show them when needed. But they're always there.

4. Know what to draw.

I think the only "shapes" that Firefox supports currently are the ones coming from the clip-path CSS property and the shape-outside CSS property. I think both supports the same types of shapes anyway.
So, when the highlighter is shown in an element that has one of these properties, you will need to know what to draw exactly.
Meaning, all you'll know is the value of the, e.g., the clip-path property. So you will need to parse it somehow into something you can apply to your SVG shapes.

For example, a CSS polygon() function may look like this:
polygon(0px 0px, 77.5333px 50%, 155.067px 0px, 232.6px 50%, 310.133px 0px, 387.667px 50%, 465.2px 0px, 542.733px 50%, 620.267px 0px, 90% 100%, 50% 60%, 10% 100%);

From this string, you need to extract every x/y pair so you can then generate the right points attribute for the polygon SVG element.
The other difficulty here as well is units. As you can see above, units can be in px and %. SVG has its own user space unit that we should translate to.
I think we should translate everything to % of the size of the element. If you know its size, you can convert the px values to %. This way, we can draw the SVG on a 100/100 scale, and then use the viewBox attribute on the SVG element to scale everything correctly.

This is only for polygons, we need the parsing for circle and ellipse too.

5. Account for CSS transforms on the element

This makes things tricky. Matteo (:zer0 on IRC and bugzilla) knows all about this and will be happy to help you if needed.
Essentially, the problem is that an element might be transformed (using the CSS transform property). It might itself be transformed, or one of its ancestors might be transformed (or even a parent iframe).
The result is that the shape you want to draw is in fact transformed, but the coordinates you'll get from the clip-path computed value for instance are not the transformed ones.

So, somehow, you'll need to draw your shape and then transform it in the same way that the element is transformed.
The CSS grid highlighter does that too. The best way to do this would be to wait for bug 1355675 to land. With this bug, we will have access to a new API that provides us with a transform matrix we can use in CSS.
It gets more complex too, when positioning the shape, because you need to place it where the element would be if it wasn't transformed at all. But I believe we can do that.

That's all I can think of now. I have a prototype that I started a while ago. It doesn't account for many of the things I listed here, but at least it's a start so you won't have to write everything from scratch, which can be hard since this is your first server-side contribution I believe.
I'll upload it next. Here's how to use it:
- import the attachment in your local repo
- build and launch firefox
- open the inspector and select an element with clip-path
- open scratchpad and enter and execute the following script in it:

/* -sp-context:browser */
var t = [...gDevTools._toolboxes][0][1];
var i = t.getPanel("inspector");
t.highlighterUtils.getHighlighterByType("ShapesHighlighter").then(h => {
  h.show(i.selection.nodeFront, {mode: "cssClipPath"}).then(() => {
    // highlighter is now shown
  }, e => console.error("could not show", e));
}, e => console.error("could not get highlighter", e));

Comment 4

[Patrick Brosset <:pbro>]

Attachment: shapes-highlighter-wip.diff

Comment 5

[Patrick Brosset <:pbro>]

Attachment: clip-path.html

Here's a clip-path example.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: shape-outside.html

And here is a shape-outside example.

Comment 7

[Patrick Brosset <:pbro>]

I forgot to mention: this is a lot to do, but as long as the highlighter is not used in DevTools yet (i.e. there's no button in the toolbox that activates it), we can iterate easily. Meaning, you can create a simple version first that doesn't auto-update, or doesn't handle transforms, or that only handles polygons, etc... Feel free to break this down in smaller, more actionable, units of work and land those in separate patches, or even sub-bugs if you want to.

Comment 8

[Mike Park [:mparkms]]

Attachment: Bug 1282716 - Add static highlighter for polygon and circle shapes.

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

Comment 9

[Mike Park [:mparkms]]

Attachment: Bug 1282716 - Add static highlighters for ellipse and inset.

Currently doesn't handle zooms or transforms, or rounded corners for inset().

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

Comment 10

[Mike Park [:mparkms]]

About the markers used to show points on the shape: the current approach, based on the one from your diff, is to have a "markers-container" div containing a small circle, and using box-shadows to display each point on the shape. But thinking ahead to making the points editable, I don't think this approach will work, since we will need to add handlers to each point and I don't think that's possible if using box-shadow to display all the points.

How would you suggest displaying the points if this won't work? We can't easily insert an arbitrary number of divs, nor use a canvas, so I don't know what the best approach would be for displaying an arbitrary number of points that are all interactable.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Mike Park [:mparkms] from comment #10)
> How would you suggest displaying the points if this won't work? We can't
> easily insert an arbitrary number of divs, nor use a canvas, so I don't know
> what the best approach would be for displaying an arbitrary number of points
> that are all interactable.
Use a SVG <path> element. Its 'd' attribute accepts drawing commands that allow you to draw pretty much anything.
In your case, you can use the 'a' command to draw an arc (circle), then 'M' to move somewhere else, and draw another circle there, etc.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #11)
> (In reply to Mike Park [:mparkms] from comment #10)
> > How would you suggest displaying the points if this won't work? We can't
> > easily insert an arbitrary number of divs, nor use a canvas, so I don't know
> > what the best approach would be for displaying an arbitrary number of points
> > that are all interactable.
> Use a SVG <path> element. Its 'd' attribute accepts drawing commands that
> allow you to draw pretty much anything.
> In your case, you can use the 'a' command to draw an arc (circle), then 'M'
> to move somewhere else, and draw another circle there, etc.
Related: I remember why I used a separate element than the main SVG element for these points: we scale the main SVG with a viewport to make it match the element. If we were adding the points inside this main SVG, they would appear squished because of the scaling.
So, if you do use a <path> like I suggested, it still needs to be in a separate element.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8868274 [details]
Bug 1282716 - Add static highlighter for polygon and circle shapes.

https://reviewboard.mozilla.org/r/139858/#review144010

Sweet!
I have made a number of comments below.
But I have 2 general comments that apply the overall commit:
- could you please add jsdoc comments above methods and functions, so it's easier to know what these functions expect and return?
- could you please start adding tests for this piece of code, so that we can be alerted in the future if it gets broken?

::: devtools/server/actors/highlighters/shapes.js:92
(Diff revision 1)
> +        "hidden": true
> +      },
> +      prefix: this.ID_CLASS_PREFIX
> +    });
> +
> +    // TODO: Append a ...

Could you please make this TODO comment more self-explanatory?
We can never know in advance when we'll have time to work on something next, and so someone else than you might be reading this comment and might need to understand what the remaining tasks are.
Of course I know you plan to work on this yourself soon, but I think as a general is better never to assume that your comments only need to make sense to yourself only.

::: devtools/server/actors/highlighters/shapes.js:98
(Diff revision 1)
> +
> +    return container;
> +  }
> +
> +  _parseCSSShapeValue(definition) {
> +    // FIXME: for now only support polygon(...)

This FIXME is out of date because you also added support for circle.

::: devtools/server/actors/highlighters/shapes.js:99
(Diff revision 1)
> +    if (definition.includes("polygon(")) {
> +      definition = definition.substring(8, definition.length - 1);
> +      return { shapeType: "polygon", coordinates: this.polygonPoints(definition) };
> +    } else if (definition.includes("circle(")) {
> +      definition = definition.substring(7, definition.length - 1);
> +      return { shapeType: "circle", coordinates: this.circlePoints(definition) };
> +    }

Maybe make this a bit more generic if we expect other types of shapes to be needed later:

```javascript
const types = [{
  name: "polygon",
  prefix: "polygon(",
  coordParser: this.polygonPoints.bind(this)
}, {
  name: "circle",
  prefix: "circle(",
  coordParser: this.circlePoints.bind(this)
}];

for (let { name, prefix, coordinatesParser } of types) {
  if (definition.startsWith(prefix)) {
    return {
      shapeType: name,
      coordinates: coordParser(definition.substring(prefix.length, definition.length - 1))
    }
  }
}

return null;

```

::: devtools/server/actors/highlighters/shapes.js:110
(Diff revision 1)
> +    // FIXME: this isn't good enough. Computed polygon can look like this:
> +    // polygon(calc(-67.2667px + 50%) 0px, calc(67.2667px + 50%) 0px, 67.2667px 100%)
> +    // We need something really computed.

You seem to have something that takes care of calc() already. Does this comment still make sense?

::: devtools/server/actors/highlighters/shapes.js:114
(Diff revision 1)
> +  polygonPoints(definition) {
> +    // FIXME: this isn't good enough. Computed polygon can look like this:
> +    // polygon(calc(-67.2667px + 50%) 0px, calc(67.2667px + 50%) 0px, 67.2667px 100%)
> +    // We need something really computed.
> +    return definition.split(",").map(coords => {
> +      return coords.trim().replace(/ \+ /g, "+").split(" ").map((coord, i) => {

This seems to work, but a comment explaining what's happening here would be great. Something that explains why you're replacing ` + ` with `+` and then splitting on space, etc...

Also, this is the kind of code where we later discover edge cases that are not covered by it.
Which is why it is necessary to write automated tests for new code. In this particular case, if you can extract the actual string splitting mechanism to another function outside the class, it would make it really super easy to test with a unit test (xpcshell, let me know if you need help on those, some docs here: https://github.com/mozilla/gecko-dev/blob/master/devtools/docs/tests/README.md).

This is a general comment btw, we need tests for any new code, so we need tests for this new highlighter. At least one mochitest that checks that it can be displayed. And probably a few unit tests that checks the input/output of your various helper functions.

One last remark about this particular function, we have a CSS lexer in DevTools that I think might be a better fit here than using regexp and string splitting.
It's here: \devtools\shared\css\lexer.js
But a simple to use wrapper is available in 
\devtools\shared\css-parsing-utils.
You could do something like this:

```javascript
const {cssTokenizer} = require("devtools/shared/css-parsing-utils");

for (let token of cssTokenizer(definition)) {
  // ... use the tokens to extract what you need ...
  // ... while being able to deal with calc() easily ...
}
```

::: devtools/server/actors/highlighters/shapes.js:126
(Diff revision 1)
> +      });
> +    });
> +  }
> +
> +  circlePoints(definition) {
> +    let values = definition.split(" at ");

Are we garanteed to have the keyword "at" in the computed value? If yes, maybe add a short comment saying this.

::: devtools/server/actors/highlighters/shapes.js:138
(Diff revision 1)
> +    if (radius.includes("%")) {
> +      radius = radius.replace("%", "");
> +    } else if (radius.includes("px")) {
> +      let px = parseFloat(radius.replace("px", ""));
> +      let size = Math.max(elemWidth, elemHeight);
> +      radius = px * 100 / size;

This looks like something our convertCoordsToPercent function should handle. There's some code duplication here. If it can't handle it, at least move this to another function close to it.

::: devtools/server/actors/highlighters/shapes.js:166
(Diff revision 1)
> +  convertCoordToPercent(coord, size) {
> +    if (coord.includes("%")) {
> +      // Just remove the % sign, nothing else to do, we're in a viewBox that's 100%
> +      // worth.
> +      return parseFloat(coord.replace("%", ""));
> +    } else if (coord.includes("px")) {
> +      // Convert the px value to a % value.
> +      let px = parseFloat(coord.replace("px", ""));
> +      return px * 100 / size;
> +    }
> +
> +    // Unit-less value, so 0.
> +    return coord;
> +  }
> +
> +  evalCalc(expression, size) {
> +    // the calc() values returned by getComputedStyle only have addition.
> +    let values = expression.split("+");
> +
> +    let result = 0;
> +    values.forEach(value => {
> +      result += this.convertCoordToPercent(value, size);
> +    });
> +
> +    return result;
> +  }

These 2 functions don't need to access to the shapes highlighter instance. They do simple calculation on an input and return something.
So I think they would be better as small helper functions at the bottom of the file, rather than methods on the prototype of the class.

```
const coordToPercent = (coord, size) => {
  ...
};

const evalCalcExpression = (expression, size) => {
  ...
};
```

Please also add some jsdoc comments above these functions to explain what they do in more details.

::: devtools/server/actors/highlighters/shapes.js:182
(Diff revision 1)
> +    // Unit-less value, so 0.
> +    return coord;
> +  }
> +
> +  evalCalc(expression, size) {
> +    // the calc() values returned by getComputedStyle only have addition.

Wow, you're right. I didn't know that.
Can you please add a comment that explains why computed values only have additions?

::: devtools/server/actors/highlighters/shapes.js:183
(Diff revision 1)
> +    return coord;
> +  }
> +
> +  evalCalc(expression, size) {
> +    // the calc() values returned by getComputedStyle only have addition.
> +    let values = expression.split("+");

Maybe also trim each part? Not sure if really needed since you just use parseFloat later, but I think it's cleaner:

`expression.split("+").map(v => v.trim());`

::: devtools/server/actors/highlighters/shapes.js:185
(Diff revision 1)
> +    let result = 0;
> +    values.forEach(value => {
> +      result += this.convertCoordToPercent(value, size);
> +    });
> +
> +    return result;

Using a reduce would make this code a tiny bit shorter. Up to you.

```
return values.reduce((curr, prev) => {
  return prev += this.convertCoordToPercent(curr, size);
}, 0);
```

::: devtools/server/actors/highlighters/shapes.js:224
(Diff revision 1)
> +      let property = this.options.mode.substring(3);
> +      property = property.substring(0, 1).toLowerCase() + property.substring(1);

Please extract this to a small helper function outside of this class. Something at the bottom of the file for instance like:

```
const shapeModeToCssPropertyName = mode => {
  let prop = mode.substring(3);
  return prop.substring(0, 1).toLowerCase() + prop.substring(1);
};
```

This way it's short and simple to understand and has a name that is self-explanatory, and it makes the `_hasMoved` code easier to read too.

Extracting things that have different responsibilities in their own functions also allows you to add good doc comments above these functions to help people understand and reuse them.

::: devtools/server/actors/highlighters/shapes.js:242
(Diff revision 1)
> +
> +  _update() {
> +    setIgnoreLayoutChanges(true);
> +
> +    let { top, left, width, height } = this.currentQuads.border[0].bounds;
> +    let markerSize = 10;

I think this should be moved as a constant at the top of the file:

`const MARKER_SIZE = 10;`

::: devtools/server/actors/highlighters/shapes.js:256
(Diff revision 1)
> +
> +      let polygonEl = this.getElement("polygon");
> +      polygonEl.setAttribute("points", points);
> +      polygonEl.removeAttribute("hidden");
> +
> +      this.getElement("ellipse").setAttribute("hidden", true);

Instead of hiding other elements individually in each branch of this if, better hide them all once before the if itself.
Right now, you only have a polygon and an ellipse, and just an if and an else, so it's easy. But we might have more in the future. 

So, maybe extract all the ...setAttribute("hidden", true) to a new function like `_hideShapes`, call it once at the start of  `_update`.

Could you also move the code for building the polygon and the code for building the ellipse into their own functions? This will avoid `_update` being too long and hard to read. These could be `_updatePolygonShape` and `_updateEllipseShape` or similar.

::: devtools/server/actors/highlighters/shapes.js:260
(Diff revision 1)
> +      // We use this as an offset to avoid the marker itself from being on top of its
> +      // shadow.

This comment seems to belong to the `let markerSize = 10` declaration, so it should also be moved close to it.
Comments are not code, they are never checked automatically, which means that they tend to become obsolete if we don't pay attention ourselves. And when that happens, that makes the code really hard for someone else to understand.

::: devtools/server/actors/highlighters/shapes.js:280
(Diff revision 1)
> +      let shadows = `${markerSize + cx * width / 100}px 
> +        ${markerSize + cy * height / 100}px 0 0, 
> +        ${markerSize + (cx + rx) * width / 100}px 
> +        ${markerSize + cy * height / 100}px 0 0`;

nit: we try to avoid trailing spaces at the end of lines of code. In fact mozreview shows them to me in big red rectangles :)
Can you please remove them?
I have configured my text editor to remove them for me automatically when I save. That's useful.

Comment 14

[Patrick Brosset <:pbro>]

I think it makes more sense if I review the 2nd patch after you've addressed/answered my comments from the 1st one. Because I think it will impact the 2nd patch.

Comment 15

[Mike Park [:mparkms]]

Comment on attachment 8868274 [details]
Bug 1282716 - Add static highlighter for polygon and circle shapes.

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

Comment 16

[Mike Park [:mparkms]]

Comment on attachment 8868275 [details]
Bug 1282716 - Add static highlighters for ellipse and inset.

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

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8868274 [details]
Bug 1282716 - Add static highlighter for polygon and circle shapes.

https://reviewboard.mozilla.org/r/139858/#review145940

Looking great! Thanks for addressing my comments and adding those test.
Please address my last minor comments below and feel free to land this if you have a green try build.

::: devtools/server/actors/highlighters/shapes.js:101
(Diff revisions 1 - 2)
> +   * Parses the CSS definition given and returns the shape type associated
> +   * with the definition and the coordinates necessary to draw the shape.
> +   * @param {String} definition the input CSS definition
> +   * @returns {Object} null if the definition is not of a known shape type,
> +   *          or an object of the type { shapeType, coordinates }, where
> +   *          shapeType is the name of the shape and coordinates are an array
> +   *          or object of the coordinates needed to draw the shape.

Great jsdoc, thanks for adding those!

::: devtools/server/actors/highlighters/shapes.js:156
(Diff revisions 1 - 2)
>    }
>  
> +  /**
> +   * Parses the definition of the CSS circle() function and returns the x/y radiuses and
> +   * center coordinates, converted to percentages.
> +   * @param {any} definition the arguments of the circle() function

The type isn't `any` here I think, it's got to be `String`

::: devtools/server/actors/highlighters/shapes.js:165
(Diff revisions 1 - 2)
>      let elemWidth = this.currentQuads.border[0].bounds.width;
>      let elemHeight = this.currentQuads.border[0].bounds.height;

This could be for a later bug: we're using the `border` quad here, however maybe the shape is different depending on whether padding and/or border is applied to the element. I think you should test these cases and file new bugs if we need to adjust this.

Another remark about this, which should also be done in another bug: what if the element is fragmented (i.e. has several quads)? Then using only the first one with `[0]` won't be enough. It would be nice to also test this and open another bug to fix this later too.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_01.js:49
(Diff revision 2)
> +  let circleNode = yield getNodeFront("#circle", inspector);
> +  yield highlighterFront.show(circleNode, {mode: "cssClipPath"});
> +
> +  let ellipseHidden = yield testActor.getHighlighterNodeAttribute(
> +    "shapes-ellipse", "hidden", highlighterFront);
> +  polygonHidden = yield testActor.getHighlighterNodeAttribute(

I think you miss an `ok()` assertion here to check that the polygon is hidden now.

::: devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html:1
(Diff revision 2)
> +<!DOCTYPE html>

Please add the following license header at the top of this file:

```html
<!-- Any copyright is dedicated to the Public Domain.
     http://creativecommons.org/publicdomain/zero/1.0/ -->
```

::: devtools/server/tests/unit/test_shapes_highlighter_helpers.js:18
(Diff revision 2)
> +  add_task(test_split_coords);
> +  add_task(test_coord_to_percent);
> +  add_task(test_eval_calc_expression);
> +  add_task(test_shape_mode_to_css_property_name);

`add_task` is useful to run an async part of a test, using a generator function that may yield on promises.
You don't need this here, everything is synchronous, so you could simply call each test function one by one:

```
function run_test() {
  test_split_coords();
  test_coord_to_percent();
  test_eval_calc_expression();
  test_shape_mode_to_css_property_name();
}
```

And then, just remove the `*` character in these function definitions so they are just plain functions, not generators.

::: devtools/server/tests/unit/test_shapes_highlighter_helpers.js:38
(Diff revision 2)
> +}
> +
> +function* test_coord_to_percent() {
> +  let size = 1000;
> +
> +  equal(coordToPercent("50%", size), 50, "coordToPercent for percent value");

50% of a size 1000 isn't 50, I think you meant to write 500 here.

::: devtools/server/tests/unit/test_shapes_highlighter_helpers.js:44
(Diff revision 2)
> +  let size = 1000;
> +  let oneValue = evalCalcExpression("50%", size);
> +  let twoValues = evalCalcExpression("50% + 100px", size);
> +  let withZero = evalCalcExpression("0 + 100px", size);
> +  let withNegative = evalCalcExpression("-200px+50%", size);
> +
> +  equal(oneValue, 50, "evalCalcExpression with one value");
> +  equal(twoValues, 60, "evalCalcExpression with percent and px values");
> +  equal(withZero, 10, "evalCalcExpression with a zero value");
> +  equal(withNegative, 30, "evalCalcExpression with a negative value");

It might be nice to make this code more generic. In fact, you could do something similar with each of these test functions. Something like:

```javascript
const size = 1000;
const tests = [{
  desc: "One value only",
  expr: "50%",
  expected: 50
}, {
  desc: "Two values",
  expr: "50% + 100px",
  expected: 60
}, 
...
];

for (let  { desc, expr, expected } of tests) {
  equal(evalCalcExpression(expr), expected, desc);
}
```

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8868275 [details]
Bug 1282716 - Add static highlighters for ellipse and inset.

https://reviewboard.mozilla.org/r/139860/#review145950

Great. I have a few remarks, but not major enough to prevent R+, so there you go! Good job so far. Really happy with how this is going.
I know, from experience, that this highlighter is going to become much more complex in the future with everything that has to do with zooming, scrolling, dealing with high-density displays, etc. but so far the structure is very simple to follow and will be easy to extend to cover other use cases.
Also, as discussed before, this is still hidden by default, so we have a lot of freedom shipping small increments and fixing those other edge cases in separate bugs.

::: devtools/server/actors/highlighters/shapes.js:211
(Diff revision 2)
> +   * Parses the definition of the CSS ellipse() function and returns the x/y radiuses and
> +   * center coordinates, converted to percentages.
> +   * @param {String} definition the arguments of the ellipse() function
> +   * @returns {Object} an object of the form { rx, ry, cx, cy }, where rx and ry are the
> +   *          radiuses for the x and y axes, and cx and cy are the x/y coordinates for the
> +   *          center of the circle. All values are evaluated and converted to percentages.

nit: it should say `ellipse` here, not `circle`

::: devtools/server/actors/highlighters/shapes.js:276
(Diff revision 2)
> +    let elemWidth = this.currentQuads.border[0].bounds.width;
> +    let elemHeight = this.currentQuads.border[0].bounds.height;

These 2 lines are now repeated in 4 methods I think. Time to extract this in its own method:

```javascript
get currentDimensions() {
  return {
    width: this.currentQuads.border[0].bounds.width,
    height: this.currentQuads.border[0].bounds.height
  };
}
```

This way instead of repeating these 2 lines in several places, you can directly access `this.currentDimenions`

::: devtools/server/actors/highlighters/shapes.js:420
(Diff revision 2)
> +    this.getElement("polygon").setAttribute("hidden", true);
> +    this.getElement("rect").setAttribute("hidden", true);

Do we need to hide these 2 elements again? They normally should already be hidden when `_hideShapes` gets called at the start of `_update`.

::: devtools/server/actors/highlighters/shapes.js:446
(Diff revision 2)
> +    this.getElement("polygon").setAttribute("hidden", true);
> +    this.getElement("rect").setAttribute("hidden", true);

Same here, I thought we were hiding everything at once and then only showing what was necessary.

::: devtools/server/actors/highlighters/shapes.js:475
(Diff revision 2)
> +    this.getElement("polygon").setAttribute("hidden", true);
> +    this.getElement("ellipse").setAttribute("hidden", true);

Same here.

::: devtools/server/actors/highlighters/shapes.js:478
(Diff revision 2)
> +    let shadows = "";
> +
> +    this.getElement("markers-container").setAttribute("style",
> +      `top:${top - MARKER_SIZE}px;left:${left - MARKER_SIZE}px;box-shadow:${shadows};`);

You can simply omit defining the `shadows` variable here and remove it from the style string below.

Comment 19

[Mike Park [:mparkms]]

Comment on attachment 8868274 [details]
Bug 1282716 - Add static highlighter for polygon and circle shapes.

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

Comment 20

[Mike Park [:mparkms]]

Comment on attachment 8868275 [details]
Bug 1282716 - Add static highlighters for ellipse and inset.

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

Comment 21

[Mike Park [:mparkms]]

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

Try is all green.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28bd68d30222
Add static highlighter for polygon and circle shapes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e9e3bea0eddd
Add static highlighters for ellipse and inset. r=pbro

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b21dd9fab3d5
Add missing newline to doc_inspector_highlighter_cssshapes.html to make ESLint happy.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28bd68d30222
https://hg.mozilla.org/mozilla-central/rev/e9e3bea0eddd
https://hg.mozilla.org/mozilla-central/rev/b21dd9fab3d5

Comment 25

[Sebastian Zartner [:sebo]]

Note: As long as there is no support for in-page editing, the corners should *not* have handles. Otherwise the UI is misleading. And the area covered by the shape should be colored, so it's easier to see.

Sebastian

Comment 26

[Mike Park [:mparkms]]

In page editing is covered in bug 1282719 and the patch is currently under review. That will land before we make it user-visible, so it's not a big deal. I agree that the outline should be more visible, but I don't know about colouring the whole shape. Any thoughts, Patrick?

Comment 27

[Patrick Brosset <:pbro>]

I agree with Mike, this feature is completely hidden for now, so we can experiment with it incrementally, and when we decide to turn it on, we'll do a review to make sure it all makes sense. As things look now, we think we'll turn it on with in-page editing support.

Coloring the whole shape might be hard depending on the background of the element. Today we have similar problems with the box-model highlighter which, in some cases may be hard to see (thankfully, the box-model highlighter has several colors, for each region, so there are less chances for it not being visible).

I think the most important here is the stroke, that's what defines the shape, and the points, which are the things you ultimately control from CSS. So I like the current design. We could however make the stroke a bit more visible. Maybe just thicker.

But we have to be careful, some shapes may be really small, like a small 4px*4px triangle. Actually clip-path is a good candidate for creating small shapes that we used to use extra DOM elements for. So if we make our stroke thicker and our points bigger, then they'll overlap.

So, long way for saying I don't know! But it's never as simple as saying we should choose this or that color, or this or that stroke. We need to discuss this in a new bug, and use actual production usages of clip-path out there that we can find, and see what our current design look like there.

Comment 28

[Jen Simmons [:jensimmons]]

Gabe, Mike and I had a meeting about the current state of this tool yesterday. 
I opened up the Shapes Editor that Razvan Caliman made, and took some screenshots as we were talking.
I thought I'd post the links here too, since these text-only conversations can get very abstract, and sometimes it's easier to just look at a thing.

https://monosnap.com/file/35QoAmiqkXYCNrJ7SsyGdznvfKOlna.png
https://monosnap.com/file/MnQPzhHFb5bF2o1dHg3NjjnWfgLYfb.png
https://monosnap.com/file/zP6QqWcODCSGUEyyxs5uyIeyUbAaDs.png