Make the CSS Shapes highlighter points editable

VERIFIED FIXED in Firefox 56

Status

P2
enhancement
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: pbro, Assigned: mpark)

Tracking

(Blocks: 2 bugs)

49 Branch
Firefox 56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified, firefox62 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Blocks: 1333358
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8877767 - Flags: review?(pbrosset) → review?(zer0)
(Reporter)

Comment 3

a year ago
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.
I'm at cssday today and tomorrow, I'll try to review it tomorrow but it's most likely on Monday.
(Assignee)

Updated

a year ago
Assignee: nobody → mpark

Comment 5

a year ago
mozreview-review
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.
Attachment #8877767 - Flags: review?(zer0) → review-
(Assignee)

Comment 6

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
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)
Attachment #8877767 - Flags: review?(zer0) → review+
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 11

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/536ed14795b2
Make CSS shapes highlighter points editable. r=zer0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/536ed14795b2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Comment 13

11 months ago
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.
Flags: qe-verify+

Comment 14

4 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox62: --- → verified
Flags: qe-verify+

Updated

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