765105 - [inspector] Add image preview for background image urls

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

Chrome has this and it is nice.

Comment 1

[nemo]

Firebug also has this, always has, as far as I can remember. :)

Comment 2

[Paul Rouget [:paul]]

We want this for:
- rule view
- computed view
- markup view

Bug triage, filter on PINKISBEAUTIFUL

Comment 3

[Patrick Brosset <:pbro>]

Arrow type panels to the rescue.
I may need mentoring for this, but I'd like to tackle it.

Comment 4

[Patrick Brosset <:pbro>]

Here is my plan:

Tooltip structure

* The tooltip would be created using a XUL <panel> element.
* The <panel> element would be appended to one of the top level XUL windows (toolbox.xul).
* The panel's openPopup() method would then be called when the tooltip needs to be shown, passing an element as the anchor.

High level API/flow

In order to easily reuse this tooltip, the low level XUL panel's method would be wrapped into a widget:

* A new widget file called Tooltip.js would be created, this would be a commonJS module.
* Using it, toolbox.js would initialize one global shared tooltip instance for tools to use. This would create the necessary XUL code and append it to toolbox.xul's window.
* Any tool that would then require a tooltip would have to require() the Tooltip module to get the shared tooltip instance.
* With the reference to that instance, the tool would be able to set the tooltip's content and decide when and where to show and hide it.
* By lack of a better idea, I propose that the content of the tooltip be set by creating new XUL elements inside the panel.

This approach allows reuse of a single tooltip instance across all tools, simply by requiring the module.
However, if for any reasons, more than one tooltip would be needed, then the tooltip factory would allow to create as many as needed.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: 765105-toolbox-tooltips.patch

WIP #1
This is by no means finished and there's a lot of rough edges.
I'm uploading this patch merely as a way to illustrate the proposal above.

Any feedback/mentoring on this issue would be great!

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This is a duplicate of bug 702576.

The bugs for all of our missing tooltips are listed in bug 711947 ... These bugs also contain some decent screenshots.

With a cursory look over the code it looks okay but I thought we were going to use a doorhanger for this?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> This is a duplicate of bug 702576.
> 
> The bugs for all of our missing tooltips are listed in bug 711947 ... These
> bugs also contain some decent screenshots.
> 
> With a cursory look over the code it looks okay but I thought we were going
> to use a doorhanger for this?

I had a lot of trouble getting a doorhanger to work in Bug 702576.  I think we should use an interface like this from our code, even if Tooltip.js changes to use doorhangers behind the scenes.  If it could be done I would be interested in seeing a comparison of the UI between both options, but it seems like they aren't meant to allow arbitrary content like we will need for all the tooltips in Bug 711947.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> > This is a duplicate of bug 702576.
> > 
> > The bugs for all of our missing tooltips are listed in bug 711947 ... These
> > bugs also contain some decent screenshots.
> > 
> > With a cursory look over the code it looks okay but I thought we were going
> > to use a doorhanger for this?
> 
> I had a lot of trouble getting a doorhanger to work in Bug 702576.  I think
> we should use an interface like this from our code, even if Tooltip.js
> changes to use doorhangers behind the scenes.  If it could be done I would
> be interested in seeing a comparison of the UI between both options, but it
> seems like they aren't meant to allow arbitrary content like we will need
> for all the tooltips in Bug 711947.

The advantage of doorhangers is that the pointed part of the bubble points at the anchored element no matter where the doorhanger has to pop up. I will play around with a doorhangers and see what I can get working.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 803096 [details] [diff] [review]
765105-toolbox-tooltips.patch

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

Ignore me, arrow panels are way better than doorhangers.

Having taken a proper look at the code I really like it. I have not tested on dark and light themes and across OSes but this is exactly what we need. The tooltips on http://dabblet.com/ look a little bit like what I would like.

When you do style the tooltip be careful as panels are completely different across OSes.

My other comments are simply suggestions but I do think that TooltipFactory.showImageTooltip(anchor, imageUrl), TooltipFactory.showFontTooltip(anchor, "Font Family") etc. would be easier to use.

::: browser/devtools/markupview/markup-view.js
@@ +878,5 @@
>      }.bind(this), false);
>      this.codeBox.appendChild(this.editor.closeElt);
>    }
> +
> +  // FIXME: put this somewhere else

Ideally we would be able to use this tooltip for any image url (for css etc). Maybe we could turn all links texts to links that open in a new tab. This would make it easy to show the tooltip on hover.

It would just be nice to have some indication that something will happen if you hover the URL.

The background image should be a white and grey grid so that shows through when something is transparent.

Maybe we should be able to get different tooltips directly from the factory? That way we could just TooltipFactory.showImageTooltip(anchor, imageUrl), TooltipFactory.showFontTooltip(anchor, "Font Family") etc.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +123,5 @@
> +  this.panel.appendChild(box);
> +
> +  // Append the tooltip to the window
> +  // FIXME: does a XULDocument always have a <window> node?
> +  this.doc.getElementsByTagName("window")[0].appendChild(this.panel);

this.doc.querySelector("window").appendChild(this.panel);

