1108288 - CSS background highlighter/editor

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

The background-position property is often used to only reveal a certain area of a larger (sprite) image when applied to an element of a certain size.

It is sometimes difficult to find the exact coordinates for this property.
The devtools could provide a new in-content highlighter/editor tool that helps solving the following use cases:

- finding out which of the 2 numbers is on for the X axis and which is for the Y axis,
- seeing the rest of the image, super-imposed on the element, to know where in the larger image the currently visible part is,
- moving precisely the background-image.

(http://jsbin.com/maqoroludo/2/edit is a quick demo that shows the entire background image on mouse-over).

I think this tool should be in-content, to be more useful. Just like the box-model highlighter, or css transform highlighter.
One way to activate this tool could be from the rule-view. For instance, the css transform highlighter today appears when moving the mouse over a transform css property in the rule-view.
If the goal, however, is to make this tool interactive (letting users actually move the image in the page), then mouse-over won't work, and we might need an icon to turn it on and off.

On top of super-imposing the image, it would be useful to add extra information on the page too. One example: arrows along the X and Y axis with dimension labels to visualize the current position offsets.

Corner cases to have in mind:
- there can be multiple background-images per element
- the size of background-images can be changed using background-size
- the units used to set the background-position can be any css unit, or top/left/bottom/right (the computed style though will always return % or px).

Comment 1

[Patrick Brosset <:pbro>]

Actually, this is a better example: http://jsbin.com/maqoroludo/3/edit
It uses a large icons sprite image. Just mouse over the image to see what this simple demo does. It's not much but it does feel really useful.

We can definitely take things in steps here and first only implement a highlighter that would just do what this demo does: show the full image, at the right place.
This could then very easily be integrated into the rule-view like the css transform highlighter.

If someone is interested in tackling this not so simple but ok bug, I can mentor them.

Comment 2

[Shubham Jindal [:shubham]]

Hi,
I want to work on this feature. I have solved some of [good first bug] and I want to try something harder. 
I am unable to get the bug here. I went through the http://jsbin.com/maqoroludo/3/edit. On hovering, I was able to see some small images in a grid.

Comment 3

[Shubham Jindal [:shubham]]

From the code, what I can understand is that we have a large image. The image is fetched using a Promise and then when we hover on the icon, the larger image is shown as the highlighter. I am unable to figure now what exactly we have to implement.:)

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal from comment #2)
> I am unable to get the bug here. I went through the
> http://jsbin.com/maqoroludo/3/edit. On hovering, I was able to see some
> small images in a grid.
Well, this isn't a bug, nothing is broken here. This is a request to implement a new feature.
As explained in comment 0, the idea proposed here is to add a new tool that would help users of the devtools understand better how css background-images work, and would also possibly let users modify the background-position property easily.

Comment 5

[Shubham Jindal [:shubham]]

>  Well, this isn't a bug, nothing is broken here. This is a request to implement a new feature.
Yeah..:)

Can you guide me how should I move ahead implementing this feature? Like I think I have to make a new file or can I just edit an existing file? And I have to show the the entire image to the user in the browser window itself right?

Comment 6

[Patrick Brosset <:pbro>]

I think the first step in this bug is to create a new highlighter whose job will be to show the entire background-image on elements that have a background-image.

This would be a first, simple-enough, step in the right direction. And we could then build more complex things on top of that. I think it still would be useful as is.

UX-wise: We can just reuse the same pattern as for the css-transform highlighter, that is, show the highlighter when hovering over a background property in the rule-view.

To do this, there are essentially 2 parts:

1 - the server part, which should consist in adding a new highlighter class to /toolkit/devtools/server/actors/highlighter.js.
    - In that file, you'll see there are already a lot of different highlighters, and they are all pretty much done the same way,
    - you'll need to add a new one, 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,
    - in show, it should do something similar to http://jsbin.com/maqoroludo/3/edit in order to display the full background image, at the right place (note that this demo doesn't take background-size into account, this should be fixed),
    - in order to actually display the image in the page, we need to use the CanvasFrameAnonymousContentHelper class, which is in the same file. I highly suggest reading through the comment for this class. I'll try to find some time this week to write a better documentation of this API on the wiki.

2 - and the client part, which should consist in making the rule-view use this new highlighter
    - We already have several highlighters used in the rule-view, so it's easy to just look at how this works in /browser/devtools/styleinspector/style-inspector-overlays.js,
    - basically, we need to make changes to the HighlightersOverlay class, its job is to track mouse movements and show the right highlighter when hovering over certain properties,
    - in _onMouseMove, we can have one more if to check if the property being hovered is either background or background-image (or maybe also bakground-position/size/...)

