1089240 - Add a measurement tool

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Tim Nguyen :ntim]

It'd be really handy to have a ruler tool built-in Firefox DevTools.

I'm currently using MeasureIt [0], but Measurer seems like an alternative as well.

[0] : https://addons.mozilla.org/en-US/firefox/addon/measureit/?src=search
[1] : https://addons.mozilla.org/en-US/firefox/addon/measurer/?src=search

Comment 1

[Tim Nguyen :ntim]

Also, this tool should be available (kinda like the eyedropper), as a command button, and an item in the Web Developer menus.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I agree on all accounts - I wonder if the new apis we use to render the highlighter could be used to do something that is safer than using content scripts ( as I assume the addons do )

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Jeff Griffiths (:canuckistani) from comment #2)
> I agree on all accounts - I wonder if the new apis we use to render the
> highlighter could be used to do something that is safer than using content
> scripts ( as I assume the addons do )
Yes, anything we want to display on top of the content page should be using the new document.insertAnonymousContent API.
The main reason being that elements inserted there aren't visible to content and do not impact content in any way.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2014-11-25 09.10.36.png

Here's a screenshot of the ruler tool in the web developer addon (https://addons.mozilla.org/en-US/firefox/addon/web-developer/)

Comment 5

[Patrick Brosset <:pbro>]

If anyone is interested in working on this bug, I'm happy to mentor.
As said above, there are at least 2 existing addons we can get ideas from, and the APIs are available. We just need someone to work on this bug.

Comment 6

[kunal jain]

hey patrick,
i am new towards development and i want to work on this with you as a mentor.
give me somework to start with

Comment 7

[Tim Nguyen :ntim]

Attachment: command-ruler.png

Attached an asset for the command button.

Comment 8

[Tim Nguyen :ntim]

Attachment: [e-mail]

2x icon

Comment 9

[Tim Nguyen :ntim]

Attachment: command-ruler.png - i02

Made it a bit bigger.

Comment 10

[Tim Nguyen :ntim]

Attachment: [e-mail] - i02

Comment 11

[Tim Nguyen :ntim]

Attachment: command-ruler.zip - i03

Chopped through an image optimizer.
The zip includes both 1x and 2x files.

Comment 12

[Patrick Brosset <:pbro>]

Thanks a lot Tim for the icons!

Here is some information about how some of the implementation of the ruler tool could be done.
First of all, it is important to go through this wiki documentation: https://wiki.mozilla.org/DevTools/Highlighter
It explains what highlighters are in the devtools, and how to create new ones.

It is important because the ruler tool needs to be displayed on top of the page, and highlighters are how we achieve this in the devtools.

So, one of the parts of the patch will be about adding one new highlighter in /toolkit/devtools/server/actors/highlighter.js:
- In this file, you'll see there are already a lot of different highlighters, and they are all pretty much done the same way,
- add a new one there, and reference its name in HIGHLIGHTER_CLASSES as well as in /toolkit/devtools/server/actors/root.js in the traits.customHighlighters array,
- this highlighter should implement show and hide, just like other highlighters,
- I think a good way to get this highlighter started is to copy/paste the RectHighlighter class. Indeed, like the RectHighlighter the Ruler should just draw a shape on the screen, rather than highlight a node and follow it around (hence no need to inherit from AutoRefreshHighlighter).

The big different between the Ruler and other highlighters that we already have is that it will be the first to actually respond to user events. This means we need to listen to click (or touch, for that matter) events on the entire document and use those to display the ruler at the right place and right dimensions.

This single fact makes this bug more complex because of the way highlighters are inserted into the page (see https://wiki.mozilla.org/DevTools/Highlighter#Inserting_content_in_the_page).
It also depends a lot on how we want this Ruler to work at all. Should you be able to drag it around? Resize it using its handles? Should it have a mini toolbar next to it showing dimensions and allowing to close it?

I haven't had a chance to think more about how to solve these particular issues yet, however I don't think they should stop us from working on the first part: creating a new ruler highlighter class (without event handling for now).

Comment 13

[Anurag Chaudhury]

This sounds really interesting. I've just started working on it and should have the highlighter part done soon. I'll attach a patch for feedback when it's done. Thanks!

Comment 14

[Patrick Brosset <:pbro>]

Matteo has started working on a couple of preliminary bugs to make this one easier. Assigning this bug to him as I know he's started to give some thoughts to it and will work on it next.

Related: this twitter discussion:
https://twitter.com/addyosmani/status/566026402592743425

Comment 15

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> Matteo has started working on a couple of preliminary bugs to make this one
> easier. Assigning this bug to him as I know he's started to give some
> thoughts to it and will work on it next.
> 
> Related: this twitter discussion:
> https://twitter.com/addyosmani/status/566026402592743425

I would see more that integrated into the responsive mode (That is tracked in bug 1031585 btw) than the ruler tool.

Comment 16

[Tim Nguyen :ntim]

Btw, I think your Geometry Highlighter work could be reusable here. This is basically the same, with only a few differences.

Comment 18

[Patrick Brosset <:pbro>]

Bug 1144163 is about a ruler tool. This bug's title is misleading, it's really about a measurement tool.

Comment 19

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

Attachment: measuring tool highlighter

Comment 20

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

Attachment: measuring tool command

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8586029 [details] [diff] [review]
measuring tool command

Review of attachment 8586029 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work ! I don't see the outline delimitating the "selected rectangle" though, the only thing I see is the tooltip with the coordinates.

Also, there seems to be lots of leading and trailing spaces in both patches.

Comment 22

[Patrick Brosset <:pbro>]

Now that the Rulers tool has landed, both of the patches here need to be rebased, they don't apply cleanly on top of fx-team.
In the meantime, I'll start reviewing the first patch (which I managed to merge easily, and will test locally using scratchpad instead of the command, which I couldn't merge easily).

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8586028 [details] [diff] [review]
measuring tool highlighter

Review of attachment 8586028 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, as I said in my last comment however, it will need to be rebased on top of fx-team.

Some general comments before I review the code itself:

- I like it, it's nice :)

- I like how the label changes position so it's always visible in the viewport. And as discussed earlier I don't think it's a problem that it sometimes has to display itself inside the rectangle.

- Minor detail: sometimes, when the rectangle is thin, the top border seems to stick out of it, by 1 or 2 pixels, see https://dl.dropboxusercontent.com/u/714210/measuring-tool-1.png

- I admit not being a huge fan of the animated stroke borders (I know this was inspired by photoshop). I see 3 somewhat problematic things with them:
  - I find the animation a little bit distracting, but that may be just me, also, when measuring a very small distance, it makes it kind of hard to see the rectangle,
  - in photoshop, the tool is a selection tool that allows much more than just measuring, and so I think the animation works there because it emphasizes that the drawn rectangle inside the borders can be manipulated,
  - it introduces a look & feel of its own, that is nowhere else in the devtools, so it lacks consistency with the rest of the tools.
I'd like other people to look at this because this is a very subjective topic, so I've made a quick screencast that others can look at and give feedback on: https://dl.dropboxusercontent.com/u/714210/measuring-tool-2.mp4

- When you just click once in the page (without dragging), the label is shown with W:0px, H:0px. The label should probably be hidden in that case.

- Sometimes, when you want to measure the horizontal distance between 2 elements, you feel like dragging your mouse in a straight horizontal line, and that works, it ends up drawing a 0px high rectangle (just a line), which is fine. The only thing is, the border looks kind of strange then: https://dl.dropboxusercontent.com/u/714210/measuring-tool-3.png
This is a result of the animated stroke border I guess, so maybe having a solid color instead would help.

- Zooming in/out the page should not zoom in/out the rectangle and label, and the rectangle's position should not change.

- Being able to move the rectangle would be great, but I think you intend to work on this in a next version, right?

