Closed Bug 1282717 Opened 8 years ago Closed 7 years ago

Highlight css shapes points in the page from the rule-view and back

Categories

(DevTools :: Inspector: Rules, enhancement, P2)

Product:
DevTools
Component:
Inspector: Rules
Version:
49 Branch
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: pbro, Assigned: mpark)

References

Details

Attachments

(1 file)

Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and vice versa.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
Once we have an overlay tool for css shapes, then we could make it easier for users to know which points in the rule-view correspond to which points in the page, and vice-versa:

- hovering over coordinates in the rule-view highlights the point in the page
- hovering over a point in the page highlights the coordinates in the rule-view
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
I'm working on this bug and I wanted to check if my approach is correct.

On the highlighter, I have an SVG path that is hidden by default but is drawn over a point when you hover over it. In the rules view, I separate the shape declaration into separate <span>s, each containing a coordinate pair and a mouseover event handler. When you hover over a coordinate pair, the show function is called on the highlighter, with an option that tells it to draw the hover marker over a certain point. The style on the span is also changed to highlight it. 

I have two questions/concerns about this approach:

1) How to communicate to the client (rule view) that a point has been hovered over on the page. The other way around can be done by calling the show function with options, but I don't know how to communicate from the server to the client. Can the client listen for an event to be emitted by the highlighter? 

2) Because of the way the markers are drawn on the highlighter (a single SVG path containing an arbitrary number of circles), a regular event handler can't be attached to the markers. Instead, we need to check on every mousemove event on the page if the mouse is over a marker, and highlight the marker if it is. I fear that having to check every point on every mousemove might negatively impact performance. Is this a valid concern?
Flags: needinfo?(pbrosset)
(In reply to Mike Park [:mparkms] from comment #2)
> I'm working on this bug and I wanted to check if my approach is correct.
> 
> On the highlighter, I have an SVG path that is hidden by default but is
> drawn over a point when you hover over it.
Ok

> In the rules view, I separate the shape declaration into separate <span>s,
I think the right approach for this is inside output-parser.js, which is where we parse css properties to be displayed in the rules view, and where we create spans, etc... Is this where you're doing this too?

> each containing a coordinate pair
> and a mouseover event handler. When you hover over a coordinate pair, the
> show function is called on the highlighter, with an option that tells it to
> draw the hover marker over a certain point. The style on the span is also
> changed to highlight it. 
Sounds great!

> I have two questions/concerns about this approach:
> 
> 1) How to communicate to the client (rule view) that a point has been
> hovered over on the page. The other way around can be done by calling the
> show function with options, but I don't know how to communicate from the
> server to the client. Can the client listen for an event to be emitted by
> the highlighter? 
For now, we do not have any highlighters that send events to the client. So you're going to have to add this feature.
We do, however, have events from actors, and highlighters are controlled by an actor, so it shouldn't be too hard.

So, when the client requests a new highlighter, it does so (most of the times) by calling toolbox.highlighterUtils.getHighlighterByType(...).
This always returns a new instance of a CustomHighlighterFront (see getHighlighterByType in server/actors/inspector.js).
Then, if you look at the implementation of CustomHighlighterActor (in server/actors/highlighters.js), you'll see that all it does is find the right highlighter class by name, and calling `new` on it.

So, what that means is the client has a reference to an instance of CustomHighlighterFront, when it uses it, that sends packets to the corresponding instance of CustomHighlighterActor (show or hide), which in turns, calls show or hide on the corresponding highlighter instance.

As I said, actors can send events. So maybe what we need is simply a listener on the CustomHighlighterActor class that would listen for *all* events that a highlighter class sends, and it would just emit them again to the client.
I don't know if there's a way to listen to all events. If not, we could have 1 event "highlighter-event" with a payload that says what the event is.

To do this, in the `initialize` method of CustomHighlighterActor, we would start listening:
this._highlighter.on("highlighter-event", this._onHighlighterEvent);

Then, stop listening in `finalize`:
this._highlighter.off("highlighter-event", this._onHighlighterEvent);