I think this clarifies what the first step could be.

Comment 7

[Shubham Jindal [:shubham]]

I started working on this. I am stuck on how to actually get the width and height of the div in highlighter.js and what exactly are the parameters(node,options) passed to the show function? 

Who then provides the required options to the "show" function?

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal from comment #7)
> Who then provides the required options to the "show" function?
The thing that will call your highlighter's show function is the CustomHighlighter's show function, but this isn't going to be very interesting to you, because it does this automatically when something on the client-side calls it.
The best example of this is in style-inspector-overlay, here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector-overlays.js#144
This is where all the highlighters that are used in the rule-view (and computed-view) are created, shown and hidden.
I suggest modifying the logic here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector-overlays.js#125 so that, if the hovered node is a background-image property, then show your new highlighter type.
(you'll first need to register your new highlighter class here too: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector-overlays.js#33).

> I am stuck on how to actually get the width and
> height of the div in highlighter.js
In your new highlighter class, in the show method, you'll receive a node argument. This node is actually a DOM node, so you can use that to access anything you want: width, height, computed style, ...

> and what exactly are the
> parameters(node,options) passed to the show function? 
- node is the DOM node for which the highlighter should be shown
- options is whatever options your highlighter might need. You might not need this at all (at least at the beginning).

Comment 9

[Shubham Jindal [:shubham]]

Attachment: bug1108288_css_background_highlighter.diff

I have added the show and hide functions and added all the things as required. I am still not currently taking care of background-size. Just wanted to initially see it working. But I am unable to see any results.
Can you please go through the attachment and suggest what did I miss?

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal from comment #9)
> Created attachment 8537034 [details] [diff] [review]
> bug1108288_css_background_highlighter.diff
> 
> I have added the show and hide functions and added all the things as
> required. I am still not currently taking care of background-size. Just
> wanted to initially see it working. But I am unable to see any results.
> Can you please go through the attachment and suggest what did I miss?
The code changes look good at first look, good job!
I'll look at it in more details in a bit and come back with more constructive feedback.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8537034 [details] [diff] [review]
bug1108288_css_background_highlighter.diff

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

This is an encouraging start.

There are various errors here and there that prevent anything from appearing on the screen, but the main idea is in place.
The very first thing to do is to check the console while running your modified build of firefox. Most of the time, it will give you the exact error message and stack trace preventing your new feature from working. 
The very first error that appears in the console after applying your patch and trying to hover over a "background-position" property is:

console.error: 
  Message: ReferenceError: getComputedStyle is not defined
  Stack:
    fetchBackgroundImage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:1338:7
BackgroundPositionHighlighter.prototype<.show@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:1392:5

Just looking at this should give you information about what to solve next.
After this is solved, there are other errors, but I'll comment about each problem below in the code.

I don't remember if I mentioned that we have some documentation about highlighters now: https://wiki.mozilla.org/DevTools/Highlighter
Make sure you check this out, this should help.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +128,5 @@
>      let type, options;
>      if (this._isRuleViewTransform(nodeInfo) ||
>          this._isComputedViewTransform(nodeInfo)) {
>        type = HIGHLIGHTER_TRANSFORM_TYPE;
> +    } else if (this._isRuleViewBackground(nodeInfo) || this._isComputedViewBackground(nodeInfo)) {

Minor comment, but important to make the code more readable: we have a rule that no line should be longer than 80 chars. One good way to reformat this is:

} else if (this._isRuleViewBackground(nodeInfo) ||
           this._isComputedViewBackground(nodeInfo)) {
  type = HIGHLIGHTER_BACKGROUND_TYPE;
}

@@ +160,5 @@
> +   * @param {Object} nodeInfo
> +   * @return {Boolean}
> +   */
> +  _isRuleViewBackground: function(nodeInfo) {
> +    let isTransform = nodeInfo.type === VIEW_NODE_VALUE_TYPE &&

The variable isTransform should instead be named isBackgroundPosition

@@ +175,5 @@
> +   * @param {Object} nodeInfo
> +   * @return {Boolean}
> +   */
> +  _isComputedViewBackground: function(nodeInfo) {
> +    let isTransform = nodeInfo.type === VIEW_NODE_VALUE_TYPE &&

Same here.

::: toolkit/devtools/server/actors/highlighter.css
@@ +178,5 @@
>    background: #80d4ff;
>    opacity: 0.8;
>  }
> +
> +:-moz-native-anonymous .background-position-highlighter {

No need for this new class. As my comments suggest in highlighter.js, we already have a class for this that you can reuse.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1323,5 @@
>      this.markup.setAttributeForElement(containerId, "style", style);
>    }
>  });
>  
> +function BackgroundPositionHighlighter(tabActor){

At some stage, you'll need to add a jsdoc comment for this new class.
Your comment should explain what this new highlighter does exactly.

@@ +1324,5 @@
>    }
>  });
>  
> +function BackgroundPositionHighlighter(tabActor){
> +  AutoRefreshHighlighter.call(this, tabActor);

I don't think your highlighter should inherit from AutoRefreshHighlighter.
You can take a look at RectHighlighter which also doesn't inherit from this.

However, not extending from this class means that |this.win| won't be defined, and you use it below, so you'll need to do this in the constructor:

this.win = tabActor.window;

(and make sure you do this.win=null; in the destroy function)

@@ +1330,5 @@
> +  this.markup = new CanvasFrameAnonymousContentHelper(this.tabActor, 
> +    this._buildMarkup.bind(this));
> +}
> +
> +function fetchBackgroundImage(el) {

This helper function should be moved to the bottom of the file, where there are other such helper functions. You can put it after createNode for instance.
Also, you need to add some jsdoc comment to explain what it does.

@@ +1332,5 @@
> +}
> +
> +function fetchBackgroundImage(el) {
> +  if (!el) {
> +    return Promise.reject();

We (unfortunately) don't yet use DOM promises in the devtools code. So, even though we will at some stage, it's better to use the Promise.jsm module (which is what we use throughout the code) to be consistent with the rest.

To do this, you'll need to add this import at the top of the file:
const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
near the other Cu.import statements.

And this line should become:
return promise.reject();

@@ +1335,5 @@
> +  if (!el) {
> +    return Promise.reject();
> +  }
> +  
> +  var style = getComputedStyle(el);

This can't work because getComputedStyle is a function that is defined on the global object that content web pages have. But the devtools actors don't run in this context. The right way to solve this is to first get the window object from the node:

let win = el.ownerDocument.defaultView;

And then:

let style = win.getComputedStyle(el);

(also note that we use 'let' instead of 'var' here).

@@ +1337,5 @@
> +  }
> +  
> +  var style = getComputedStyle(el);
> +  if (!style.backgroundImage || style.backgroundImage.indexOf("url(") === -1) {
> +    return Promise.reject();

return promise.reject(); instead

@@ +1340,5 @@
> +  if (!style.backgroundImage || style.backgroundImage.indexOf("url(") === -1) {
> +    return Promise.reject();
> +  }
> +
> +  var url = style.backgroundImage.substring(5, style.backgroundImage.length - 2);

let instead of var

@@ +1341,5 @@
> +    return Promise.reject();
> +  }
> +
> +  var url = style.backgroundImage.substring(5, style.backgroundImage.length - 2);
> +  var img = new Image();

Image isn't available here, you can do:

let img = new win.Image();

@@ +1343,5 @@
> +
> +  var url = style.backgroundImage.substring(5, style.backgroundImage.length - 2);
> +  var img = new Image();
> +
> +  return new Promise((resolve, reject) => {

The syntax of the promise implementation we use is different, unfortunately, so you'll need to rewrite this whole part with:

let def = promise.defer();
img.onload = function() {
  def.resolve(img);
}
img.onerror = function() {
  def.reject();
}
img.src = url;

return def.promise;

@@ +1357,5 @@
> +    img.src = url;
> +  });
> +}
> +
> +BackgroundPositionHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.prototype, {

Since you don't need to extend from AutoRefreshHighlighter, this line becomes:

BackgroundPositionHighlighter.prototype = {

@@ +1364,5 @@
> +  _buildMarkup: function() {
> +    let doc = this.win.document;
> +
> +    let container = doc.createElement("div");
> +    container.className = "background-position hidden";

We have a css class already for all highlighter containers:

let container = doc.createElement("div");
container.className = "highlighter-container";

no need for the hidden class here.

@@ +1366,5 @@
> +
> +    let container = doc.createElement("div");
> +    container.className = "background-position hidden";
> +
> +    let rootWrapper = createNode(this.win, {

No need for the extra rootWrapper element for now here. Right now, you just want to display something on the screen. So to test this out, the simplest way is to create the container (as you did above with the highlighter-container class), and then just add one element in it.

@@ +1376,5 @@
> +      prefix: this.ID_CLASS_PREFIX
> +    });
> +
> +    let backgroundDiv = createNode(this.win, {
> +      parent: rootWrapper,

parent: container, instead

@@ +1389,5 @@
> +    return container;
> +  },
> +
> +  show: function(node,options) {
> +    fetchBackgroundImage(node).then(function(img){

We now have arrow functions () => {} and the nice thing about them is that they are automatically bound to this. This means that if you use |this| in the arrow function body, it will work as expected. If you use a normal function instead, |this| won't work, you'd need to bind the function yourself.
So you should rewrite this line:

fetchBackgroundImage(node).then(img => {

@@ +1401,5 @@
> +      // Get the element position
> +      var nodeX = node.offsetLeft;
> +      var nodeY = node.offsetTop;
> +
> +      var pos = getComputedStyle(node).backgroundPosition.split(" ");

getComputedStyle won't work.
node.ownerDocument.defaultView.getComputedStyle will work.
Also, instead of accessing it twice (you need it once more further below in this function), save it locally:

let win = node.ownerDocument.defaultView;
let style = win.getComputedStyle(node);

and then:

let pos = style.backgroundPosition.split(" ");

But you should also check if node has a backgroundPosition defined at all. If it doesn't, you should hide the highlighter.

@@ +1416,5 @@
> +      var style = "width:" + imgWidth + "px;";
> +      style += "height:" + imgHeight + "px;";
> +      style += "left:" + nodeX + imgX + "px;";
> +      style += "top:" + nodeY + imgY + "px;";
> +      style += "background-image:" + getComputedStyle(node).backgroundImage+";";

If you have saved the computed style locally as told above:

style += "background-image:" + style.backgroundImage + ";";

Comment 12

[Shubham Jindal [:shubham]]

Attachment: bug1108288_css_background_highlighter(v2).diff

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8537183 [details] [diff] [review]
bug1108288_css_background_highlighter(v2).diff

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

As discussed on IRC, I see a few errors in the console (fixed them with http://pastebin.mozilla.org/8033078), but I'm not seeing the Promise.reject error you talked about.

Comment 14

[Shubham Jindal [:shubham]]

Attachment: bug1108288_css_background_highlighter(v3).diff

I have also taken into account the background-size. I know that there are corner cases like what if we have CSS like background-size: 100%;

I think getComputedStyle will return "100% auto". So, we will have to take care of that.

Also, I am facing a weird problem. Try to run this patch on http://jsbin.com/febeludewa/1/edit?html,css,js,output
When you scroll down the document and hover on background-position attribute, the highlighter that appears shifts down. In a way, it's "top" style has been changed. I am not able to figure out the reason.

And,in the JS Bin link http://jsbin.com/maqoroludo/3/edit?html,css,js,output
>  var imgX = translatePos(pos[0], elWidth);
>  var imgY = translatePos(pos[1], elHeight);

I think it should be
>  var imgX = translatePos(pos[0], imgWidth);
>  var imgY = translatePos(pos[1], imgHeight);

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8537291 [details] [diff] [review]
bug1108288_css_background_highlighter(v3).diff

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

Find a few advices below. These should help progress.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1353,5 @@
> +
> +    return container;
> +  },
> +
> +  show: function(node,options) {

The first thing you should do in this function is early return if the node doesn't have a background image:

    // Early return if the node doesn't have a background image to highlight.
    if (style.backgroundImage === "none") {
      return;
    }

@@ +1367,5 @@
> +      let win = node.ownerDocument.defaultView;
> +      let style = win.getComputedStyle(node);
> +
> +      let divWidth = style.width;
> +      let divHeight = style.height;

style.width and style.height aren't always defined, if you want to know the actual size of the node, you should use: node.offsetWidth and node.offsetHeight.

@@ +1371,5 @@
> +      let divHeight = style.height;
> +      
> +      let size, imgWidth, imgHeight;
> +
> +      if (!style.backgroundSize) {

I suggest we handle backgroundSize later, once the highlighter basics work. So let's assume for now that backgroundSize hasn't been set and that therefore, the image is exactly at its natural size.

I think this is a sensible thing to do for now because:
- background size won't affect the way the image is positioned, so we won't have to change the positioning logic later
- there are all sorts of edge cases: as you said, 100% is one, but the background-size property also accepts keywords like "cover" and "contain", which are going to be a bit complex to handle.

So, you should remove the else, and only keep what's in the if.

@@ +1387,5 @@
> +      let nodeHeight = node.offsetHeight;
> +      
> +      // Get the element position
> +      let nodeX = node.offsetLeft;
> +      let nodeY = node.offsetTop;

Although this used to work in the simple jsbin demo I linked to earlier, this won't work with a variety of cases: if the page has been scrolled, if the element is inside an iframe, etc ...

So, the other way to get this information is by doing this:

Add this to your constructor:
this.layoutHelpers = new LayoutHelpers(this.win);

Then add this to your destroy function:
this.layoutHelpers = null;

And then here, you can do:
let {bounds} = this.layoutHelpers.getAdjustedQuads(node);
let nodeX = bounds.x;
let nodeY = bounds.y;

Comment 16

[Patrick Brosset <:pbro>]

btw, I created a handy grid image and put that into a jsbin. You might want to use this for your tests:
http://jsbin.com/yejuxacecu/2/edit?css,output
It can probably help while developing.

Comment 17

[Shubham Jindal [:shubham]]

Attachment: bug1108288_css_background_highlighter(v4).diff

I am adding the patch for the code styling. I will now test this for various cases and then take care of all the background-position activities as discussed before.
I am really sorry for the delay.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8539280 [details] [diff] [review]
bug1108288_css_background_highlighter(v4).diff

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

Awesome, this seems to work fine. Of course, there will be edge cases, but it's a great start.
I've highlighted some minor changes that I'd like you to make below.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +162,5 @@
> +   * @return {Boolean}
> +   */
> +  _isRuleViewBackground: function(nodeInfo) {
> +    let isBackgroundPosition = nodeInfo.type === VIEW_NODE_VALUE_TYPE &&
> +                      nodeInfo.value.property === "background-position";

nit: please align the 2nd line with the first one, so that it's easier to read (nodeInfo.value.property.... should be vertically aligned with nodeInfo.type ...).

@@ +176,5 @@
> +   * @param {Object} nodeInfo
> +   * @return {Boolean}
> +   */
> +  _isComputedViewBackground: function(nodeInfo) {
> +    let isBackgroundPosition = nodeInfo.type === VIEW_NODE_VALUE_TYPE &&

Instead of repeating the same logic as in |_isRuleViewBackground|, you should extract it into a separate function called |_isBackgroundPositionProperty| and call this function from both |_isRuleViewBackground| and |_isComputedViewBackground|.

::: toolkit/devtools/server/actors/highlighter.js
@@ +15,5 @@
>  const {setIgnoreLayoutChanges} = require("devtools/server/actors/layout");
>  
>  Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +const { Promise: promise } = 

2 minor comments:
- please removing the trailing whitespace after the = sign
- please remove the spaces after { and before } to be consistent with the other imports in this file.

@@ +1324,5 @@
>      let style = "top:" + top + "px;left:" + left + "px;";
>      this.markup.setAttributeForElement(containerId, "style", style);
>    }
>  });
> +/**

nit: please separate this class from the previous one with one empty line.

@@ +1326,5 @@
>    }
>  });
> +/**
> + * The BackgroundPositionHighlighter is the class that displays an entire image around a
> + * background-positioned element.

I would like to rephrase this comment to something like: The BackgroundPositionHighlighter is a highlighter class that reveals the entire CSS background-image of an element.

@@ +1357,5 @@
> +
> +    return container;
> +  },
> +
> +  show: function(node,options) {

nit: please add a whitespace after the ,

@@ +1371,5 @@
> +    fetchBackgroundImage(node).then(img => {
> +      let win = node.ownerDocument.defaultView;
> +      let style = win.getComputedStyle(node);
> +
> +      if (!style.backgroundPosition === "none") {

You should check this earlier in the show function, to avoid having to fetch anything at all if there is no background.
So, instead I would put these lines:

let win = node.ownerDocument.defaultView;
let style = win.getComputedStyle(node);
if (!style.backgroundPosition === "none") {

at the very top of the show function.

And in fact, shouldn't we be checking for backgroundImage in the |if| instead of backgroundPosition?
The user might want to see the full background image of an element even if this image isn't positioned, since the element might have a size such that the image isn't completely visible. So, it should be:

if (style.backgroundImage === "none") {
  return;
}

Also, you don't need to encapsulate the whole rest of the function in an |else| because you have a return in the |if| anyway. This avoid having to indent everything below.

@@ +1379,5 @@
> +        let imgHeight = img.naturalHeight;
> +
> +        // Get the element size
> +        let nodeWidth = node.offsetWidth;
> +        let nodeHeight = node.offsetHeight;

These 2 variables aren't used anymore, please remove them.

@@ +1380,5 @@
> +
> +        // Get the element size
> +        let nodeWidth = node.offsetWidth;
> +        let nodeHeight = node.offsetHeight;
> +        

nit: please remove these trailing whitespaces.

@@ +1385,5 @@
> +        // Get the element position
> +        let {bounds} = this.layoutHelpers.getAdjustedQuads(node);
> +        let nodeX = bounds.x;
> +        let nodeY = bounds.y;
> +      

nit: please remove these trailing whitespaces.

@@ +1387,5 @@
> +        let nodeX = bounds.x;
> +        let nodeY = bounds.y;
> +      
> +        let pos = style.backgroundPosition.split(" ");
> +        

nit: please remove these trailing whitespaces.

@@ +1390,5 @@
> +        let pos = style.backgroundPosition.split(" ");
> +        
> +        let imgX = translatePos(pos[0], imgWidth);
> +        let imgY = translatePos(pos[1], imgHeight);
> +        

nit: please remove these trailing whitespaces.

@@ +1392,5 @@
> +        let imgX = translatePos(pos[0], imgWidth);
> +        let imgY = translatePos(pos[1], imgHeight);
> +        
> +        let leftX = nodeX + imgX;
> +        let topY = nodeY + imgY;

left and X mean the same thing, and top and Y too. So either name these variables just left and top, or just x and y.

@@ +1398,5 @@
> +        // Styling the highlighter
> +        let styleStr = "width:" + imgWidth + "px;";
> +        styleStr += "height:" + imgHeight + "px;";
> +        styleStr += "position: absolute;";
> +        styleStr += "opacity: 0.2;";

position absolute and opactity 0.2 aren't dynamic, they are always the same values, so instead of putting them in the style attribute in the JS code here, you should just move them to highlighter.css by adding:

:-moz-native-anonymous .background-position-highlighter {
  position: absolute;
  opacity: 0.2;
}

Comment 19

[Patrick Brosset <:pbro>]

I really like this. Even with just the basics in place, it's already useful.
I would change the opacity to something a lot higher though. The whole goal is to be able to see the rest of the image, so 0.2 is far too low. Maybe 0.8, and with a white background color, to really isolate it from the rest of the page.
I created a screencast from your patch for people to watch and contribute ideas if they want to: https://www.youtube.com/watch?v=kM9vL2YQuRg&feature=youtu.be

Comment 20

[Shubham Jindal [:shubham]]

Yeah..Even I am excited about this. The screencast looks nice.:)

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8539280 [details] [diff] [review]
bug1108288_css_background_highlighter(v4).diff

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

Awesome work ! I would suggest to add arrows with x and y coordinates, but that can be done in a another bug :)

Found a few more trailing whitespaces.
Also, can you change the commit message to this : "Bug 1108288 - Add a CSS background position highlighter. r=pbrosset" ?

::: toolkit/devtools/server/actors/highlighter.js
@@ +1941,5 @@
> +function fetchBackgroundImage(el) {
> +  if (!el) {
> +    return promise.reject();
> +  }
> +  

nit : Trailing whitespace

@@ +1961,5 @@
> +  // If the URL doesn't point to a resource, reject
> +  img.onerror = function() {
> +    def.reject();
> +  }
> +    

nit: Trailing whitespace

Comment 22

[Tim Nguyen :ntim]

I have a newer patch that fixes the following :
- fixed position when the page is zoomed
- fixed position on retina devices
- background-size support
- a few review comments as well (not all of them)

Things you still need to handle though :
- percent values for background position (doesn't seem to work properly for me)
- percent values for background size
- keywords for background position such as center, top, left, ...

Do you want me to upload it ?

Comment 23

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> @@ +1379,5 @@
> > +        let imgHeight = img.naturalHeight;
> > +
> > +        // Get the element size
> > +        let nodeWidth = node.offsetWidth;
> > +        let nodeHeight = node.offsetHeight;
> 
> These 2 variables aren't used anymore, please remove them.
These are used in the JS bin.

Comment 24

[Shubham Jindal [:shubham]]

(In reply to Tim Nguyen [:ntim] from comment #22)
> I have a newer patch that fixes the following :
> - fixed position when the page is zoomed
> - fixed position on retina devices
> - background-size support
> - a few review comments as well (not all of them)
> 
> Things you still need to handle though :
> - percent values for background position (doesn't seem to work properly for
> me)
> - percent values for background size
> - keywords for background position such as center, top, left, ...
> 
> Do you want me to upload it ?

Sure Tim. I will then work on the other left out cases. :)

Comment 25

[Tim Nguyen :ntim]

Attachment: css_background_highlighter(v5).diff

There you go :)

Comment 26

[Shubham Jindal [:shubham]]

Thanks Tim for the patch.

I was wondering why do we need to handle cases for keywords in background-position like center, top, left
We already are doing getComputedStyle which automatically tells us the styling in % values.

Am I wrong somewhere?

Comment 27

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal [:shubham] from comment #26)
> Thanks Tim for the patch.
> 
> I was wondering why do we need to handle cases for keywords in
> background-position like center, top, left
> We already are doing getComputedStyle which automatically tells us the
> styling in % values.
> 
> Am I wrong somewhere?
You are correct, getComputedStyle will return "top left" as "0% 0%".

Comment 28

[Shubham Jindal [:shubham]]

I guess we are then just left with the issues Tim has pointed out.

> Things you still need to handle though :
> - percent values for background position (doesn't seem to work properly for
> me)
> - percent values for background size

Comment 29

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

Attachment: css_background_highlighter(v6).diff

Rebased this patch after the changes bug 1127238.

@Shubham, are you still interested in continuing working on this bug?
I mainly interested in starting the next part after this patch which is to add the editor element. Let me know.

Thanks!

Comment 30

[Shubham Jindal [:shubham]]

Hi Gabriel,
I am still interested in working on this bug. But I have some assignment deadlines and exams till 19th in my college. Can I get some time till 19th and then I can start working on the bug after 19th?

Comment 31

[Shubham Jindal [:shubham]]

Hi Patrick and Gabriel,

I am now free from my exams and assignments and I can now continue working on this bug!

Comment 32

[Tim Nguyen :ntim]

(In reply to Shubham Jindal [:shubham] from comment #31)
> Hi Patrick and Gabriel,
> 
> I am now free from my exams and assignments and I can now continue working
> on this bug!

You can probably start by what was mentioned in comment 28, then you'll need to add tests.
I recommend using the patch Gabriel posted as base, to ease up your work.

As for the next steps, you could allow the user to drag the overlay, to adjust the background-position. But I suggest doing so in another bug.

Comment 33

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #32)
> As for the next steps, you could allow the user to drag the overlay, to
> adjust the background-position. But I suggest doing so in another bug.
Yeah, that's definitely one of the next steps, and I do agree this should be done in another bug. Furthermore, this should be done after bug 1123851 which I'm using to add event handling at highlighter level.

Comment 34

[Shubham Jindal [:shubham]]

Is the patch already in the mozilla-central directory? Because when I did |hg pull -u| I can see already most of the things from the patch by Gabriel in the directory.

Comment 35

[Shubham Jindal [:shubham]]

There are some errors in the directory when I run the http://jsbin.com/yejuxacecu/2/edit?css,output test.

-There is a redefinition of promise in toolkit/devtools/server/actors/highlighter.js file on line 8 and 20. Here is the pastebin link https://pastebin.mozilla.org/8823721

-Hovering on background property does not show the overlay at all. The terminal throws an error 

>"JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools
>/styleinspector/rule-view.js, line 1264: TypeError: classes is undefined" 

though the code in rule-view.js looks fine to me.

Comment 36

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

(In reply to Shubham Jindal [:shubham] from comment #35)
> There are some errors in the directory when I run the
> http://jsbin.com/yejuxacecu/2/edit?css,output test.
> 
> -There is a redefinition of promise in
> toolkit/devtools/server/actors/highlighter.js file on line 8 and 20. Here is
> the pastebin link https://pastebin.mozilla.org/8823721
> 
> -Hovering on background property does not show the overlay at all. The
> terminal throws an error 
> 
> >"JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools
> >/styleinspector/rule-view.js, line 1264: TypeError: classes is undefined" 
> 
> though the code in rule-view.js looks fine to me.

Your old patch was out of date because of new changes to the m-c. My patch rebases your patch with the latest code base.

Comment 37

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal [:shubham] from comment #34)
> Is the patch already in the mozilla-central directory? Because when I did
> |hg pull -u| I can see already most of the things from the patch by Gabriel
> in the directory.
None of the patches in this bug have landed in mozilla-central.
As Gabriel said, the code in m-c has changed quite a bit since you uploaded your patch, so for sure, there will be things to fix.

Comment 38

[Shubham Jindal [:shubham]]

Yes! I messed things up with mercurial queues and got confused. Things have sorted out now. And now the things are working fine.:)

Comment 39

[Shubham Jindal [:shubham]]

Attachment: bug1108288_css_background_highlighter(v7).diff

The patch works fine for:
1) If the CSS rule is "background" instead of specifically "background-position", we can see the overlay. Earlier, it used to work only for "background-position".
2) It works fine for the percentage values of "background-position". Changed the formula when we have "%" in the values.
3) It also works fine for pixels values of "background-size".

I have to work on now different values of background-size like "auto", percentage values etc. 
I exactly don't know what rules are followed when we specify auto or other keywords in background-size. It would be good if there is a link or documentation which explains different keywords of background-size.

Comment 40

[Patrick Brosset <:pbro>]

(In reply to Shubham Jindal [:shubham] from comment #39)
> I exactly don't know what rules are followed when we specify auto or other
> keywords in background-size. It would be good if there is a link or
> documentation which explains different keywords of background-size.
I think you need to read the spec: http://www.w3.org/TR/2002/WD-css3-background-20020802/#background-size
But this MDN page helps too: https://developer.mozilla.org/en-US/docs/Web/CSS/background-size

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8572613 [details] [diff] [review]
bug1108288_css_background_highlighter(v7).diff

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

I haven't done a full review yet, but this looks quite good to me.
I think we may need some more UI for the highlighter to make it very obvious to users what is being displayed, but that's a good start and we can re-iterate later.
Please take a look at my comments below and ping me for review when you have a new patch.

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +158,5 @@
> +   * Is the current hovered node a background-position property value
> +   * @param {Object} nodeInfo
> +   * @return {Boolean}
> +   */
> +  _isBackgroundPositionProperty: function(nodeInfo) {

Should be renamed to _isBackgroundRelatedProperty or something like this, because it's not only about background-position.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1389,5 @@
>  /**
> + * The BackgroundPositionHighlighter is the class that displays an entire image
> + * around a background-positioned element.
> + */
> +function BackgroundPositionHighlighter(tabActor){

I think a better name for this highlighter would be BackgroundImageHighlighter, because it's not only useful for debugging background-position issues.

@@ +1424,5 @@
> +  show: function(node, options) {
> +    let win = node.ownerDocument.defaultView;
> +    let style = win.getComputedStyle(node);
> +
> +    if (style.backgroundImage === "none") {

You also need to check that the background-image actually points to an image (e.g. not linear-gradient, radial-gradient, etc...).
In fact, showing the highlighter for gradients could also be useful, but I would keep this for a follow-up bug. So let's focus on actual images for now.

@@ +1449,5 @@
> +      let nodeWidth = node.offsetWidth;
> +      let nodeHeight = node.offsetHeight;
> +
> +      // Get the element position
> +      let {bounds} = this.layoutHelpers.getAdjustedQuads(node);

I think this will be incorrect if the element has padding. This should be:
let {bounds} = this.layoutHelpers.getAdjustedQuads(node, "padding");
I might be wrong, so please test your highlighter with various elements with margin, border, padding.

@@ +2016,5 @@
>  
>    return node;
>  }
>  
> +function fetchBackgroundImage(el) {

I think this function should take a String URL as input, not an element.
fetchImage(url) and the caller should be checking if the element has a background image, if it's actually a URL, and should extract it.

Comment 42

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

Attachment: 1108288.patch [8.0]

Rebased

Comment 43

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.

No activity for a year, removing assignment.

Comment 44

[Patrick Brosset <:pbro>]

Probably an uncommon case, but definitely would benefit from some kind of background editor:
https://a.singlediv.com/
Lynn uses a ton of background gradient images quite creatively to draw images.
Imagine trying to find the right gradient to edit ... Having a tool that makes it easy to determine which part of background image corresponds to which part of the final background would help these cases.

Comment 45

[Razvan Caliman [:rcaliman]]

Potentially useful feature: split multiple background images into individual layers of stack for easy visualization:
https://github.com/oslego/layerstack