That way we don't need to use an index.

@@ +135,5 @@
> +   * @param node anchor
> +   *        Which node should the popup be shown on
> +   * @param string position
> +   *        See https://developer.mozilla.org/en-US/docs/XUL/PopupGuide/Positioning
> +   *        Deftaults to before_start

Defaults

::: browser/devtools/styleinspector/rule-view.js
@@ +1794,5 @@
> +      imagePreviewNode.setAttribute("src", this.resolveURI(resourceURI));
> +
> +      this.valueSpan.addEventListener("mouseenter", (evt) => {
> +        // FIXME: find a better way to append content into the panel
> +        sharedTooltip.panel.childNodes[0].appendChild(imagePreviewNode);

sharedTooltip.panel.firstElementChild.appendChild(imagePreviewNode);

Comment 10

[Patrick Brosset <:pbro>]

Wow, thanks Mike, awesome feedback!
I'll take a look at the mentioned meta bug and attached screenshots.
That'll give me plenty of ideas to evolve the proposal.

I think providing specialized functions like showFontTooltip, showImageTooltip, ... on top of the simple show is a great idea!

About the doorhanger vs. arrow panel discussion, I have to say I'm not so sure what the differences are.
It seems to me they're pretty much exactly the same. We already use arrow-type panels in the debugger, in the search field and conditional breakpoints, that's why I thought I'd use them.

Reading this: https://developer.mozilla.org/en-US/docs/Using_popup_notifications , it seems to me that the panel that appears when you click on the lock icon, left of the location bar on a https site, is a doorhanger, but I don't see a difference with an arrow panel then.
Take a look at the screenshot attached for a comparison.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Comparison between a doorhanger and an arrow-panel. Or is this 2 arrow-panels?

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset from comment #10)
> About the doorhanger vs. arrow panel discussion, I have to say I'm not so
> sure what the differences are.
> It seems to me they're pretty much exactly the same.

I originally thought you were using a tooltip panel so I should have looked at the panel creation code.

Doorhangers === arrow panels

Comment 13

[Victor Porof [:vporof][:vp]]

Would this be a reusable component? I would love to have it for bug 859135!

Comment 14

[Patrick Brosset <:pbro>]

It's the goal, definitely.
Right now the API is very limited and requires more code to be written by the consumer than I would like it to, but I'll work on that.

Comment 15

[Patrick Brosset <:pbro>]

I have re-worked the tooltip API almost entirely (will be attaching a new patch just after) and implemented some preview tooltips in the rules-view panel.

You can see early results here: http://people.mozilla.org/~pbrosset/tooltips/

There are at this stage 2 open issues:
- rules-view property values are not granular enough to allow the separate preview of a color and an image in such an expression: background: red url(myImage.png) no-repeat top left;
- rules-view panel rules are created using a TextEditor instance that never gets destroyed properly, meaning that all events attached are not detached. I will try to find a way to do this properly.

Work left:
- make sure the API proposed fits the global needs of the devtools
- add preview tooltips to the markup-view and computed-view
- ...

Comment 16

[Patrick Brosset <:pbro>]

Attachment: 765105-toolbox-tooltips.patch

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to Patrick Brosset from comment #15)

Wow, this looks really good!

Comment 18

[Tim Nguyen :ntim]

(In reply to Patrick Brosset from comment #15)
> I have re-worked the tooltip API almost entirely (will be attaching a new
> patch just after) and implemented some preview tooltips in the rules-view
> panel.
> 
> You can see early results here: http://people.mozilla.org/~pbrosset/tooltips/

Nice ! but I think a panel is too much for a color or a gradient, same thing for the fonts and the box-shadow.
Maybe the color/gradient could just be a small square next to the property value (like Chrome).
And the font could be a small button that would open the fonts panel.


(In reply to Victor Porof [:vp] from comment #13)
> Would this be a reusable component? I would love to have it for bug 859135!
It would also be nice for web console requests log.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to ntim007 from comment #18) 
> Nice ! but I think a panel is too much for a color or a gradient, same thing
> for the fonts and the box-shadow.
> Maybe the color/gradient could just be a small square next to the property
> value (like Chrome).

We are also planning on doing that.

> And the font could be a small button that would open the fonts panel.
> 

Other devtools do it the same way we are doing it here... I personally like it.

Comment 20

[Patrick Brosset <:pbro>]

(In reply to ntim007 from comment #18)
> Nice ! but I think a panel is too much for a color or a gradient, same thing
> for the fonts and the box-shadow.
> Maybe the color/gradient could just be a small square next to the property
> value (like Chrome).
> And the font could be a small button that would open the fonts panel.
I think you're right for the color, it doesn't really require a panel, perhaps the small square thing would be enough. I don't agree for the gradients however, gradients can be pretty complex especially when background repeats (may be used to create patterns).
As for box-shadow and font, I'd personally keep them as panels.
We'll need more feedback here.

Comment 21

[Mihai Sucan [:msucan]]

(as requested by patrick, posting info on how to add the tooltip to the console)

First of all, this stuff is looking really great! I'd really like to have this in the console.

To show the image preview tooltip in the web console, simply require() tooltip.js in browser/devtools/webconsole/webconsole.js, then create an ImageTooltip in WCF_logNetEvent(). Attach the events to urlNode.

Concerns:

1. image previews would load the image from the client? or the server? do you put an image element that points the client to the url? or do you get the image through the remote protocol from the server?

The client, when pointed to the URL, may not have access to the image. It's not really the same as the remote Firefox instance loading the image.

2. in the network monitor case and the web console case you need to check the response content type to see if the url being pointed to is an image.

You might want to work on adding the tooltip to the network monitor and to the web console in a separate bug.

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset from comment #20)
> (In reply to ntim007 from comment #18)
> > Nice ! but I think a panel is too much for a color or a gradient, same thing
> > for the fonts and the box-shadow.
> > Maybe the color/gradient could just be a small square next to the property
> > value (like Chrome).
> > And the font could be a small button that would open the fonts panel.
> I think you're right for the color, it doesn't really require a panel,
> perhaps the small square thing would be enough. I don't agree for the
> gradients however, gradients can be pretty complex especially when
> background repeats (may be used to create patterns).
> As for box-shadow and font, I'd personally keep them as panels.
> We'll need more feedback here.

I didn't explain clearly. If you use a panel to display the color then you can use the transparency grid as the background image and then you can see any alpha.

Comment 23

[Tim Nguyen :ntim]

Well I guess the background-image and shadow could be a bit smaller, it's kinda wasted space  for me.
For the fonts, well, we have a font inspector that allows much more than the font tooltip. Why not link to it instead ? Maybe you could do both, but the panel is useless in my opinion.

The tooltip widget could also be nice for length units, or degrees.

Comment 24

[Patrick Brosset <:pbro>]

Thanks for all these inputs. I've created a document to track the progress in these areas. Indeed, several bugs will be needed. https://gist.github.com/captainbrosset/6681978
Feel free to comment in the document.

I guess we should keep this bug for the first implementation of the shared widget + a few preview doorhangers in the style inspector and markup view, and take care of the rest in other bugs that I'll open next.
I'll perhaps open a meta bug too.

Comment 25

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

This patches introduces the following:

- A new shared/widgets/tooltip.js component that takes care of creating the tooltips and providing some content in them for commonly used tooltips
- A canvas/img preview for the markup-view
- A img/color/border/shadow/gradient preview for both computed and rule views
- Tests for the above

Note that to do the img/canvas preview, I have added a new protocol method in the NodeActor that returns the base64 image data.

As said in my previous comment, let's keep this patch "fairly" small and iterate from there for other types of tooltips.

Comment 26

[Patrick Brosset <:pbro>]

Also note that I had to give up using XUL <panel>s and instead start using <tooltip>s as they former steal inputs and therefore make it that much harder to use the panels (i.e. clicking on a tag in the markup-view would not focus it anymore). Tooltips don't have the nice arrow shape, but they work a lot better.
So perhaps, we can add more styling to those in a future patch, to make them look more like doorhangers after all!

Comment 27

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

Sorry, last minute change as I got some info back after having posted a question to [e-mail].
Indeed, turns out XUL panels can in fact be used instead of tooltips, by using the consumeoutsideclicks attribute!!

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 811147 [details] [diff] [review]
bug765105-inspector-image-preview-popup.patch

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

This looks pretty good.

When the image does not exist we display an empty doorhanger at the moment ... we should probably show some kind of broken image placeholder, probably an image saying "missing image."

It misses stuff within style="" and in the markup panel mousing over an img src atribute displays a doorhanger pointing at the start of the img tag rather than pointing to the image attribute. Similar issue in the computed view.

Saying that, when we switch to using the output parser all of these issues will be easily fixed so I wouldn't worry about them for now.

Please correct the issues below and ask for review again. Maybe have somebody else look over this as well because the painkillers I am taking leave me intellectually challenged at the moment.

::: browser/devtools/styleinspector/computed-view.js
@@ +1018,5 @@
> +  {
> +    this.templateMatchedSelectors = null;
> +    this.element = null;
> +
> +    this.matchedExpander.removeEventListener("click");

You also need to use the functions and bubble state to remove listeners. The same goes for other event listeners in this destructor.

This event listener was added using:
this.matchedExpander.addEventListener("click",
      this.onMatchedToggle.bind(this), false);

I would add:
this.onMatchedToggle = this.onMatchedToggle.bind(this);
to the constructor and then use:
this.matchedExpander.addEventListener("click",
      this.onMatchedToggle, false);

You can then easily remove the listener using:
this.matchedExpander.removeEventListener("click",
      this.onMatchedToggle, false);

this.onMatchedToggle = this.onMatchedToggle.bind(this) is special in that you will not need to remove this.onMatchedToggle in the destructor because the JS engine takes care of that. If you saved the bound version as this.boundOnMatchedToggle you would have had to set it to null in the destructor.

::: browser/themes/shared/devtools/common.inc.css
@@ +113,5 @@
>  }
> +
> +/* Tooltip widget (see browser/devtools/shared/widgets/Tooltip.js) */
> +
> +.devtools-tooltip.devtools-tooltip-t {

Nit: Maybe you should use devtools-tooltip-tooltip and devtools-tooltip-panel instead of devtools-tooltip-t and devtools-tooltip-p.

Their meaning is much clearer then.

Comment 29

[Patrick Brosset <:pbro>]

Thank you Mike for the review.
 
> When the image does not exist we display an empty doorhanger at the moment
> ... we should probably show some kind of broken image placeholder, probably
> an image saying "missing image."

Good catch! I've just made a change so that now a default "image was not found" localized string is shown in the doorhanger instead. 
Perhaps you should not show any doorhanger at all in fact ...

> It misses stuff within style=""

That's right, I haven't done that part. This bug was more about displaying images only for now. I'll put this into the roadmap here: https://gist.github.com/captainbrosset/6681978
And in fact, I'm entirely sure we should do it. Indeed, style attributes have their own rule block in the style inspector panel, with more space to give information to the user. It may not be very convenient to show different tooltips within an inline attribute.

> and in the markup panel mousing over an img src atribute displays a
> doorhanger pointing at the start of the img tag rather than pointing
> to the image attribute.

I've just changed that. Thanks.

> Similar issue in the computed view.

What do you mean? The doorhangers display on the left of the values in the computed view.

> Saying that, when we switch to using the output parser all of these issues
> will be easily fixed so I wouldn't worry about them for now.

Agreed! Let's go ahead with this first version and iterate later with the output parser to make doorhangers a lot more aware of the context.

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +1018,5 @@
> > +  {
> > +    this.templateMatchedSelectors = null;
> > +    this.element = null;
> > +
> > +    this.matchedExpander.removeEventListener("click");
> 
> You also need to use the functions and bubble state to remove listeners. The
> same goes for other event listeners in this destructor.
> 
> This event listener was added using:
> this.matchedExpander.addEventListener("click",
>       this.onMatchedToggle.bind(this), false);
> 
> I would add:
> this.onMatchedToggle = this.onMatchedToggle.bind(this);
> to the constructor and then use:
> this.matchedExpander.addEventListener("click",
>       this.onMatchedToggle, false);
> 
> You can then easily remove the listener using:
> this.matchedExpander.removeEventListener("click",
>       this.onMatchedToggle, false);
> 
> this.onMatchedToggle = this.onMatchedToggle.bind(this) is special in that
> you will not need to remove this.onMatchedToggle in the destructor because
> the JS engine takes care of that. If you saved the bound version as
> this.boundOnMatchedToggle you would have had to set it to null in the
> destructor.

I had actually seen this after attaching the patch, so that's corrected.

> ::: browser/themes/shared/devtools/common.inc.css
> @@ +113,5 @@
> >  }
> > +
> > +/* Tooltip widget (see browser/devtools/shared/widgets/Tooltip.js) */
> > +
> > +.devtools-tooltip.devtools-tooltip-t {
> 
> Nit: Maybe you should use devtools-tooltip-tooltip and
> devtools-tooltip-panel instead of devtools-tooltip-t and devtools-tooltip-p.
> 
> Their meaning is much clearer then.

Done. Thanks.

I will be attaching a new version of the patch soon.

Comment 30

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

Patch v2, after corrections from Mike's review.

Comment 31

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

Last minute change as there was one failing test case.
This patch is ready for review, and is ongoing a TRY build at https://tbpl.mozilla.org/?tree=Try&rev=e0fb8e7b66ed

Comment 32

[Heather Arthur [:harth]]

Patrick, I think you said that the tests needed fixing here. Could you cancel review if so?

Comment 33

[Patrick Brosset <:pbro>]

Green try build: https://tbpl.mozilla.org/?tree=Try&rev=799b5f33ec8a
Will be attaching the patch for review in a bit!

Comment 34

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

Here's basically V2. Since last patch, I simplified a lot the tooltip widget itself and the way content gets filled into the tooltip.
Also, test finally pass now!!

Comment 35

[Heather Arthur [:harth]]

Comment on attachment 819848 [details] [diff] [review]
bug765105-inspector-image-preview-popup.patch

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

Overall this patch looks really good, and it works well. I have a few nits and some comments on the API.

::: addon-sdk/source/python-lib/cuddlefish/docs/__init__.py
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +MOCHITEST_BROWSER_FILES := \

I'm confused about why we have to add stuff to cuddlefish, is there something I don't know about?

@@ +8,5 @@
> +    browser_inspector_markup_navigation.js \
> +    browser_inspector_markup_mutation.html \
> +    browser_inspector_markup_mutation.js \
> +    browser_inspector_markup_mutation_flashing.html \
> +	browser_inspector_markup_mutation_flashing.js \

Looks like you're mixing tabs and spaces here?

::: browser/devtools/framework/toolbox.js
@@ +178,5 @@
>    },
>  
>    /**
> +   * Open the toolbox.
> +   * Typically the first method to call after a new toolbox is instantiated.

Why this unrelated change?

::: browser/devtools/markupview/markup-view.js
@@ +51,5 @@
>   * @param iframe aFrame
>   *        An iframe in which the caller has kindly loaded markup-view.xhtml.
>   */
> +function MarkupView(aInspector, aFrame, aControllerWindow)
> +{

The rest of the file doesn't follow the braces on the next line convention.

@@ +52,5 @@
>   *        An iframe in which the caller has kindly loaded markup-view.xhtml.
>   */
> +function MarkupView(aInspector, aFrame, aControllerWindow)
> +{
> +  this.inspector = aInspector;

Curious about why you made changed ._inspector to .inspector in this patch? Edit: aha I see that you add an access to `.inspector` in the computed view code. See my comment on that.

@@ +614,5 @@
>    /**
>     * Called when the markup panel initiates a change on a node.
>     */
> +  nodeChanged: function MT_nodeChanged(aNode)
> +  {

Another moving brace to a new line, and adding the MT_x prefix when the rest of the file doesn't have this.

@@ +955,5 @@
>    this.expander.addEventListener("click", this._onToggle, false);
>  
>    // Dealing with the highlighting of the row via javascript rather than :hover
>    // This is to allow highlighting the closing tag-line as well as reusing the
> +  // theme css classes

Why get rid of this comment?

@@ +979,5 @@
>    },
>  
> +  _attachTooltipIfNeeded: function () {
> +    let tagName = this.node.tagName && this.node.tagName.toLowerCase();
> +    let isImage = tagName && tagName === "img" &&

minor, but don't need `tagName && tagName === "img"`, `tagName === "img"` will do, right?

@@ +1497,5 @@
> +   * @param string attrName The name of the attribute to get the element for
> +   * @return DOMElement
> +   */
> +  getAttributeElement: function EE_getAttributeElement(attrName)
> +  {

Rest of the file doesn't have the EE_x prefix or braces on newlines style.

::: browser/devtools/markupview/test/browser_inspector_markup_765105_tooltip.js
@@ +100,5 @@
> +    testImageTooltip(index + 1);
> +  });
> +}
> +
> +function compareImageData(img, imgData) {

Wow, you are really testing it, sweet.

@@ +125,5 @@
> +    // the image data comes back from the server. However, this test is executed
> +    // synchronously as soon as "inspector-updated" is fired, which is before
> +    // the data for images is known.
> +    let hasImage = () => tooltip.panel.getElementsByTagName("image").length;
> +    let poll = setInterval(() => {

Really shouldn't need to poll. Avoid it if you can. Can you listen for the load event on the image?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +74,5 @@
> + * Better usage:
> + *   let t = new Tooltip(xulDoc);
> + *   t.toggleOnHover(container, target => {
> + *     if (<condition based on target>) {
> + *       t.contentFactory.image("http://image.png");

Nice usage comment.

This API here is hard to follow. `t.contentFactory.image("img.png")`. I can't tell if it's getting, setting or other. My guess is it's setting the content of the tooltip to be that image? If so, it should be more explicit.

@@ +191,5 @@
> +   * @param Number showDelay
> +   *        An optional delay that will be observed before showing the tooltip.
> +   *        Defaults to 750ms
> +   */
> +  toggleOnHover: function(baseNode, targetNodeCb, showDelay = 750) {

This is just a question, but maybe 'startTogglingOnHover' would be better to match up with 'stopTogglingOnHover'? Your opinion though.

@@ +192,5 @@
> +   *        An optional delay that will be observed before showing the tooltip.
> +   *        Defaults to 750ms
> +   */
> +  toggleOnHover: function(baseNode, targetNodeCb, showDelay = 750) {
> +    this._basedNode = baseNode;

Here's my problem with the API as it is. You can only call "toggleOnHover" once really. If you call it again, that `._basedNode` gets overwritten, and `stopToggling` will just remove the listeners for whatever the last base node was.

If we're only going have it showing over one base node, it seems like the base node should be passed into the Tooltip's constructor.

@@ +210,5 @@
> +   * tracking
> +   * @param node baseNode
> +   *        The container for all target nodes
> +   */
> +  stopTogglingOnHover: function(baseNode) {

You don't end up using the baseNode argument actually, so get rid of it.

@@ +259,5 @@
> + *   let t = new Tooltip(xulDoc);
> + *   t.contentFactory.image("myImg.png", 600);
> + *   t.show();
> + */
> +function TooltipContentFactory(tooltip) {

Is this a factory really? It's not returning new instances of things, each of the methods seems to just fill in the tooltip's content. Maybe you could just move these functions to the Tooltip class, like setImage() etc.

@@ +296,5 @@
> +
> +    this.tooltip.content = vbox;
> +
> +    // Load the image to get dimensions and display it when done
> +    let imgObj = new this.doc.ownerGlobal.Image();

whoaaa, I've never seen this `ownerGlobal` before. Does `defaultView` work too?

::: browser/devtools/styleinspector/computed-view.js
@@ +169,5 @@
>    // The element that we're inspecting, and the document that it comes from.
>    this.viewedElement = null;
>  
> +  // Properties preview tooltip
> +  this.tooltip = new Tooltip(this.styleInspector.inspector.panelDoc);

Does the computed view's document not work? `this.styleDocument`.

::: browser/devtools/styleinspector/rule-view.js
@@ +1069,5 @@
>      theme: "auto"
>    };
>    this.popup = new AutocompletePopup(aDoc.defaultView.parent.document, options);
>  
> +  this.tooltip = new Tooltip(this.inspector.panelDoc);

Is the rule view's document not adequate here? why do we need the inspector's?

::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_existing_property.js
@@ +75,5 @@
>      editor = aEditor;
>      checkStateAndMoveOn(0);
>    });
>  
> +  EventUtils.synthesizeMouse(brace, 1, 1, {}, ruleViewWindow);

Why this change here?

::: browser/devtools/styleinspector/test/head.js
@@ +10,1 @@
>  

Why comment this stuff out?

Comment 36

[Patrick Brosset <:pbro>]

Thanks Heather for your review. I'll attach a new patch for review soon. My answers below:

> ::: addon-sdk/source/python-lib/cuddlefish/docs/__init__.py
> @@ +1,5 @@
> >  # This Source Code Form is subject to the terms of the Mozilla Public
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +MOCHITEST_BROWSER_FILES := \
> 
> I'm confused about why we have to add stuff to cuddlefish, is there
> something I don't know about?

I honestly have no idea why this file changed at all! Reverting now.

> ::: browser/devtools/framework/toolbox.js
> @@ +178,5 @@
> >    },
> >  
> >    /**
> > +   * Open the toolbox.
> > +   * Typically the first method to call after a new toolbox is instantiated.
> 
> Why this unrelated change?

Must have been when I was still researching how things worked globally in the toolbox. You're right, it's unrelated.

> ::: browser/devtools/markupview/markup-view.js
> @@ +51,5 @@
> >   * @param iframe aFrame
> >   *        An iframe in which the caller has kindly loaded markup-view.xhtml.
> >   */
> > +function MarkupView(aInspector, aFrame, aControllerWindow)
> > +{
> 
> The rest of the file doesn't follow the braces on the next line convention.

Corrected.

> @@ +52,5 @@
> >   *        An iframe in which the caller has kindly loaded markup-view.xhtml.
> >   */
> > +function MarkupView(aInspector, aFrame, aControllerWindow)
> > +{
> > +  this.inspector = aInspector;
> 
> Curious about why you made changed ._inspector to .inspector in this patch?
> Edit: aha I see that you add an access to `.inspector` in the computed view
> code. See my comment on that.

Yes, that was to make the inspector public across the different inspector views and therefore be able to access its XUL doc. I think I tried with each view's XUL doc with no luck, but you're right, I'll give this another try, that should make it cleaner.

> @@ +614,5 @@
> >    /**
> >     * Called when the markup panel initiates a change on a node.
> >     */
> > +  nodeChanged: function MT_nodeChanged(aNode)
> > +  {
> 
> Another moving brace to a new line, and adding the MT_x prefix when the rest
> of the file doesn't have this.

Corrected

> @@ +955,5 @@
> >    this.expander.addEventListener("click", this._onToggle, false);
> >  
> >    // Dealing with the highlighting of the row via javascript rather than :hover
> >    // This is to allow highlighting the closing tag-line as well as reusing the
> > +  // theme css classes
> 
> Why get rid of this comment?

Corrected

> @@ +979,5 @@
> >    },
> >  
> > +  _attachTooltipIfNeeded: function () {
> > +    let tagName = this.node.tagName && this.node.tagName.toLowerCase();
> > +    let isImage = tagName && tagName === "img" &&
> 
> minor, but don't need `tagName && tagName === "img"`, `tagName === "img"`
> will do, right?

Well, it may be that `this.node.tagName` is null, in which case tagName is too. So I'm kind of using tagName both as a boolean and string here. I'll make this easier to understand in the code.

> @@ +1497,5 @@
> > +   * @param string attrName The name of the attribute to get the element for
> > +   * @return DOMElement
> > +   */
> > +  getAttributeElement: function EE_getAttributeElement(attrName)
> > +  {
> 
> Rest of the file doesn't have the EE_x prefix or braces on newlines style.

Corrected

> @@ +125,5 @@
> > +    // the image data comes back from the server. However, this test is executed
> > +    // synchronously as soon as "inspector-updated" is fired, which is before
> > +    // the data for images is known.
> > +    let hasImage = () => tooltip.panel.getElementsByTagName("image").length;
> > +    let poll = setInterval(() => {
> 
> Really shouldn't need to poll. Avoid it if you can. Can you listen for the
> load event on the image?

Well there's no image in the tooltip at all, so no load event I can listen to. Polling was the only solution I found that would work.

> This API here is hard to follow. `t.contentFactory.image("img.png")`. I
> can't tell if it's getting, setting or other. My guess is it's setting the
> content of the tooltip to be that image? If so, it should be more explicit.

See your comment about the factory further down.

> @@ +191,5 @@
> > +   * @param Number showDelay
> > +   *        An optional delay that will be observed before showing the tooltip.
> > +   *        Defaults to 750ms
> > +   */
> > +  toggleOnHover: function(baseNode, targetNodeCb, showDelay = 750) {
> 
> This is just a question, but maybe 'startTogglingOnHover' would be better to
> match up with 'stopTogglingOnHover'? Your opinion though.

I agree, corrected.

> @@ +192,5 @@
> > +   *        An optional delay that will be observed before showing the tooltip.
> > +   *        Defaults to 750ms
> > +   */
> > +  toggleOnHover: function(baseNode, targetNodeCb, showDelay = 750) {
> > +    this._basedNode = baseNode;
> 
> Here's my problem with the API as it is. You can only call "toggleOnHover"
> once really. If you call it again, that `._basedNode` gets overwritten, and
> `stopToggling` will just remove the listeners for whatever the last base
> node was.
> 
> If we're only going have it showing over one base node, it seems like the
> base node should be passed into the Tooltip's constructor.

I don't think it makes a lot of sense to allow calling this function several times on several different base nodes indeed. So what I'll do is add some clarifications in the comment and make sure previous baseNode listeners are removed if toggleOnHover is called again.
However, as to passing baseNode in the constructor, I don't really agree because one doesn't have to use toggleOnHover, this is just one convenient function. If you end up not using it and calling show/hide yourself on some element's click event listener, it would be strange to have to pass a baseNode in the constructor.

> @@ +210,5 @@
> > +   * tracking
> > +   * @param node baseNode
> > +   *        The container for all target nodes
> > +   */
> > +  stopTogglingOnHover: function(baseNode) {
> 
> You don't end up using the baseNode argument actually, so get rid of it.

Done

> @@ +259,5 @@
> > + *   let t = new Tooltip(xulDoc);
> > + *   t.contentFactory.image("myImg.png", 600);
> > + *   t.show();
> > + */
> > +function TooltipContentFactory(tooltip) {
> 
> Is this a factory really? It's not returning new instances of things, each
> of the methods seems to just fill in the tooltip's content. Maybe you could
> just move these functions to the Tooltip class, like setImage() etc.

It's a factory in the sense that it instantiates new XUL nodes but, true, it doesn't return them and only
sets them as content of the tooltip. I'll think of something better.

> @@ +296,5 @@
> > +
> > +    this.tooltip.content = vbox;
> > +
> > +    // Load the image to get dimensions and display it when done
> > +    let imgObj = new this.doc.ownerGlobal.Image();
> 
> whoaaa, I've never seen this `ownerGlobal` before. Does `defaultView` work
> too?

Corrected

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +169,5 @@
> >    // The element that we're inspecting, and the document that it comes from.
> >    this.viewedElement = null;
> >  
> > +  // Properties preview tooltip
> > +  this.tooltip = new Tooltip(this.styleInspector.inspector.panelDoc);
> 
> Does the computed view's document not work? `this.styleDocument`.

I'll try this, you're right it would be better. I had some problems when I first started, but I don't recall what exactly.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1069,5 @@
> >      theme: "auto"
> >    };
> >    this.popup = new AutocompletePopup(aDoc.defaultView.parent.document, options);
> >  
> > +  this.tooltip = new Tooltip(this.inspector.panelDoc);
> 
> Is the rule view's document not adequate here? why do we need the
> inspector's?

Same as above.

> :::
> browser/devtools/styleinspector/test/
> browser_bug893965_css_property_completion_existing_property.js
> @@ +75,5 @@
> >      editor = aEditor;
> >      checkStateAndMoveOn(0);
> >    });
> >  
> > +  EventUtils.synthesizeMouse(brace, 1, 1, {}, ruleViewWindow);
> 
> Why this change here?

:) I can't remember! This test must have been failing at some stage (I started this bug a long time ago, so I can't remember exactly). I'll change it back and test again.

> ::: browser/devtools/styleinspector/test/head.js
> @@ +10,1 @@
> >  
> 
> Why comment this stuff out?

Good catch. Was only for testing. Corrected.

Comment 37

[Patrick Brosset <:pbro>]

I now recall why the rule, markup and computed views are referring to the inspector.panelDoc XULDocument when creating their tooltips: it's simply because tooltips need a XUL doc to live in, and none of these views have one, they're hosted in HTML documents instead.

However, I think there's no need to change the MarkupView's private `_inspector` to a public `inspector`, so I'll revert this.

Comment 38

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

New patch, fixes Heather's feedback + rebased against latest changes.
Ongoing TRY build: https://tbpl.mozilla.org/?tree=Try&rev=49243004dc6d

Comment 39

[Heather Arthur [:harth]]

(In reply to Patrick Brosset from comment #37)
> I now recall why the rule, markup and computed views are referring to the
> inspector.panelDoc XULDocument when creating their tooltips: it's simply
> because tooltips need a XUL doc to live in, and none of these views have
> one, they're hosted in HTML documents instead.

Aha, thanks. Have to ask, did you try `document.createElementNS(XUL_NS, "panel")`?

where `XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"`

Comment 40

[Patrick Brosset <:pbro>]

I've tried this just now, with no luck. Seems like including a XUL panel in an HTML document isn't really supposed to work. It gets appended alright, but either doesn't show up at all, or somehow breaks the layout of the HTML document.
Have you had previous experiences with this? Maybe I'm doing something wrong here...

Comment 41

[Heather Arthur [:harth]]

(In reply to Patrick Brosset from comment #40)
> I've tried this just now, with no luck. Seems like including a XUL panel in
> an HTML document isn't really supposed to work. It gets appended alright,
> but either doesn't show up at all, or somehow breaks the layout of the HTML
> document.
> Have you had previous experiences with this? Maybe I'm doing something wrong
> here...

Thanks for checking it out. Usually you can add XUL elements to HTML documents this way, but I guess not in this case.

Comment 42

[Heather Arthur [:harth]]

Comment on attachment 820959 [details] [diff] [review]
bug765105-inspector-image-preview-popup.patch

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

This is great, thanks. Just a few really minor nits.

I'm worried about the try failure above, however. Any idea about that?

::: addon-sdk/source/python-lib/cuddlefish/docs/__init__.py
@@ -1,4 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -

Still looks like you have a change here to that python file.

You might want to skim the diff/patch for this kind of stuff before or after uploading it.

::: browser/devtools/markupview/markup-view.js
@@ +979,5 @@
>      return "[MarkupContainer for " + this.node + "]";
>    },
>  
> +  _attachTooltipIfNeeded: function() {
> +    if (!!this.node.tagName) {

`if (x)` will always do the same thing as `if (!!x)`, so no need for the "!!" here.

::: browser/devtools/styleinspector/computed-view.js
@@ +864,5 @@
>  
>      this.onMatchedToggle = this.onMatchedToggle.bind(this);
>  
>      // Build the container element
> +    this.onMatchedToggle = this.onMatchedToggle.bind(this);

looks like this is a repeat of the line above.

::: browser/devtools/styleinspector/test/browser_bug893965_css_property_completion_existing_property.js
@@ +102,5 @@
>    }
>    else {
>      editor.once("after-suggest", checkState);
>    }
> +

random change here.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

Thanks Heather for the review.

I have corrected the last problems. Yes you're right, I should have looked at the patch more carefully before attaching it, this update to the python file shouldn't have made it there.

Since this new patch only brings corrections to these final feedback points, I have r+'ed it myself.

Comment 44

[Patrick Brosset <:pbro>]

As for the previous TRY build, 3 failures were infra failures that seemed to have been resolved, and 3 were mochitest browser test failures. Out of those 3, 2 were unrelated, and re-running them solved the problem. 1 was related and also there, re-running it solved it.
So what I'm going to do is launch another TRY build and see what happens. I hope it's not going to be a strange intermittent failure.

Comment 45

[Patrick Brosset <:pbro>]

Ok, try build is green: https://tbpl.mozilla.org/?tree=Try&rev=873adea60830
Asking for checkin

Comment 46

[Girish Sharma [:Optimizer]]

(In reply to Patrick Brosset from comment #45)
> Ok, try build is green: https://tbpl.mozilla.org/?tree=Try&rev=873adea60830
> Asking for checkin

Patrick, so much as I want this in before merge. If some related tests failed and rerunning them gave green, then it means that it is an intermittent. I am triggering more runs for those tests in your latest green try to see.

Comment 47

[Paul Rouget [:paul]]

(In reply to Girish Sharma [:Optimizer] from comment #46)
> (In reply to Patrick Brosset from comment #45)
> > Ok, try build is green: https://tbpl.mozilla.org/?tree=Try&rev=873adea60830
> > Asking for checkin
> 
> Patrick, so much as I want this in before merge. If some related tests
> failed and rerunning them gave green, then it means that it is an
> intermittent. I am triggering more runs for those tests in your latest green
> try to see.

The only weird thing is:
> 1 was related and also there, re-running it solved it.

But we can't solve it if we don't see the intermittent (if any) and more trys will just delay the feature. If we indeed see that there's an intermittent, we'll back out.

Comment 48

[Patrick Brosset <:pbro>]

Indeed, I have an intermittent ... https://tbpl.mozilla.org/?tree=Try&rev=873adea60830
This is due to one of the images I have in my mochitest browser test which is on a remote server!
I've copied the image in the test folder instead: https://tbpl.mozilla.org/?tree=Try&rev=7633f010a777

Comment 49

[Patrick Brosset <:pbro>]

Attachment: bug765105-inspector-image-preview-popup.patch

And here is the patch with the local image.

Comment 50

[Ed Morley [:emorley]]

(In reply to Patrick Brosset from comment #48)
> I've copied the image in the test folder instead:
> https://tbpl.mozilla.org/?tree=Try&rev=7633f010a777

I've requested a number of browser-chrome retriggers on that try push; please re-add checkin-needed when they come back green :-)

Comment 51

[Girish Sharma [:Optimizer]]

Looks green enough

Comment 52

[Girish Sharma [:Optimizer]]

Landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/2bcd412cf362

Comment 53

[Patrick Brosset <:pbro>]

Yay! Thanks!

Comment 54

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2bcd412cf362