- Pretty much anything you do with the highlighter (resizing the window, drawing a rectangle) causes exceptions to be thrown: TypeError: Argument 1 of Window.getComputedStyle is not an object.: CssLogic.getComputedStyle@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/styleinspector/css-logic.js:813:10
This exception is something we should fix, I've seen it when doing other things with the inspector, I will log a bug when I find a good way to reproduce. But in any case, it seems odd that your highlighter would cause this. After a quick investigation, I think this is related to reflows caused by your highlighter. Indeed, highlighters are part of the content document, and may cause reflows when they move/resize/whatever. And because we have an actor that listens for reflows in the content document, it detects the ones caused by the highlighter too.
Reflows are then used to drive several things in the devtools (in the inspector especially, but also webconsole, which can log reflows). So you should filter your reflows out. I've added a helper function to do just this. Everywhere you change attributes/styles on your highlighter that will cause a reflow, you should first do this:
 setIgnoreLayoutChanges(true);
and after you're done, this:
 setIgnoreLayoutChanges(false, this.currentNode.ownerDocument.documentElement);
The BoxModelHighlighter does this for instance.

::: toolkit/devtools/server/actors/highlighter.css
@@ +225,5 @@
>  }
> +
> +/* Measuring Tape highlighter */
> +
> +@keyframes -moz-measuring-tool-highlighter-dash {

The problem with adding this animation is that it's now available to web content. Of course the name is long, complex and starts with -moz, so it could, in theory, be alright, but since we went to great care about hiding the other rules (by prefixing with the chrome-only pseudo class :-moz-native-anonymous), I'm not so sure adding this not-chrome-only keyframes animation name would be ok.

@@ +239,5 @@
> +}
> +
> +:-moz-native-anonymous .measuring-tool-highlighter-root {
> +  position: absolute;
> +  top: 0px;

nit: no need for px unit here. In fact, you might even be able to remove top and left altogether since this is the default.

@@ +247,5 @@
> +}
> +
> +/*
> +  Setting `stroke-width` with a low floating point number, ensure that `path`
> +  draws always 1px despites the zoom level, with `crispEdges` on the ancestor.

As reported in bug 1150814, this trick won't work on all platforms.

::: toolkit/devtools/server/actors/highlighter.js
@@ +51,5 @@
> +// Hard coded value about the size of measuring tool label, in order to
> +// position and flip it when is needed.
> +const MEASURING_LABEL_MARGIN = 8;
> +const MEASURING_LABEL_WIDTH = 80 + MEASURING_LABEL_MARGIN;
> +const MEASURING_LABEL_HEIGHT = 52 + MEASURING_LABEL_MARGIN;

It's fine to have the values hard-coded here I think (knowing that we can't dynamically measure the element once it's in the canvasFrame), but to avoid duplication, you could remove it from the highlighter.css file and set it on the element style attribute from js instead.

@@ +547,5 @@
>  
>    _insert: function() {
>      // Re-insert the content node after page navigation only if the new page
>      // isn't XUL.
> +

This change seems unrelated, can you please remove it?

@@ +2618,5 @@
>  });
>  register(GeometryEditorHighlighter);
>  exports.GeometryEditorHighlighter = GeometryEditorHighlighter;
>  
> +

Just one empty line before this class is enough.

@@ +2620,5 @@
>  exports.GeometryEditorHighlighter = GeometryEditorHighlighter;
>  
> +
> +/**
> + * The MeasuringToolHighlighter is used to measure area and distance in a page.

Please elaborate a bit more:
The MeasuringToolHighlighter is used to measure distances in a content page.
It allows users to click and drag with their mouse to draw an area whose dimensions will be displayed in a tooltip next to it. This allows users to measure distances between elements on a page.

@@ +2624,5 @@
> + * The MeasuringToolHighlighter is used to measure area and distance in a page.
> + */
> +function MeasuringToolHighlighter(tabActor) {
> +  this.win = tabActor.window;
> +  this.markup = new CanvasFrameAnonymousContentHelper(tabActor, 

nit: trailing whitespace

@@ +2627,5 @@
> +  this.win = tabActor.window;
> +  this.markup = new CanvasFrameAnonymousContentHelper(tabActor, 
> +    this._buildMarkup.bind(this));
> +
> +  this[measuringCoords] = {};

I'm no sure I see the value of using a symbol to store the coordinates on the instance.
A simpler `this.coords` should work just as fine.

@@ +2631,5 @@
> +  this[measuringCoords] = {};
> +
> +  this.win.addEventListener("mousedown", this);
> +  this.win.addEventListener("scroll", this);
> +  this.win.addEventListener("pagehide", this);

You're adding event listeners on a content window here, so you're impacting the content in a way that will be visible to the user. For instance, if you open the inspector and click on the (ev) icon next to the <html> node, you'll see these event listeners there, as shown here: https://dl.dropboxusercontent.com/u/714210/measuring-tool-4.png

Please use getPageListenerTarget(tabActor) instead.

@@ +2633,5 @@
> +  this.win.addEventListener("mousedown", this);
> +  this.win.addEventListener("scroll", this);
> +  this.win.addEventListener("pagehide", this);
> +
> +  this._cancelUpdate();

the rafID will be undefined at this stage, why call cancelUpdate?

@@ +2634,5 @@
> +  this.win.addEventListener("scroll", this);
> +  this.win.addEventListener("pagehide", this);
> +
> +  this._cancelUpdate();
> +  this._update();

Also, when constructing the highlighter, it's still hidden until show is called, so why call update here?

@@ +2713,5 @@
> +
> +    return container;
> +  },
> +
> +  // 

nit: trailing whitespace and empty line comment.

@@ +2715,5 @@
> +  },
> +
> +  // 
> +  _update: function() {
> +

nit: unnecessary empty line here.

@@ +2716,5 @@
> +
> +  // 
> +  _update: function() {
> +
> +    let body = this.win.document.body,

So, for now, highlighters that go into the canvasFrame only work in HTML windows, so getting the <body> will always work, but this is only temporary. We want highlighters to work everywhere, even in XUL windows (see bug 1094959).
So when that is fixed, getting the body will fail.

@@ +2720,5 @@
> +    let body = this.win.document.body,
> +        html = this.win.document.documentElement;
> +
> +    // get the size of the content document despite the compatMode
> +    let width =  Math.max(body.scrollWidth, body.offsetWidth, 

nit: trailing whitespace

@@ +2722,5 @@
> +
> +    // get the size of the content document despite the compatMode
> +    let width =  Math.max(body.scrollWidth, body.offsetWidth, 
> +                       html.clientWidth, html.scrollWidth, html.offsetWidth);
> +    let height =  Math.max(body.scrollHeight, body.offsetHeight, 

nit: trailing whitespace

@@ +2751,5 @@
> +    this.hide();
> +
> +    this._cancelUpdate();
> +
> +    events.off(this.markup.tabActor, "navigate", this._onNavigate);

You haven't subscribed to that event in the code, and there's no _onNavigate method here.

@@ +2760,5 @@
> +    this.win.removeEventListener("scroll", this);
> +    this.win.removeEventListener("pagehide", this);
> +
> +    this.markup.destroy();
> +

Please also nullify this.win and this.tabActor here.

@@ +2766,5 @@
> +  },
> +
> +  show: function() {
> +    this.markup.removeAttributeForElement(this.ID_CLASS_PREFIX + "elements",
> +      "hidden");

You should start the update loop here, in show, rather than in the constructor.
And you should also use setIgnoreLayoutChanges to avoid reflows to be detected by the devtools here.
Also, you have added the nice getElement shortcut, so you should probably use it here to avoid having to use the prefix:
this.getElement("elements").removeAttribute("hidden");

@@ +2772,5 @@
> +
> +  hide: function() {
> +    this.hideLabel();
> +
> +    this.markup.setAttributeForElement(this.ID_CLASS_PREFIX + "elements",

Same here, use setIgnoreLayoutChanges.
And cancel the rAF loop.
Also, you have added the nice getElement shortcut, so you should probably use it here to avoid having to use the prefix:
this.getElement("elements").setAttribute("hidden", "true");

@@ +2786,5 @@
> +
> +    coords.w = w;
> +    coords.h = h;
> +
> +    this.updatePaths();

You should use setIgnoreLayoutChanges around updatePaths and updateLabel.
Either you use it here and in setCoords, or you use it inside updatePaths and updateLabel instead.
Basically, anything that modifies the position or size of something inside the canvasFrame should be surrounded by a pair of setIgnoreLayoutChanges calls.

@@ +2790,5 @@
> +    this.updatePaths();
> +    this.updateLabel();
> +  },
> +
> +  setCoords: function(x, y, w, h) {

setSize and setCoords seem very similar, it looks like you could just make setSize call setCoords with the right arguments.

@@ +2805,5 @@
> +
> +  updatePaths: function() {
> +    let coords = this[measuringCoords];
> +    let { x, y, w, h } = coords;
> +    let dir = `M0 0 L${w} 0 L${w} ${h} L0 0 L0 ${h} L ${w} ${h}`;

nit: everywhere else than in the last command, you've not put a space between L and the coordinate, so you should do this there too.

@@ +2809,5 @@
> +    let dir = `M0 0 L${w} 0 L${w} ${h} L0 0 L0 ${h} L ${w} ${h}`;
> +
> +    this.getElement("white-path").setAttribute("d", dir);
> +    this.getElement("black-path").setAttribute("d", dir);
> +  

nit: trailing whitespace

@@ +2829,5 @@
> +    let d = Math.sqrt(w * w + h * h).toFixed(2);
> +
> +    label.setTextContent(`W: ${Math.abs(w)} px
> +                          H: ${Math.abs(h)} px
> +                          ↘: ${d}px`);

Just a thought, it might be nice to display a different diagonal arrow depending on the actual diagonal direction, to match.

@@ +2833,5 @@
> +                          ↘: ${d}px`);
> +
> +    let isGoingLeft = w < scrollX;
> +    let isExceedingLeftMargin = x - MEASURING_LABEL_WIDTH < scrollX;
> +    let isExceedingRightMarging = x + MEASURING_LABEL_WIDTH + MEASURING_LABEL_MARGIN > innerWidth + scrollX;

s/Marging/Margin

@@ +2839,5 @@
> +
> +    if ((isGoingLeft && !isExceedingLeftMargin) || isExceedingRightMarging) {
> +      x -= MEASURING_LABEL_WIDTH;
> +    } else {
> +      x += MEASURING_LABEL_MARGIN; 

nit: trailing whitespace

@@ +2851,5 @@
> +  updateViewport: function() {
> +    let { scrollX, scrollY } = this.win;
> +    let { documentWidth, documentHeight } = this[measuringCoords];
> +
> +    this.getElement("root").setAttribute("style", 

nit: trailing whitespace
Also setIgnoreLayoutChanges here.

@@ +2858,5 @@
> +       transform: translate(${-scrollX}px,${-scrollY}px)`);
> +  },
> +
> +  showLabel: function() {
> +    this.getElement("label").removeAttribute("hidden");

Also setIgnoreLayoutChanges here.

@@ +2862,5 @@
> +    this.getElement("label").removeAttribute("hidden");
> +  },
> +
> +  hideLabel: function() {
> +    this.getElement("label").setAttribute("hidden", "true");

Also setIgnoreLayoutChanges here.

@@ +2872,5 @@
> +
> +    switch (event.type) {
> +      case "mousedown":
> +        if (event.button) return;
> +    

nit: trailing whitespace

@@ +2878,5 @@
> +        x = event.clientX + scrollX;
> +        y = event.clientY + scrollY;
> +
> +        this.win.addEventListener("mouseup", this);
> +        this.win.addEventListener("mousemove", this);

Same remark about adding an event listening on the content window here.

@@ +2880,5 @@
> +
> +        this.win.addEventListener("mouseup", this);
> +        this.win.addEventListener("mousemove", this);
> +
> +        //this.getElement("root").removeAttribute("style");

nit: commented out code should be removed.

@@ +2916,5 @@
> +        this.destroy();
> +        break;
> +    }
> +  }
> +} 

nit: trailing whitespace and missing ;

Comment 24

[Patrick Brosset <:pbro>]

I'd like other people's point of view on the style of the tool, so pinging Brian and Jeff on this. Here's my comment on the topic:

> - I admit not being a huge fan of the animated stroke borders (I know this was inspired by photoshop). I see 3 somewhat problematic things with them:
>   - I find the animation a little bit distracting, but that may be just me, also, when measuring a very small distance, it makes it kind of hard to see the rectangle,
>   - in photoshop, the tool is a selection tool that allows much more than just measuring, and so I think the animation works there because it emphasizes that the drawn rectangle inside the borders can be manipulated,
>   - it introduces a look & feel of its own, that is nowhere else in the devtools, so it lacks consistency with the rest of the tools.
> I'd like other people to look at this because this is a very subjective topic, so I've made a quick screencast that others can look at and give feedback on: https://dl.dropboxusercontent.com/u/714210/measuring-tool-2.mp4

Brian, Jeff, could take a look at that video and let us know your thoughts about it?

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8586029 [details] [diff] [review]
measuring tool command

Clearing the flag for now as I was not able to apply the patch cleanly, it needs rebasing.
Matteo, can you ping me for review again after rebasing?

Comment 26

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
...
> >   - it introduces a look & feel of its own, that is nowhere else in the devtools, so it lacks consistency with the rest of the tools.

This to me is the key issue - for consistency I think the rect should be a non-animated dashed line with the same color as the ones used in the normal highlighter.

Comment 27

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)

> - Minor detail: sometimes, when the rectangle is thin, the top border seems
> to stick out of it, by 1 or 2 pixels, see
> https://dl.dropboxusercontent.com/u/714210/measuring-tool-1.png

That's interesting. It is happen when the content is zoomed? Or it's independent from that?
I would say that is somehow related to the fact that I'm using one shape to draw both the rectangle and the diagonal inside. Not sure why it doesn't close properly, since the size specified are the same. I could probably try to fix using two shape (a rect for the rectangle, and a line for the diagonal); however it could end up in a similar situation (the line won't overlap precisely to the rectangle).

> - I admit not being a huge fan of the animated stroke borders (I know this
> was inspired by photoshop). I see 3 somewhat problematic things with them:
>   - I find the animation a little bit distracting, but that may be just me,
> also, when measuring a very small distance, it makes it kind of hard to see
> the rectangle,
>   - in photoshop, the tool is a selection tool that allows much more than
> just measuring, and so I think the animation works there because it
> emphasizes that the drawn rectangle inside the borders can be manipulated,
>   - it introduces a look & feel of its own, that is nowhere else in the
> devtools, so it lacks consistency with the rest of the tools.
> I'd like other people to look at this because this is a very subjective
> topic, so I've made a quick screencast that others can look at and give
> feedback on: https://dl.dropboxusercontent.com/u/714210/measuring-tool-2.mp4

The animated stroke borders gives one big advantage, IMVHO, that solid colors doesn't:

 It's visible despite the background color we have behind the tool: if we're using just a solid color, and the background we're working on has a similar color, the tool is basically invisible.

I'd like to keep the animated stroke borders, at least during the dragging phase – that was the original plan anyway – and remove once the user action is ended. That also gives to the animated borders another advantage; it clearly indicate that the action is "in progress", that the shape is "alive": once the user's action is end, it will be displayed "still", with the same solid color as the others tools.
I think this will also address the consistency issue; and the issue where we have a "line rectangle" you described.

> - Zooming in/out the page should not zoom in/out the rectangle and label,
> and the rectangle's position should not change.

I agree about the label, I'm not sure about the position. I think we have two options, here:
The first, the rectangle keeps the relative size and position like now. It means, if I select an area starting from 320x200 px, and I end the selection at 420x300 (a 100x100 rectangle) and then I zoom, then the rectangle is moved and resized following the zoom. So that, if we have the rulers active, the top left corner of the rectangle is still pointing to 320x200, even if zoomed, and the bottom right is still pointing to 420x300.
This is how it works now, because the rectangle is affected by the zoom, and this is how I personally think it should works: in this way, I can zoom a page content to make a more precise selection, and then zoom out, keeping the right values. This is how usually the tools works in Photoshop and similar.
To make more clear what I mean, you can try to use the measuring tools on this page: https://dl.dropboxusercontent.com/u/5374957/highlighters/rules-4.html, the one used for the prototype of the rulers, and select the red or blue rectangle: it's easier if you zoom in.

The other option, is makes the whole tool unaffected by the zoom, so that the rectangle's position won't change when zoom in/out as you said. But honestly, I don't see the advantage of doing that, it seems to me counterintuitive. 

> - Being able to move the rectangle would be great, but I think you intend to
> work on this in a next version, right?

Right; I have filed a couple of follow-up bugs for that: one is for changing the position of the entire area (bug 1152316), and the other is for resizing the area (bug 1152321).

> this. After a quick investigation, I think this is related to reflows caused
> by your highlighter. Indeed, highlighters are part of the content document,
> and may cause reflows when they move/resize/whatever. And because we have an
> actor that listens for reflows in the content document, it detects the ones
> caused by the highlighter too.

It's probably caused by the HTML label; I'll check.

> > +@keyframes -moz-measuring-tool-highlighter-dash {
> 
> The problem with adding this animation is that it's now available to web
> content. Of course the name is long, complex and starts with -moz, so it
> could, in theory, be alright, but since we went to great care about hiding
> the other rules (by prefixing with the chrome-only pseudo class
> :-moz-native-anonymous), I'm not so sure adding this not-chrome-only
> keyframes animation name would be ok.

Unfortunately I didn't find any other way to do so, any suggestion are more than welcome!
I think we should try to find resources to fix the <style> issues in the anonymous content; or even fix bug 992988 that would also useful for that as well.

> > +  this.win.addEventListener("pagehide", this);
 
> You're adding event listeners on a content window here, so you're impacting
> the content in a way that will be visible to the user. For instance, if you
> open the inspector and click on the (ev) icon next to the <html> node,
> you'll see these event listeners there, as shown here:
> https://dl.dropboxusercontent.com/u/714210/measuring-tool-4.png

I did the same in the rulers, and because you didn't raise a yellow flag there I thought OK – I was thinking that in such case it would fall back to the `tabActor.window` anyway – see the `getPageListenerTarget`.
I was obviously wrong, so I filed bug 1152330 to fix that too.

> > +                          ↘: ${d}px`);

> Just a thought, it might be nice to display a different diagonal arrow
> depending on the actual diagonal direction, to match.

I thought the same, but unfortunately the utf-8 characters for such arrows are not consistent (see: http://www.fileformat.info/info/unicode/block/arrows/utf8test.htm). We could using a CSS transformation to do so, but then the label can't be just a text container; we need more sub-elements, and I thought that the effort was more than the actual benefit.

If you think it should be something to have anyway, I can add in a follow-up bug, let me know.

Thanks a lot for the review; I'll submit the new patch once I've addressed all your comments!

Comment 28

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

I made a small video about how it works and it looks now; there is still a small thing to fix – I need to hide the label when I left the document's area; you'll see – but I'd like to get a feedback from you guys:

https://dl.dropboxusercontent.com/u/5374957/measuring-tool.mp4

What do you think?

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> I made a small video about how it works and it looks now; there is still a
> small thing to fix – I need to hide the label when I left the document's
> area; you'll see – but I'd like to get a feedback from you guys:
> 
> https://dl.dropboxusercontent.com/u/5374957/measuring-tool.mp4
> 
> What do you think?

Getting a 404 on the video

Comment 30

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

I also added Brian, but probably 'cause the pending needinfo it wasn't added again – and it makes sense.

(The video also shake a bit sometimes, probably something went wrong during the conversion to mp4, but the main part should be OK)

Comment 31

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

(In reply to Gabriel Luong (:gl) from comment #29)

> Getting a 404 on the video

Try now, probably dropbox was a bit slower to upload the update. :)

Comment 32

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28)
> I made a small video about how it works and it looks now; there is still a
> small thing to fix – I need to hide the label when I left the document's
> area; you'll see – but I'd like to get a feedback from you guys:
> 
> https://dl.dropboxusercontent.com/u/5374957/measuring-tool.mp4
> 
> What do you think?

I think it's really close, but

> I'd like to keep the animated stroke borders, at least during the dragging
> phase – that was the original plan anyway – and remove once the user action
> is ended. That also gives to the animated borders another advantage; it
> clearly indicate that the action is "in progress", that the shape is
> "alive": once the user's action is end, it will be displayed "still", with
> the same solid color as the others tools.
> I think this will also address the consistency issue; and the issue where we
> have a "line rectangle" you described.

I think it's still a problem - I don't like the animation in particular. I do agree generally that the measure tool should be visible during the drag, I just wish you had another option for how you ensure that visibility.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #32)
> > I'd like to keep the animated stroke borders, at least during the dragging
> > phase – that was the original plan anyway – and remove once the user action
> > is ended. That also gives to the animated borders another advantage; it
> > clearly indicate that the action is "in progress", that the shape is
> > "alive": once the user's action is end, it will be displayed "still", with
> > the same solid color as the others tools.
> > I think this will also address the consistency issue; and the issue where we
> > have a "line rectangle" you described.
> 
> I think it's still a problem - I don't like the animation in particular. I
> do agree generally that the measure tool should be visible during the drag,
> I just wish you had another option for how you ensure that visibility.

My 2 cents - the animation doesn't bother me.  It is familiar due to lots of other programs that use something similar.

Can someone help me understand the exact use case(s) we are targeting for v1 of this tool?  Here are a few notes after watching the videos:

1) Switching to a highlighter overlay once selected does make it more clear that you can't do photoshop-like things with the element.  However going to a pretty opaque color could block some detail and get in the way of the element that you are trying to line up.
2) The sizes box that shows the dimensions / coords in the corner after you drag a region seems to go away once you move away from it, and I can't tell if you can get it back. 
3) In Comment 24 Patrick mentions that the animation can get in the way when doing very small measurements - I haven't tried that yet so I can't comment on it

Comment 34

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

(In reply to Jeff Griffiths (:canuckistani) from comment #32)

> > I'd like to keep the animated stroke borders, at least during the dragging
> > phase – that was the original plan anyway – and remove once the user action
> > is ended. That also gives to the animated borders another advantage; it
> > clearly indicate that the action is "in progress", that the shape is
> > "alive": once the user's action is end, it will be displayed "still", with
> > the same solid color as the others tools.
> > I think this will also address the consistency issue; and the issue where we
> > have a "line rectangle" you described.
> 
> I think it's still a problem - I don't like the animation in particular. I
> do agree generally that the measure tool should be visible during the drag,
> I just wish you had another option for how you ensure that visibility.

The animation was chosen because we don't have anything like that in devtools currently, so we're not breaking anything in term of consistency; instead, it's familiar to most of the designer around, 'cause most of the programs (e.g. Photoshop) does the same. I honestly think it's a good ux, following the "standard", here.
Plus, as Brian noted, removing the animation after the selection part, it makes clear that we're not in the same domain of other application, therefore the user won't expect to perform similar operations – I have to say, before that I was tempt to press cmd+C to "copy" portion of the screen, like I was using Photoshop. :)

(In reply to Brian Grinstead [:bgrins] from comment #33)

> Can someone help me understand the exact use case(s) we are targeting for v1
> of this tool?

We want a tool to measure freely area and distance between two points, in a web page. For example, the space between two elements; or how big is a specific area.

For that reason…

> 1) Switching to a highlighter overlay once selected does make it more clear
> that you can't do photoshop-like things with the element.  However going to
> a pretty opaque color could block some detail and get in the way of the
> element that you are trying to line up.

…I also don't think it's a big issue the opacity; we're not really interested in the content – especially not after the selection – and sometimes also the label is overlapping to the selection's area (something I was discussing with Patrick, see comment 23).

I added the opacity for two reasons, mainly:
- Provide a consistent look & feel across the highlighters
- Make sure the diagonal is still visible without animation – as noted in comment 27, with just solid color we could have it mostly invisible; we can clearly see that if we select an area inside the Firefox's Nightly logo, that has similar colors.

We can definitely remove the background color from the tool, having it always transparent, I just not sure if it's worthy: in my opinion, if you're interested in the content, is during the selection phase, just to take the exact measure. And in some cases you're not interested at all – e.g. when you want to take the distances between two elements.

> 2) The sizes box that shows the dimensions / coords in the corner after you
> drag a region seems to go away once you move away from it, and I can't tell
> if you can get it back.

Good catch; it was because I added the displaying of X / Y before upload the video, so yeah, there is still some stuff to refine; also I'm not sure we want have it. The initial idea was to display the X / Y before the selection, 'cause it could help – and most of the application do the same.
However, as you saw, that brings the problem that after the selection, the label that should keep persistent – the one with the measurement – becoming again the label for the X / Y.
 
We could either remove such functionality – keep as it was before, no X / Y label – or create an additional label – but then I would use a different "style" for that – that is always displaying when is not in the selection phase.

> 3) In Comment 24 Patrick mentions that the animation can get in the way when
> doing very small measurements - I haven't tried that yet so I can't comment
> on it

I think I've fixed that – "very small measurements" was basically 0 in one of the axis.

Comment 35

[Patrick Brosset <:pbro>]

It would help continue the discussion if you could either provide a try build that others can test, or upload the new patches you used in the last video.
I think I have to try the tool for myself before commenting any further on the look/feel discussion.

Comment 36

[Patrick Brosset <:pbro>]

Looking at that last video, I like the fact that once dropped, the style changes to something more consistent and the animation goes away.
I'm not so sure about the horizontal/vertical guides that extend to the full width/height though, but as I said, I'd have to try that for myself.

One thing occurred to me: the WebDeveloper addon has a measuring tool, and they chose to display a semi-transparent overlay over the whole viewport, and dragging the measuring tool would "cut off" this overlay in a way that the area of the page that is measured doesn't have the overlay on top of it.
I'll attach a screenshot.

Comment 37

[Patrick Brosset <:pbro>]

Attachment: webdeveloper-measure.gif

Comment 38

[Patrick Brosset <:pbro>]

Adding the [devtools-ux] flag so the bug shows up on our UX radar. I don't think we should block this bug on a UX review, but it would be nice having one at some stage, even after the bug is resolved.

Comment 39

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Adding some priority to a first set of bugs. :)

Comment 40

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

Attachment: Add a measurement tool

Comment 41

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

Attachment: Add a measurement tool

Comment 42

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

Comment on attachment 8665170 [details] [diff] [review]
Add a measurement tool

I should definitely find a way to review what `git bz` is attaching before it actually attached…

Comment 43

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

Attachment: Add a measurement tool

Comment 44

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

Comment on attachment 8665171 [details] [diff] [review]
Add a measurement tool

I'd like to add more unit tests, but I'd like also to check if this approach is OK - for the unit testing – and start to land this stuff. :)

Here the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=823d59c22dae

Comment 45

[Patrick Brosset <:pbro>]

Comment on attachment 8665171 [details] [diff] [review]
Add a measurement tool

Review of attachment 8665171 [details] [diff] [review]:
-----------------------------------------------------------------

I've downloaded the try build and played with it. Overall it seems really nice. A very good first version that is worth landing I think, and later add more features to.

It would however be great to have Helen's point of view on this. I'm going to flag her for a ui-review here.

@Helen: you can download the build from here : http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-823d59c22dae/try-macosx64/
Then open the devtools, go in the settings panel, scroll down, activate the "Measure a portion of the page" Toolbox Button.
This should add a new button to the devtools toolbar which you can click to start measuring things in the page.
I think that the ui-related points we discussed in the past about this were: the look and feel of the tool while dragging (animated dashed borders, that look like photoshop), and the look and feel of the tool after dragging (that looks more like our normal devtools element highlighter, with the guides).

On that note, The image for this new toolbar button doesn't show up for me. The button is there, but it's blank.

A couple of general notes before digging into the code:
- The icon you added is PNG. My impression is that, going forward, we wanted to use SVGs instead. @Brian, can you confirm?
- I'm not too sure about the way the position label (the one that follows the mouse around) is displayed work. The text seemed to be too contrasty for me and I would have expected the background to be the same as the other (position) label, i.e. not semi-transparent. But that might just be me.
- Also, that label tends to lag behind a lot when the mouse moves quickly. At first I thought that was because the label was only being moved on requestanimationFrame, but it's not. Can you take a look at what's causing the lag?

I really like what you've done with the tests and the head.js helper function, but I'd like to see a couple more tests for the scroll and zoom features. Can you take a look at how these could be implemented?

I don't have many comments about the code itself. It's easy to read.

::: devtools/client/commandline/test/browser_cmd_measure.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests the various highlight command parameters and options

Well, except it doesn't have any parameters or options. This should be rephrased.

::: devtools/client/inspector/test/head.js
@@ +483,5 @@
> +
> +/**
> + * Encapsulate some common operations for highlighter's tests, to have
> + * the tests cleaner, without exposing directly `inspector`, `highlighter`, and
> + * `testActor` if no needed.

s/if no needed/if not needed

@@ +490,5 @@
> + *    The URL to open for the inspector
> + * @param {String} type
> + *    The highlighter's type
> + */
> +function* getHighlighterHelperFor(url, type) {

I really like this new function (btw I'm sure a lot of highlighter tests could be made to use this, could you file a bug so we don't forget). My only concern is with the name. It doesn't imply that the inspector panel will be opened. Would openInspectorAndGetHighlighter be too long?
That's not a big deal anyway, feel free to ignore.

@@ +503,5 @@
> +    set prefix(value) {
> +      prefix = value;
> +    },
> +
> +    show: function* (selector = ":root") {

nit: I think I've seen other places in the codebase where this was formatted like:
function*(
no space between * and (

@@ +524,5 @@
> +        prefix + id, name, highlighter);
> +    },
> +
> +    synthesizeMouse: function* (options) {
> +      testActor.synthesizeMouse(options);

Do we want to wait for this to be done here? i.e. use `yield`?

::: devtools/server/actors/highlighters.css
@@ +275,5 @@
>  }
> +
> +/* Measuring Tool highlighter */
> +
> +@keyframes -moz-measuring-tool-highlighter-dash {

I still think that this is dangerous at this stage. The risks are very limited, but I'm tempted to say, let's get rid of the animation until we have <style scoped> working in native anonymous content.
This also depends on what Helen says in the ui-review. If this turns out to be a key ui/ux feature that we must keep, then I'd be willing to reconsider.

::: devtools/server/actors/highlighters/measuring-tool.js
@@ +104,5 @@
> +      prefix
> +    });
> +
> +    let g = createSVGNode(window, {
> +      nodeType: "g",

Can you explain (not just to me, in a comment in the code), why is this <g> group needed here?

@@ +164,5 @@
> +
> +    return container;
> +  },
> +
> +  //

nit: please remove //

@@ +175,5 @@
> +
> +    let { documentElement } = window.document;
> +
> +    let width = Math.max(documentElement.clientWidth,
> +                        documentElement.scrollWidth,

nit: indentation is off by 1 here

::: toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
@@ +1619,5 @@
>  # tooltip of button in devtools toolbox which toggles the rulers.
>  rulersTooltip=Toggle rulers for the page
> +
> +# LOCALIZATION NOTE (measureDesc) A very short description of the
> +# 'measure' command. See highlightManual for a fuller description of what

s/highlightManual/measureManual

Comment 46

[Patrick Brosset <:pbro>]

> - The icon you added is PNG. My impression is that, going forward, we wanted
> to use SVGs instead. @Brian, can you confirm?

Comment 47

[Tim Nguyen :ntim]

Comment on attachment 8665171 [details] [diff] [review]
Add a measurement tool

Review of attachment 8665171 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/toolbars.inc.css
@@ +812,5 @@
>    #command-button-rulers > image {
>      background-image: url("chrome://devtools/skin/themes/images/[e-mail]");
>    }
> +
> +  #command-button-rulers > image {

nit: this should be #command-button-measure

Comment 48

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #46)
> > - The icon you added is PNG. My impression is that, going forward, we wanted
> > to use SVGs instead. @Brian, can you confirm?

Yes, SVG is the future for our icons.   I believe Helen is in the process of working through some of the existing ones.  Given the CSS here (just setting a background image) I think it'd be OK to start by switching this new image to SVG.  Helen, can you please take a pass at this icon and convert it to SVG: https://bugzilla.mozilla.org/attachment.cgi?id=8539494?

Comment 49

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: new-opacity.jpg

Opacity of 10%

Comment 50

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: strange-type.png

text shadow???

Comment 51

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #45)
> On that note, The image for this new toolbar button doesn't show up for me.
> The button is there, but it's blank.
Same here 

Comment 52

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #45)
> - I'm not too sure about the way the position label (the one that follows
> the mouse around) is displayed work. The text seemed to be too contrasty for
> me and I would have expected the background to be the same as the other
> (position) label, i.e. not semi-transparent. But that might just be me.
For me the transparency isn't the problem, it's that something strange is happening with the type. I've uploaded a screenshot; looks like it has some sort of text-shadow?

I also think that you can go lighter with the opacity of the highlighted area so it's easier to see what's underneath—probably can go as low as 10%, I added a screenshot of what that sort of looks like (got lazy about drawing the other lines). 

It would be cool if there was a way to get out of the tool other than clicking the tool in the toolbar again—I would recommend 4rd click if they're clicking in the same spot over and over again, since currently it goes 1st click: start + drag, 2nd click: clears current box, 3rd click could be creating another shape. 

Also, I notice the tool stays active if the devtools are closed, should we disable it when that happens?

@bgrins: yep, sure thing! I notice this is currently the ruler icon, is that functionality going to be hidden behind a key command? I know that Photoshop does "Cmd/Ctrl+R"...

That was a lot of stuff, but it looks great!

Comment 53

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

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #45)

> On that note, The image for this new toolbar button doesn't show up for me.
> The button is there, but it's blank.

During my "rebasing" I did manually some file, and I forgot, As Tim pointed out on comment 47, to replace the proper CSS rule. Now is fixed. Thanks Tim!

> - I'm not too sure about the way the position label (the one that follows
> the mouse around) is displayed work. The text seemed to be too contrasty for
> me and I would have expected the background to be the same as the other
> (position) label, i.e. not semi-transparent. But that might just be me.

I'm not happy with that either: In most of app, the coordinate are displayed somewhere else – e.g. toolbar – but we don't have such UI at the moment, and it's important that such tool display the current coordinate from where you start to select. About the color, the problem was that using the same exact style of the other label, it would be confusing if the labels are overlapping – you can make a quick try changing the CSS.

> - Also, that label tends to lag behind a lot when the mouse moves quickly.
> At first I thought that was because the label was only being moved on
> requestanimationFrame, but it's not. Can you take a look at what's causing
> the lag?

I know, but I wasn't able to find what is causing such lag. Also, I don't want spend too much time on it 'cause in v2 the overall structure will change, and we shouldn't have this issue. 

> I really like what you've done with the tests and the head.js helper
> function, but I'd like to see a couple more tests for the scroll and zoom
> features. Can you take a look at how these could be implemented?

I just filed Bug 1208142.

> > +function* getHighlighterHelperFor(url, type) {
> 
> I really like this new function (btw I'm sure a lot of highlighter tests
> could be made to use this, could you file a bug so we don't forget). My only
> concern is with the name. It doesn't imply that the inspector panel will be
> opened. Would openInspectorAndGetHighlighter be too long?
> That's not a big deal anyway, feel free to ignore.

I'm not sure about the name either, bug I thought that put both `inspector` and `highlighter` in the name would be too much.
 
> nit: I think I've seen other places in the codebase where this was formatted
> like:
> function*(
> no space between * and (

We have actually both code style, but I don't mind to follow one or another. Maybe we could add as eslinter rule?

> > +    synthesizeMouse: function* (options) {
> > +      testActor.synthesizeMouse(options);
> 
> Do we want to wait for this to be done here? i.e. use `yield`?

Oh, good catch! Thanks!

> > +@keyframes -moz-measuring-tool-highlighter-dash {
> 
> I still think that this is dangerous at this stage. The risks are very
> limited, but I'm tempted to say, let's get rid of the animation until we
> have <style scoped> working in native anonymous content.

I don't think it would happen soon. I would love have some platform guy that could work on this.

> This also depends on what Helen says in the ui-review. If this turns out to
> be a key ui/ux feature that we must keep, then I'd be willing to reconsider.

OK.

Comment 54

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

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #52)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #45)
> > - I'm not too sure about the way the position label (the one that follows
> > the mouse around) is displayed work. The text seemed to be too contrasty for
> > me and I would have expected the background to be the same as the other
> > (position) label, i.e. not semi-transparent. But that might just be me.

> For me the transparency isn't the problem, it's that something strange is
> happening with the type. I've uploaded a screenshot; looks like it has some
> sort of text-shadow?

Yes, there is a text-shadow; otherwise I felt like the text color (black) on dark background, wasn't clear enough, especially if it's hovering a black element – the black element will makes the label's text unreadable.
I'm not super happy with that solution, but as mentioned in my previous comment, I didn't find a nice place where to put this information. In v2 I'm experimenting with a different approach – but in the meantime, I'd like to land the v1.
But any suggestion here is more than welcome!

> I also think that you can go lighter with the opacity of the highlighted
> area so it's easier to see what's underneath—probably can go as low as 10%,
> I added a screenshot of what that sort of looks like (got lazy about drawing
> the other lines).

The opacity I used is taken by the other highlighters – e.g. when you inspect a node from devtools. I did that to be consistent; shall I change just this highlighter's opacity, or also the others?
 
> It would be cool if there was a way to get out of the tool other than
> clicking the tool in the toolbar again—I would recommend 4rd click if
> they're clicking in the same spot over and over again, since currently it
> goes 1st click: start + drag, 2nd click: clears current box, 3rd click could
> be creating another shape.

So, currently the 1st click create the selection, if the selection is 0x0 is not displayed. It means that the 2nd click can either clears the current box, than also create a new shape, if the user stars to drag.

Notice also that, since the context menu here is mostly useless – 'cause there is the tool that blocks everything – we could also get out of the tool using the right click instead.

> Also, I notice the tool stays active if the devtools are closed, should we
> disable it when that happens?

That should be OK, since you can also call such tools (rulers, measuring tool) even from gcli (Tools > Web Developer > Developer Toolbar) – even if the inspector is not opened.

> That was a lot of stuff, but it looks great!

Thanks! Could you also give your opinion about the animation when you're selecting? I did that to mimic most of the tools around here (e.g. Photoshop) and give a familiar look & feel during the dragging, but Patrick is unsure about it (see Comment 45). So, we'll keep it if it's a key ui/ux feature in this scenario.
I can provide to you a patch where we don't have such animation, to evaluate both if you want, and see how it feels.

Comment 55

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #54)
> Yes, there is a text-shadow; otherwise I felt like the text color (black) on
> dark background, wasn't clear enough, especially if it's hovering a black
> element – the black element will makes the label's text unreadable.
> I'm not super happy with that solution, but as mentioned in my previous
> comment, I didn't find a nice place where to put this information. In v2 I'm
> experimenting with a different approach – but in the meantime, I'd like to
> land the v1.
> But any suggestion here is more than welcome!
My only suggestion would be to make the type white and give the background a high opacity just to make it readable for when you ship (right now it isn't super legible yet).

> The opacity I used is taken by the other highlighters – e.g. when you
> inspect a node from devtools. I did that to be consistent; shall I change
> just this highlighter's opacity, or also the others?
Oh whoa, you're so right! Never mind. If that changes it should probably be everything that changes, so never mind, you're right to stay consistent.

> Notice also that, since the context menu here is mostly useless – 'cause
> there is the tool that blocks everything – we could also get out of the tool
> using the right click instead.
I think it's worth giving this some thought for V2—right now getting out of it feels more difficult than it should be.

> Thanks! Could you also give your opinion about the animation when you're
> selecting? I did that to mimic most of the tools around here (e.g.
> Photoshop) and give a familiar look & feel during the dragging, but Patrick
> is unsure about it (see Comment 45). So, we'll keep it if it's a key ui/ux
> feature in this scenario.
> I can provide to you a patch where we don't have such animation, to evaluate
> both if you want, and see how it feels.
So the animation feels like exactly what is in Photoshop, so it's more of a UI than a UX problem—so, personally, I prefer it without the animation (à la Sketch) but yeah, that's just a personal preference. I guess you could argue that the measurement tool and the Photoshop marquee tool aren't exactly the same thing, but I don't think that nuance will confuse anyone using this...

Comment 56

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

Attachment: Add a measurement tool

Comment 57

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

Comment on attachment 8665671 [details] [diff] [review]
Add a measurement tool

I should have address both comments from code and ui/ux; let me know if you think it's better now.

I also agreed that we should lower the overall opacity of our highlighters, as Helen suggested, but in case we should do that in a separate bug.

Until there aren't the new icons, I left the PNG version of them. IMVHO they shouldn't be a blocker to land this patch; we can always add them in a follow up patch.

Patrick, I'm working on the additional tests, but I will add them in a separate patch – to this bug.

Comment 58

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: l-square.svg

~svgs~

Comment 59

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: ruler.svg

~svgs~

Comment 60

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #58)
> Created attachment 8666023 [details]
> l-square.svg
> 
> ~svgs~

At least with the current format we use for the command buttons, I think these will need to be sprites with 4 states like so (starting with white-ish and going to blue):  https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/command-console.png.  The svg filter handles inverting this to dark fills for the light theme.

Comment 61

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Brian Grinstead [:bgrins] from comment #60)
> At least with the current format we use for the command buttons, I think
> these will need to be sprites with 4 states like so (starting with white-ish
> and going to blue): 
Wait, meaning even for SVGs???? D:

Comment 62

[Tim Nguyen :ntim]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #61)
> (In reply to Brian Grinstead [:bgrins] from comment #60)
> > At least with the current format we use for the command buttons, I think
> > these will need to be sprites with 4 states like so (starting with white-ish
> > and going to blue): 
> Wait, meaning even for SVGs???? D:
Yeah, until all command button icons are converted to SVG.
Anyway, it shouldn't be too hard to make a sprite, you can simply duplicate the paths then translate them.

Comment 63

[Brian Grinstead [:bgrins]]

Discussed with Helen, and we are going to handle the SVG conversion separately to make this bug easier.  Feel free to proceed with the existing png and @2x.png.

Comment 64

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

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #53)

> > I really like this new function (btw I'm sure a lot of highlighter tests
> > could be made to use this, could you file a bug so we don't forget). My only
> > concern is with the name. It doesn't imply that the inspector panel will be
> > opened. Would openInspectorAndGetHighlighter be too long?
> > That's not a big deal anyway, feel free to ignore.

> I'm not sure about the name either, bug I thought that put both `inspector`
> and `highlighter` in the name would be too much.

I was thinking, what about otherwise something like:

  let helper = yield openInspectorForURL(TEST_URL)
                       .then(getHighlighterHelperFor(HIGHLIGHTER_TYPE));

It would be less magical and more clear, keep the two operation separate (open inspector, get the helper for the highlighter)?

Comment 65

[Patrick Brosset <:pbro>]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #64)
>   let helper = yield openInspectorForURL(TEST_URL)
>                        .then(getHighlighterHelperFor(HIGHLIGHTER_TYPE));
> 
> It would be less magical and more clear, keep the two operation separate
> (open inspector, get the helper for the highlighter)?
Looks good to me.

Comment 66

[Patrick Brosset <:pbro>]

Comment on attachment 8665671 [details] [diff] [review]
Add a measurement tool

Review of attachment 8665671 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
About the code changes: I see all my concerns have been addressed, and the code is easy to read. So R+ for this.
I see you've changed the UI too, I like it a lot. I think it would be useful to Helen if you provided a new try build so she can try locally too.
Can you file a bug for investigating the performance problem that's causing the label to lag behind when the mouse is moved?

::: devtools/server/actors/highlighters.css
@@ +313,5 @@
> +  -moz-user-select: none;
> +  background: hsla(214, 13%, 24%, 0.8);
> +}
> +
> +:-moz-native-anonymous .measuring-tool-highlighter-label-size {

This rule contains ~80% of the same declarations as the label-position. Can you create a measuring-tool-label class that contains all the common stuff?

Comment 67

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #66)
> I see you've changed the UI too, I like it a lot. I think it would be useful
> to Helen if you provided a new try build so she can try locally too.

Alas, I tried my best to apply your patch, and even though I'm seeing your commit in my log I'm not seeing your changes... So it would be much appreciated if you could supply a new try build!

Comment 68

[Helen V. Holmes (:helenvholmes) (:✨)]

With some help from :pbro, I too approve of the new changes! It looks really good. I'd love to keep thinking about ways to exit the tool other than clicking on the button that activates it but that probably belongs in a separate enhancement bug.

Comment 69

[Patrick Brosset <:pbro>]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #68)
> I'd love to keep thinking about ways to exit the tool other than
> clicking on the button that activates it but that probably belongs in a
> separate enhancement bug.
Matteo, could/did you file one?
For info, the box-model highlighter (the thing that overlays elements in the page when you use the inspector) has the escape key bound.

Comment 70

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #68)
> With some help from :pbro, I too approve of the new changes! It looks really
> good. I'd love to keep thinking about ways to exit the tool other than
> clicking on the button that activates it but that probably belongs in a
> separate enhancement bug.

It's very common in other tools to bind the 'esc' key for this - however it is *also* common to have the keybinding that enables the tool to be a 'toggle' that also disables it. 

Also: if we do more tools like this that use the highlighter apis, we should make sure they have a common interaction model, obv.

Comment 71

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

Attachment: Add a measurement tool

Comment 72

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5870c6011e64

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5870c6011e64

Comment 74

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on Firefox nightly version 31.0a1 (2014-10-25) 

It's fixed and verified on Latest Nightly
Build ID 	20151002030204
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0

Tested OS- Windows 8.1 32bit

Comment 75

[alex_mayorga]

Verifying per https://bugzilla.mozilla.org/show_bug.cgi?id=1089240#c74

Comment 76

[Mihai Boldan, Desktop QA [:mboldan]]

While testing the measurement tool on Nightly 44.0a1(20151019030227) across platforms [1] I noticed the following:
- it can be closed by the 'measure' command in the Developers Toolbar, or by refreshing the page
- it can be accessed from the Developer Toolbar with the 'measure' command or by clicking the 'Measure a portion of the page' button from the 'Toggle Tools' (only if the button is enabled).
- the measurements are displayed with a white text on a grey background.

Also I've observed that:
- if the page is zoomed in, and the mouse cursor is moved on the right side of the page, the measurement info is hidden (see screenshot 'Zoom in').
- if the page is zoomed out, the measurement info is not displayed correctly. The measurements are displayed on the bottom of the border (see screenshot 'zoom out') - this issue is reproducible only on Windows
- the 'measure' command from the Developer Toolbar is not working, if 'Measure a portion of the page' was previously enabled/disabled. It's reproducible across platforms [1], with and without e10s enabled (the same problem noticed while testing the 'rulers' - #1144163)
- There's some sort of lag displayed for the measure tool's bubble (containing the actual pixel size) when scrolling the page. It has a delay when it's repositioning itself according to the zoom or scroll action in question (see the attached 'Lag' video file). 

[1] Windows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.10.4

Comment 77

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: Zoom in.png

Comment 78

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: Zoom out.png

Comment 79

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: lag.ogv

Comment 80

[Mihai Boldan, Desktop QA [:mboldan]]

If an element is measured and the page is resized, should the measurements modify accordingly?
For example, if an element from the https://www.mozilla.org/en-US/firefox/nightly/firstrun/ page is measured, when the page is resized, the elements are resized and reordered, but the measurements are not modifying as well.

Comment 81

[Patrick Brosset <:pbro>]

(In reply to Mihai Boldan, QA [:mboldan] from comment #80)
> If an element is measured and the page is resized, should the measurements
> modify accordingly?
> For example, if an element from the
> https://www.mozilla.org/en-US/firefox/nightly/firstrun/ page is measured,
> when the page is resized, the elements are resized and reordered, but the
> measurements are not modifying as well.
My 2 cents: I don't think the measurement tool should be resized as well. This tool has no knowledge of the underlying structure of the page. It is not intended as a tool to keep track of various elements' sizes or positions, but rather as a quick way to measure something.

Comment 82

[Sebastian Zartner [:sebo]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Important for web designers to measure parts of a page
[Suggested wording]: Added tool to measure parts of a page
[Links (documentation, blog post, etc)]:

Sebastian

Comment 83

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to FF44 release notes

Comment 84

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

(In reply to Mihai Boldan, QA [:mboldan] from comment #76)

> Also I've observed that:
> - if the page is zoomed in, and the mouse cursor is moved on the right side
> of the page, the measurement info is hidden (see screenshot 'Zoom in').

That's a bug that should be fixed. I can file a bug if you haven't already.

> - if the page is zoomed out, the measurement info is not displayed
> correctly. The measurements are displayed on the bottom of the border (see
> screenshot 'zoom out') - this issue is reproducible only on Windows

ditto.

> - the 'measure' command from the Developer Toolbar is not working, if
> 'Measure a portion of the page' was previously enabled/disabled. It's
> reproducible across platforms [1], with and without e10s enabled (the same
> problem noticed while testing the 'rulers' - #1144163)

As written on bug 1144163, I'm not able to reproduce this behavior; if it's still reproducible for you let me know!

> - There's some sort of lag displayed for the measure tool's bubble
> (containing the actual pixel size) when scrolling the page. It has a delay
> when it's repositioning itself according to the zoom or scroll action in
> question (see the attached 'Lag' video file). 

Unfortunately that's depends by APZ, we can't do much for the moment; see bug 1212020.

Comment 85

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #84)
> (In reply to Mihai Boldan, QA [:mboldan] from comment #76)
> 
> > Also I've observed that:
> > - if the page is zoomed in, and the mouse cursor is moved on the right side
> > of the page, the measurement info is hidden (see screenshot 'Zoom in').
> 
> That's a bug that should be fixed. I can file a bug if you haven't already.

I will log this issue first thing tomorrow.

> 
> > - if the page is zoomed out, the measurement info is not displayed
> > correctly. The measurements are displayed on the bottom of the border (see
> > screenshot 'zoom out') - this issue is reproducible only on Windows
> 
> ditto.

I will log this issue first thing tomorrow.

> 
> > - the 'measure' command from the Developer Toolbar is not working, if
> > 'Measure a portion of the page' was previously enabled/disabled. It's
> > reproducible across platforms [1], with and without e10s enabled (the same
> > problem noticed while testing the 'rulers' - #1144163)
> 
> As written on bug 1144163, I'm not able to reproduce this behavior; if it's
> still reproducible for you let me know!

This issue is still reproducible on Firefox 47.0a2 (2016-03-21).

Here are STR for this issue:
1. Launch FF with a clean profile.
2. Enable Toogle tools (Ctrl+Shift+I).
3. Enable Developer Toolbar (Shift+F2).
4. Open Toolbox Options and enable 'Measure a portion of the page' option.
5. Disable and re-enable the option from step 4.
6. Enable the Measure tool by clicking on the Measure a portion of the page icon.
7. Disable the measure tool by using the 'measure' command in the Developer Toolbar.

ER: The measure tool is disabled.

AR: Nothing happens.

I managed to reproduce this issue on Windows 10 x86, Mac OS X 10.11.1 and on Ubuntu 14.04 x86

Should I log a new bug for this issue?

Comment 86

[Mihai Boldan, Desktop QA [:mboldan]]

I've logged Bug 1258699 and Bug 1258701 for the issues described above.

Comment 87

[Sebastian Zartner [:sebo]]

I've added a note for this tool at https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Extra_tools, but it still requires a separate page explaining in more detail how it works.

I've created bug 1287083 for the issue mentioned in comment 85.

Sebastian

Comment 88

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Dustin, maybe you could help break out the MDN docs for these "extra" tools to separate pages (or whatever seems best to you)?  The disabled by default ones in particular aren't very discoverable, so more docs with pictures would likely help!

Comment 89

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I have heard that Chris Mills might be working with DevTools docs, let's try him instead.

Comment 90

[Chris Mills [:cmills]]

I've documented this feature, see:

https://developer.mozilla.org/en-US/docs/Tools/Measure_a_portion_of_the_page

Let me know if that is OK, or if there is anything else you'd like the see added or changed on the page.

I've also added it to the left hand navigation, under "More tools" so it is a little bit more discoverable (you might not see this on all devtools pages; if not, do a Shift refresh to force reload the page).

Feel free to bug me about any other tools that are not covered.

Comment 91

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #90)
> I've documented this feature, see:
> 
> https://developer.mozilla.org/en-US/docs/Tools/Measure_a_portion_of_the_page
> 
> Let me know if that is OK, or if there is anything else you'd like the see
> added or changed on the page.
> 
> I've also added it to the left hand navigation, under "More tools" so it is
> a little bit more discoverable (you might not see this on all devtools
> pages; if not, do a Shift refresh to force reload the page).

Thanks for working on this, it's great to have.  In addition to the nav bar, it is worth also adding to the "More Tools" section on the main page[1]?  I guess it's a bit tricky that the section and the matching part of the nav bar are maintained separately...

I don't have a strong opinion, so whatever seems best to you.

[1]: https://developer.mozilla.org/en-US/docs/Tools

Comment 92

[Chris Mills [:cmills]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #91)
> 
> Thanks for working on this, it's great to have.  In addition to the nav bar,
> it is worth also adding to the "More Tools" section on the main page[1]?  I
> guess it's a bit tricky that the section and the matching part of the nav
> bar are maintained separately...
> 
> I don't have a strong opinion, so whatever seems best to you.
> 
> [1]: https://developer.mozilla.org/en-US/docs/Tools

Yeah, I think that's a good idea. I've added this and the Rulers tool to that section.