1282719 - Make the CSS Shapes highlighter points editable

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

In bug 1282716, we're creating a new CSS Shapes highlighter that shows where a given shape is in the page.

This bug is about making the points in the shape editable.

- It should be possible to click to add a new point
- It should be possible drag a point to a different position
- It should be possible to delete a point

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Mike Park [:mparkms]]

Attachment: Bug 1282719 - Make CSS shapes highlighter points editable.

Click and drag markers, circle edges, and radius edges to move them.
Double click on a polygon edge to add a new point.
Double click on a polygon point to remove it.

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

Comment 3

[Patrick Brosset <:pbro>]

I think this is much more in Matteo's alley now, especially because he has worked on the GeometryEditor in the past, and this change is similar to that.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

I'm at cssday today and tomorrow, I'll try to review it tomorrow but it's most likely on Monday.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8877767 [details]
Bug 1282719 - Make CSS shapes highlighter points editable.

https://reviewboard.mozilla.org/r/149200/#review157488

Thanks, it looks great! I set r- only for one major concern: everything is calculated in percentage: if you edit a shape that use different units (or a mix of them) after editing one point the whole shape rule is now in percent. And this is not what we want, we should keep the units of the path since you have often a reason to express a point in pixel (absolute) and in percentage (relative).

Beside that, there are some minor nits, the other point I'd like to address in the usage of the timeout in the unit test.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_04.js:229
(Diff revision 1)
> +  ok(definition.includes(
> +    `${y + 10}% ${100 - x - width - 10}% ${100 - y - height - 10}% ${x + 10}%`),
> +     "Inset edges successfully moved");
> +}
> +
> +/**

We shouldn't use timeout in our tests, since they become more fragile and error prone across different (and slower) machine.

Given your comment here, I believe you could just wait for the next repaint (`requestAnimationFrame`), or the next tick (`waitForTick` – based on `executeSoon`).

::: devtools/client/inspector/test/head.js:506
(Diff revision 1)
>        //   mouse.down(10, 20); // synthesize "mousedown" at 10,20
>        //   mouse.move(20, 30); // synthesize "mousemove" at 20,30
>        //   mouse.up();         // synthesize "mouseup" at 20,30
>        mouse: new Proxy({}, {
>          get: (target, name) =>
> -          function* (x = prevX, y = prevY) {
> +          function* (x = prevX, y = prevY, selector = ":root") {

Is there a reason why the :root element is not enough? In test we usually synth event related to the root element – that are captured by children elements too.

::: devtools/server/actors/highlighters/shapes.js:39
(Diff revision 1)
>      this.referenceBox = "border";
>      this.useStrokeBox = false;
> +    this.geometryBox = "";
>  
>      this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv,
>      this._buildMarkup.bind(this));

nit: indentation missing

::: devtools/server/actors/highlighters/shapes.js:42
(Diff revision 1)
>  
>      this.markup = new CanvasFrameAnonymousContentHelper(this.highlighterEnv,
>      this._buildMarkup.bind(this));
> +
> +    let { pageListenerTarget } = this.highlighterEnv;
> +    pageListenerTarget.addEventListener("mousedown", this);

Nit: You could have a `const`ant that defines the dom events, and iterate that in order to add and remove the listeners. That ensure that we add and remove the same set of events every time, and also we have a list of events we're interested in (see the geometry-editor).

::: devtools/server/actors/highlighters/shapes.js:160
(Diff revision 1)
> +    if (this.shapesHidden()) {
> +      return;
> +    }
> +
> +    const { target, type, pageX, pageY } = event;
> +    let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);

I'm not entirely sure why you need to convert to percent here.

Correct me if I'm wrong, but it seems to me that you convert everythin in percent, when you should probably deal with the default value (in that case, pixel, since the event returns pixels as coordinate) and just make one conversion at the end.

::: devtools/server/actors/highlighters/shapes.js:163
(Diff revision 1)
> +
> +    const { target, type, pageX, pageY } = event;
> +    let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);
> +
> +    switch (type) {
> +      case "pagehide":

This is my fault, since I was always thinking to polish this type of code in other highlighters but I never did, so you just inherit my bad approach here. :)

Adding the highlighter itself as listener is really good for a bunch of things (first of all, we do not have to `bind` all the method and override the functions again in order to remove them), however the downside is that the `handleEvent` function become too crowd. The solution would be having separate functions for each listener, so something like:

```js
  switch(type) {
    case "pagehide":
      this.onPageHide(event);
      break;
    case "mousedown":
      this.onMouseDown(event);
      break;
    .
    .
    .
  }
```

Or something like that. It's not a big deal, and we can also do it all together with the other highlighters, but I felt it was probably better to mention here.

::: devtools/server/actors/highlighters/shapes.js:680
(Diff revision 1)
>     */
>    circlePoints(definition) {
>      // The computed value of circle() always has the keyword "at".
>      let values = definition.split(" at ");
>      let radius = values[0];
> -    let zoom = getCurrentZoom(this.win);
> +    let elemWidth = this.zoomAdjustedDimensions.width;

use the destructuring assignment here, as you did already in other part of the code; especially since you're calling a getter.

::: devtools/server/actors/highlighters/shapes.js:720
(Diff revision 1)
>     *          radiuses for the x and y axes, and cx and cy are the x/y coordinates for the
>     *          center of the ellipse. All values are evaluated and converted to percentages
>     */
>    ellipsePoints(definition) {
>      let values = definition.split(" at ");
> -    let zoom = getCurrentZoom(this.win);
> +    let elemWidth = this.zoomAdjustedDimensions.width;

use the destructuring assignment here, as you did already in other part of the code; especially since you're calling a getter.

::: devtools/server/actors/highlighters/shapes.js:755
(Diff revision 1)
>      let values = definition.split(" round ");
> +    this.insetRound = values[1];
>      let offsets = splitCoords(values[0]).map(this.convertCoordsToPercent.bind(this));
>  
> -    let x, y = 0;
> -    let width = this.currentDimensions.width;
> +    let top, left = 0;
> +    let right = this.currentDimensions.width;

use the destructuring assignment here, as you did already in other part of the code; especially since you're calling a getter.

::: devtools/server/actors/highlighters/shapes.js:778
(Diff revision 1)
>  
> -    return { x, y, width, height };
> +    return { top, left, right, bottom };
>    }
>  
>    convertCoordsToPercent(coord, i) {
> -    let zoom = getCurrentZoom(this.win);
> +    let elemWidth = this.zoomAdjustedDimensions.width;

use the destructuring assignment here, as you did already in other part of the code; especially since you're calling a getter.

::: devtools/server/actors/highlighters/shapes.js:804
(Diff revision 1)
>     */
>    getElement(id) {
>      return this.markup.getElement(this.ID_CLASS_PREFIX + id);
>    }
>  
> +  shapesHidden() {

s/shapesHidden/areShapesHidden

Also, add a jsdoc for this method.

::: devtools/server/actors/highlighters/shapes.js:868
(Diff revision 1)
>     * or CSS shape has changed.
>     * @returns {Boolean} whether the highlighter was successfully updated
>     */
>    _update() {
>      setIgnoreLayoutChanges(true);
> +    let root = this.getElement("root");

You should use the `hidden` attribute, unless there is a specific reason to not do so – in that case, it should be stated in a comment.

::: devtools/server/actors/utils/shapes-geometry-utils.js:91
(Diff revision 1)
> +  return a.reduce((prev, curr, i) => {
> +    return prev + curr * b[i];
> +  }, 0);
> +};
> +
> +exports.getDistance = getDistance;

Add the `export` right after each functions.

Comment 6

[Mike Park [:mparkms]]

Comment on attachment 8877767 [details]
Bug 1282719 - Make CSS shapes highlighter points editable.

https://reviewboard.mozilla.org/r/149200/#review157488

> Is there a reason why the :root element is not enough? In test we usually synth event related to the root element – that are captured by children elements too.

synthesizeMouse scrolls to the given selector, so if it's :root, it scrolls to the top of the page, which makes most of the page out of bounds.

Comment 7

[Mike Park [:mparkms]]

Comment on attachment 8877767 [details]
Bug 1282719 - Make CSS shapes highlighter points editable.

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

Comment 8

[Mike Park [:mparkms]]

Comment on attachment 8877767 [details]
Bug 1282719 - Make CSS shapes highlighter points editable.

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

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8877767 [details]
Bug 1282719 - Make CSS shapes highlighter points editable.

https://reviewboard.mozilla.org/r/149200/#review164348

Looks great! Thanks for addressed the unit issue. There are a couple of things yet, but I think they should be in follow up bugs; so we can land this.

Here my notes, that beside the first should go in separate bugs:

1. I noticed in your demo that you had the proper UI marker as we mentioned previously, smaller, and with the over effect; but I don't see in this patch them: just to double check, is in another bug?
2. The tool seems not working well when the element is transformed (e.g. rotation)
3. The tool seems not handling SVG well with zoom, where it works perfectly for HTML element (STR: I opened the devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html, and zoom out to 30%, with the highlighter enabled for the <rect> element)
4. The tool seems not working well when the content is in a frame: it is displayed perfectly, but the dragging seems not considering the iframe's coordinates (STR: I opened the devtools/client/inspector/test/doc_inspector_highlighter_cssshapes.html, appended a <iframe> pointing to `window.location`, and enable the highlighter for one of the shape inside the iframe)

Comment 10

[Mike Park [:mparkms]]

Thanks for the review.

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #9)
> 1. I noticed in your demo that you had the proper UI marker as we mentioned
> previously, smaller, and with the over effect; but I don't see in this patch
> them: just to double check, is in another bug?

Yes, this is in bug 1373817.

Thanks for finding the other bugs. I'll file separate bugs for them.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/536ed14795b2
Make CSS shapes highlighter points editable. r=zer0

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/536ed14795b2

Comment 13

[Mike Park [:mparkms]]

Notes for testing:
Editing works for the four basic-shapes (polygon, circle, ellipse, inset) defined for the clip-path CSS property.
Editable shapes will be shown with a small icon next to the clip-path definition. Clicking that icon will enable the highlighter.
Points can be added to a polygon by double clicking on a polygon edge. Points can be removed from a polygon by double clicking the point.

Comment 14

[Rares Doghi, Desktop QA]

Hi everyone, i have retested this issue on Windows 10 , Windows 7 , Mac OSx and Linux using the latest version of nightly 62.0a1 (2018-05-28)as well as 56.0 (64-bit) and i can Confirm it as Fixed.
I will mark it accordingly.