That's one piece of the puzzle. Next, we need to implement `_onHighlighterEvent`:
_onHighlighterEvent: function (data) {
  events.emit(this, "highlighter-event", data);
},

Now, you also need to declare the fact that the CustomHighlighterActor can send events, at the protocol level.
So, in shared/specs/highlighters.js, inside the CustomHighlighterSpec, you'll need to add a section for it:
  events: {
    "highlighter-event": {
      type: "highlighter-event",
      data: Arg(0, "json")
    }
  },

Now, thanks to this, you can make your css shapes highlighter class emit events. First make it into an EventEmitter:
EventEmitter.decorate(this);
Next, send events:
this.emit("highlighter-event", {
  type: "an-event-type-name",
  foo: "some data"
});

And, on the client, with your instance of CustomHighlighterFront:
shapesHighlighter.on("highlighter-event", this._onHighlighterEvent);

I think this should work.
Now, 2 things to note:
- maybe there's a nicer way than having to define this meta event "highlighter-event", but I haven't looked.
- you need to realize that that's only going to work when both the client and server have the same level of code. The highlighter code has landed in 55. And you're adding this in 56. Let's assume that someone uses Firefox 56, to connect to a remote Firefox (could be on a remote device) that is 55. In this case, no events would be sent, the code is just not there on the server.
Events are nice when it comes to backward compatibility, because if the event doesn't get to the client, fine, we usually don't have code to absolutely relies on it, life continues, the feature is just less interesting.
So, we should be good.

> 2) Because of the way the markers are drawn on the highlighter (a single SVG
> path containing an arbitrary number of circles), a regular event handler
> can't be attached to the markers. Instead, we need to check on every
> mousemove event on the page if the mouse is over a marker, and highlight the
> marker if it is. I fear that having to check every point on every mousemove
> might negatively impact performance. Is this a valid concern?
I see what you mean. I don't think we have another solution though. So, to make it perform as fast as we can, you should store in an object the coordinates of all points. This way, everytime mousemove happens, it's a really quick check, just comparing coordinates basically. That shouldn't be too much of a problem.
Flags: needinfo?(pbrosset)
Assignee: nobody → mpark
Comment on attachment 8884416 [details]
Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and vice versa.

https://reviewboard.mozilla.org/r/155332/#review161594

::: devtools/client/inspector/shared/highlighters-overlay.js:204
(Diff revision 1)
> +   * Highlight the given shape point in the rule view.
> +   *
> +   * @param {String} point
> +   *        The point to highlight.
> +   */
> +  highlightRuleViewShapePoint: function (point) {

I think this function belongs in the rule-view, or in fact, possibly the text-property-editor. Closer to where the DOM for the points is actually handled.
This overlay class should have a minimal dependency on the rule-view (I would have liked it to be just like a service that doesn't really rely on the rule-view, but that's not the case).

So, I think this class should just signal that a shape point is being hovered, and then the text-property-editor should use that information to toggle the active state on the point DOM element.
This can be done using an event.

::: devtools/client/inspector/shared/highlighters-overlay.js:377
(Diff revision 1)
> +  _onHighlighterEvent: function (data) {
> +    if (data.type === "shape-hover-on") {
> +      this.state.shapes.hoverPoint = data.point;
> +      this.highlightRuleViewShapePoint(data.point);
> +    } else if (data.type === "shape-hover-off") {
> +      this.state.shapes.hoverPoint = data.point;

Do you still want to save the point in the state even if we're not hovering it anymore?

::: devtools/client/inspector/shared/highlighters-overlay.js:443
(Diff revision 1)
>  
>      if (!highlighter) {
>        return null;
>      }
>  
> +    highlighter.on("highlighter-event", this._onHighlighterEvent.bind(this));

You need to also remove this event listener when the instance is being destroyed.

I think adding a `highlighter.off(...` in the `destroy` method should do it.

But that means you need to do:

`this._onHighlighterEvent = this._onHighlighterEvent.bind(this)` in the constructor somewhere.

::: devtools/client/inspector/shared/highlighters-overlay.js:486
(Diff revision 1)
> +   * @param {NodeFront} node
> +   *        The NodeFront of the shape point to toggle
> +   * @param {Boolean} active
> +   *        Whether the shape point should be active
> +   */
> +  _toggleShapePointActive: function (node, active) {

Same comment than earlier, this belongs in the text-property-editor I think.

::: devtools/client/inspector/shared/highlighters-overlay.js:572
(Diff revision 1)
> +   * Is the current hovered node a highlightable shape point in the rule-view.
> +   *
> +   * @param  {Object} nodeInfo
> +   * @return {Boolean}
> +   */
> +  _isRuleViewShapePoint: function (nodeInfo) {

Well, this function doesn't really tell you if the node is shape point, it only tells you if the property that this node is inside is a shape CSS property.

I think you should instead change the `getNodeInfo` inside the rule-view so it returns a special type of the hovered node is a shape point.

::: devtools/client/inspector/shared/highlighters-overlay.js:621
(Diff revision 1)
>      if (!nodeInfo) {
>        return;
>      }
>  
> +    let classList = event.target.classList;
> +    if (classList.contains("ruleview-shape-point") &&

With my previous comment, you can simplify this part. No need to check the structure of the DOM here. You already have access to a `nodeInfo`, so if it contains the right information about whehter you are currently hovering a shape point, then you can change this to 
`if (this._isRuleViewShapePoint(nodeInfo))`
no need to check the DOM here. This would be already done in `getNodeInfo` in the rule-view.

::: devtools/client/inspector/shared/highlighters-overlay.js:627
(Diff revision 1)
> +        this._isRuleViewShapePoint(nodeInfo)) {
> +      let point = event.target.dataset.point;
> +      // Inset points use classes instead of data because a single span can represent
> +      // multiple points.
> +      let insetClasses = [];
> +      classList.forEach(className => {

Same here, the `getNodeInfo` could already check the type of shape so you don't have to do it here.

::: devtools/client/inspector/shared/highlighters-overlay.js:627
(Diff revision 1)
> +      classList.forEach(className => {
> +        if (INSET_POINT_TYPES.includes(className)) {
> +          insetClasses.push(className);
> +        }
> +      });
> +      if (insetClasses.length > 0) {
> +        point = insetClasses.join(",");
> +      }

Also, for info, this logic could be simplified a bit:

```
classList.some(className => INSET_POINT_TYPES.includes(className));
```

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:7
(Diff revision 1)
> +// Test toggling the shapes highlighter in the rule view and the display of the
> +// shapes highlighter.

This test doesn't check toggling of the highlighter only. It tests hovering over points. So this comment needs to be updated.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:16
(Diff revision 1)
> +
> +const HIGHLIGHTER_TYPE = "ShapesHighlighter";
> +const CSS_SHAPES_ENABLED_PREF = "devtools.inspector.shapesHighlighter.enabled";
> +
> +add_task(function* () {
> +  SpecialPowers.setBoolPref(CSS_SHAPES_ENABLED_PREF, true);

The advised way to set prefs only for the duration of a test is actually:

```
yield pushPref(PREF, value);
```

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:17
(Diff revision 1)
> +const HIGHLIGHTER_TYPE = "ShapesHighlighter";
> +const CSS_SHAPES_ENABLED_PREF = "devtools.inspector.shapesHighlighter.enabled";
> +
> +add_task(function* () {
> +  SpecialPowers.setBoolPref(CSS_SHAPES_ENABLED_PREF, true);
> +  let inspector = yield openInspectorForURL(TEST_URL);

The return value here isn't really `inspector`. This is a bit misleading because then you access the `testActor` and `inspector` properties on this object.

So I would prefer doing:

```
let {inspector, testActor} = yield openInspectorForURL(TEST_URL);
```

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:19
(Diff revision 1)
> +  let { testActor } = inspector;
> +  inspector = inspector.inspector;

Then you can remove those 2 lines that are a bit confusing.

Oh sorry, I missed the helper line before. I understand now why you needed this.

So let's do this instead:

```
let env = yield openInspectorForURL...;
let helper = yield getHighlighter...(env);
let {testActor, inspector} = env;
```

It's a little more obvious.

::: devtools/client/inspector/test/browser_inspector_highlighter-cssshape_05.js:78
(Diff revision 1)
> +  info("Show shapes highlighter");
> +  let container = getRuleViewProperty(view, "#polygon", "clip-path").valueSpan;
> +  let shapesToggle = container.querySelector(".ruleview-shape");
> +  let onHighlighterShown = highlighters.once("shapes-highlighter-shown");
> +  shapesToggle.click();
> +  yield onHighlighterShown;

Maybe move this in the head.js file that is in the same directory.
I'm guessing you need to show the highlighter many times in your tests, so better put the code in common in one file and then just use it.
Something like:
`function toggleShapeHighlighter(view, selector, property) {...}`

::: devtools/client/shared/output-parser.js:396
(Diff revision 1)
> +        let coordsBegin = prefix.length;
> +        let coordsEnd = shape.lastIndexOf(")");
> +        let valContainer = this._createNode("span", {});
>  
> -    container.appendChild(toggle);
> +        container.appendChild(toggle);
> -    container.appendChild(value);
> +
> +        valContainer.innerHTML += shape.substring(0, coordsBegin);
> +
> +        let coordsString = shape.substring(coordsBegin, coordsEnd);
> +        valContainer = coordParser(coordsString, valContainer);
> +
> +        valContainer.innerHTML += shape.substring(coordsEnd);
> +        container.appendChild(valContainer);

I see a problem with this logic as well as with the parser functions below:

the rule-view deals with **authored** code. What that means is that it deals with the CSS source code that authors have created. This is very different than the computed values that the shapes highlighter deals with.
The computed values give us safety, because we know what the syntax will look like.
Authored values have no safety at all. This could be a totall invalid polygon declaration, or valid but with no spaces where you expect them, etc.

So for instance, you can't split the coordinaes on ", " in the `_addPolygonPointNodes` function, because there might not be a space.

In `_addCirclePointNodes` there might not be a `at` keyword at all.

Furthermore, we want to keep authored values unchanged. The rule-view's job is important in that it shows what people wrote, so they see what worked and what didn't. That means that the rule-view should not rewrite what they wrote manually. That means we can't trim strings, or generate things ourselves if it doesn't correspond exactly to what they wrote.

The importance here is that at some stage in the near future, we're going to provide a diff/export view, that lists all changes made by users in the rule-view. So if the rule-view itself changes a few things like indentation, spaces, etc. then those will appear as changes even though the user hasn't made those changes.

I realize that will make your life harder here. It means you'll need to use the css lexer instead of splitting strings, etc. and try and extract information from there.

Using the lexer, you can just advance through the tokens, until you find a , character, in a polygon for instance, and then you enclose the previous value in a span, so it can be highlighted.

::: devtools/server/actors/highlighters/shapes.js:205
(Diff revision 1)
>            this[_dragging] = null;
>          }
>          break;
>        case "mousemove":
>          if (!this[_dragging]) {
> +          if (this.shapeType === "polygon") {

Can you move all of this mousemove handling in a separate function please? This makes it easier to understand what's going on.

::: devtools/server/actors/highlighters/shapes.js:207
(Diff revision 1)
>          break;
>        case "mousemove":
>          if (!this[_dragging]) {
> +          if (this.shapeType === "polygon") {
> +            let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);
> +            let point = this.getPolygonClickedPoint(percentX, percentY);

This isn't a clicked pointer anymore, you're doing a mousemove here. So maybe rename this function to `getPolygonPointAt(x, y)`

::: devtools/server/actors/highlighters/shapes.js:217
(Diff revision 1)
> +            }
> +            this._handleMarkerHover(point);
> -          return;
> +            return;
> +          } else if (this.shapeType === "circle") {
> +            let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);
> +            let point = this.getCircleClickedPoint(percentX, percentY);

Same comment for the "Clicked" part of this function's name

::: devtools/server/actors/highlighters/shapes.js:227
(Diff revision 1)
> +            }
> +            this._handleMarkerHover(point);
> +            return;
> +          } else if (this.shapeType === "ellipse") {
> +            let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);
> +            let point = this.getEllipseClickedPoint(percentX, percentY);

Same comment for the "Clicked" part of this function's name

::: devtools/server/actors/highlighters/shapes.js:237
(Diff revision 1)
> +            }
> +            this._handleMarkerHover(point);
> +            return;
> +          } else if (this.shapeType === "inset") {
> +            let { percentX, percentY } = this.convertPageCoordsToPercent(pageX, pageY);
> +            let point = this.getInsetClickedPoint(percentX, percentY);

Same comment for the "Clicked" part of this function's name

::: devtools/server/actors/highlighters/shapes.js:605
(Diff revision 1)
>      this.currentNode.style.setProperty(this.property, insetDef, "important");
>    }
>  
> +  _handleMarkerHover(point) {
> +    if (point === null || point === undefined) {
> +      this.getElement("marker-hover").setAttribute("hidden", true);

If I understand correctly, all other branches in this function end up either hiding this element, or showing it (via `_drawHoverMarker`).

So, you could simplify the code by having, as a first line of this function:

```
// Hide the hover marker for now, it will be shown if we find the right point.
this.getElement("marker-hover").setAttribute("hidden", true);
```

This way you can remove all other calls to `this.getElement("marker-hover").setAttribute("hidden", true)` in the function.
Attachment #8884416 - Flags: review?(pbrosset)
Comment on attachment 8884416 [details]
Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and vice versa.

https://reviewboard.mozilla.org/r/155332/#review161594

> Also, for info, this logic could be simplified a bit:
> 
> ```
> classList.some(className => INSET_POINT_TYPES.includes(className));
> ```

The only array iteration function that classList supports is forEach. So I can't use .filter or anything like that.
(In reply to Mike Park [:mparkms] from comment #8)
> Comment on attachment 8884416 [details]
> Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and
> vice versa.
> 
> https://reviewboard.mozilla.org/r/155332/#review161594
> 
> > Also, for info, this logic could be simplified a bit:
> > 
> > ```
> > classList.some(className => INSET_POINT_TYPES.includes(className));
> > ```
> 
> The only array iteration function that classList supports is forEach. So I
> can't use .filter or anything like that.
Oh I see. You could still do something like that though:
[...classList].some(className => INSET_POINT_TYPES.includes(className));
Thanks for the new commit, I'll take a look at it soon.
Comment on attachment 8884416 [details]
Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and vice versa.

https://reviewboard.mozilla.org/r/155332/#review163490

Thank you for addressing my feedback.

The parsing code you added in output-parser.js looks good, now we really should test it. We have a series of tests in devtools\client\shared\test\browser_outputparser.js that you can take inspiration from. You should write a test that exercises a bunch of different shape values, with spaces, no spaces, with newlines, or all on one line, valid, invalid, etc. just to make sure that the code you wrote works fine in all cases, not just the common case. It will also be useful to make sure all cases still pass when we'll make changes to the code in the future.

I'm happy for the test changes to be done in a separate commit if you prefer this.

I have a few additional comments below too.

::: devtools/client/inspector/rules/rules.js:1557
(Diff revisions 1 - 3)
>      }
>      node = node.parentNode;
>    }
>  }
>  
> +function getShapeToggleActive(node) {

Could you please add a short jsdoc comment before this function so it's easy to know what it's for?

::: devtools/client/inspector/rules/rules.js:1571
(Diff revisions 1 - 3)
> +    }
> +    node = node.parentNode;
> +  }
> +}
> +
> +function getShapePoint(node) {

Could you please add a short jsdoc comment before this function so it's easy to know what it's for?

::: devtools/client/inspector/rules/views/text-property-editor.js:955
(Diff revisions 1 - 3)
> +   * Highlight the given shape point in the rule view.
> +   *
> +   * @param {String} point
> +   *        The point to highlight.

nit: The description should also say that this is called when the "hover-shape-point" event is being emitted.

Also, to be totally correct, this function takes 2 @params, not just 1. So you should document both.

::: devtools/client/inspector/rules/views/text-property-editor.js:962
(Diff revisions 1 - 3)
> +   * @param {String} point
> +   *        The point to highlight.
> +   */
> +  _onHoverShapePoint: function (event, point) {
> +    let { highlighters } = this.ruleView;
> +    let view = highlighters.inspector.getPanel("ruleview").view;

I wonder if this is really needed, isn't `this.ruleView` the same as `highlighters.inspector.getPanel("ruleview").view` ?
I think it is. Here you're going the long way to get a property you already have.

::: devtools/client/inspector/rules/views/text-property-editor.js:969
(Diff revisions 1 - 3)
> +    let selector = `.ruleview-shape-point.active`;
> +    for (let pointNode of ruleViewEl.querySelectorAll(selector)) {
> +      this._toggleShapePointActive(pointNode, false);
> +    }
> +
> +    if (point !== null && point !== undefined) {

Wouldn't `if (point)` be enough here? Or maybe `if (typeof point === "string")` to make sure point exists and is a string.

::: devtools/client/inspector/rules/views/text-property-editor.js:978
(Diff revisions 1 - 3)
> +      // Because one inset value can represent multiple points, inset points use classes
> +      // instead of data.
> +      selector = (INSET_POINT_TYPES.includes(point)) ?
> +                 `.ruleview-shape-point.${point}` :
> +                 `.ruleview-shape-point[data-point='${point}']`;
> +      for (let pointNode of ruleViewEl.querySelectorAll(selector)) {

I'm not sure I understand the logic of running the selector on the entire rule-view element. Wouldn't that catch other potential values of clip-path or shape-outside in other rules?

Here this code exists in a text-property-editor.js, which only deals with 1 property only, inside 1 rule. So why would it need to toggle the state of potentially other properties?

It should only ever act on its own things, not elements that exist outside what it knows.

If all text-property-editor.js instances listen to the highlighter events anyway, they'll all be running this code. So I think it should be fine to restrict this to their DOM elements only.

::: devtools/client/shared/output-parser.js:460
(Diff revisions 1 - 3)
> +          fillRule = false;
> +        } else {
> +          container.appendChild(coordNode);
> +          i++;
> -      }
> +        }
> +        container.innerHTML += coords.substring(token.startOffset, token.endOffset);

Appending user-submitted code to innerHTML directly isn't safe. It's unlikely here, but what if the CSS value contained HTML code, like a button or a link to a website, then we'd be creating these elements directly in the rule-view, and giving them chrome-level privileges.

I suggest using `textContent` instead, to make the user-submitted text harmless.
Attachment #8884416 - Flags: review?(pbrosset)
Comment on attachment 8884416 [details]
Bug 1282717 - Highlight CSS shapes points in the page from the rule-view and vice versa.

https://reviewboard.mozilla.org/r/155332/#review164528

::: devtools/client/shared/test/browser_outputparser.js:434
(Diff revision 6)
> +    let frag = parser.parseCssProperty("clip-path", definition, {
> +      shapeClass: "ruleview-shape"
> +    });
> +    let spans = frag.querySelectorAll(".ruleview-shape-point");
> +    is(spans.length, spanCount, desc + " span count");
> +    is(frag.textContent, expectedText, desc + " text content");

The expectedText is always the same as the definition, right? That's what we want anyway. So maybe you can rid of the expectedText in the array above, and change this assertion to be:
`is(frag.textContent, definition, ...)`
Attachment #8884416 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8b9d3feecb
Highlight CSS shapes points in the page from the rule-view and vice versa. r=pbro
https://hg.mozilla.org/mozilla-central/rev/2f8b9d3feecb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: