987797 - Font preview tooltip does not preview web fonts

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

Open http://www.google.com/fonts/
Inspect one of the custom font previews on the page
Hover the font-family value with the custom font name

Expected: I see a tooltip with the custom font
Actual: I see a tooltip with the fallback font

This is presumably because the webfonts are not loaded in the frame with the tooltip.

Comment 1

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

I am currently working on building a canvas in the current window to draw the web font, and transferring the image as a dataURL into the Tooltip. The second approach was to use an iframe like the font inspector, however, there is no api to get the used fonts for a single node and instead gets the used font for a range.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong (:gluong) from comment #1)
> I am currently working on building a canvas in the current window to draw
> the web font, and transferring the image as a dataURL into the Tooltip. The

Sounds like a reasonable approach - and won't need to be limited to web fonts only.  It seems like it should work for the case of:

<p id="webfont" class="cursive">a</p>

p {
  font: Arial; /* Should show arial when hovering */
}

.cursive {
  font: cursive; /* Should show arial when hovering */
}

#webfont {
  font: "Some Unloaded Web Font"; /* I guess show whatever the canvas shows when rendering a web font that isn't loaded on the page */
  font: "Some Web Font"; /* Should show 'Some Web Font' when hovering */
}

> second approach was to use an iframe like the font inspector, however, there
> is no api to get the used fonts for a single node and instead gets the used
> font for a range.

You should be able to get the list of fonts through the rule view / output parser without doing a range.selectNode().  I'm guessing you are going to have to do the same thing for the canvas implementation (unless if I'm misunderstanding).

Comment 3

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

(In reply to Brian Grinstead [:bgrins] from comment #2)
> > second approach was to use an iframe like the font inspector, however, there
> > is no api to get the used fonts for a single node and instead gets the used
> > font for a range.
> 
> You should be able to get the list of fonts through the rule view / output
> parser without doing a range.selectNode().  I'm guessing you are going to
> have to do the same thing for the canvas implementation (unless if I'm
> misunderstanding).
The second approach would use DOMUtils.getUsedFontFaces(), which takes a Range and provides the font information for all the nodes in the Range. The point of this would be to get the web font href to load in the iframe. Hope that clarifies things.

Comment 4

[Paul Rouget [:paul]]

A range can be inside a node. A node can have several fonts. Just get the first font.

Using canvas won't help.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> A range can be inside a node. A node can have several fonts. Just get the
> first font.
> 
> Using canvas won't help.

One of the benefits of the canvas (I think) is to be able to show exactly what the user is seeing, or would be seeing, on the page even for font rules that aren't being due to specificity.  This could be a misunderstanding of getUsedFontFaces, but if you had:

<p class="className">a</p>

And the style inspector looked like this:

.className {
  font: cursive, serif;
}

p {
  font: Arial;
}

Wouldn't calling getUsedFontFaces ignore Arial and serif, since they are not actually being used in the range?  In other words, we care about the font stack that is being hovered, rather than the font stack that is applied to the node.

Whereas with a canvas, you could just set the ctx.font to whatever the string is from the output parser, and it would choose the same applied one in the stack that the element would if this was applied.  Something like this: http://jsfiddle.net/bgrins/emcR9/

Comment 6

[Paul Rouget [:paul]]

Right. I understand now why we can't use getUsedFontFaces.

But about canvas,

Why can't we just do `p.style.font = ...` ? How that would be different from `ctx.font = ...` ?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #6)
> Right. I understand now why we can't use getUsedFontFaces.
> 
> But about canvas,
> 
> Why can't we just do `p.style.font = ...` ? How that would be different from
> `ctx.font = ...` ?

In the case of:

.className {
  font: cursive;
}
p {
  font: "Web Font";
}

It will be tricky to actually display the web font in an iframe, since we don't know the usedFontFace.  By using a canvas owned by the same document as the node, the web font can be rendered without having to find the URL of the font and inject it into a frame.

Comment 8

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

Attachment: 987797-canvas.patch

Got an early working font family tooltip patch after going the iframe path initially. I still need to style the iframe quite a bit, but I was wondering if there is a better way of getting the current windows' document so that I can make use of the loaded web font api.

I still need to do some string manipulation on the font family name that we pass into the Tooltip. It does not work with Google Web Fonts currently because the string that is passed in looked like: ""Oswald" !important"

We will need to replace the double quotes and remove the !important.

Comment 9

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

Attachment: 987797-canvas.patch

Fixed with correct doc

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8423223 [details] [diff] [review]
987797-canvas.patch

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

::: browser/devtools/shared/widgets/Tooltip.js
@@ +782,5 @@
>      if (!font) {
>        return;
>      }
>  
> +    let doc = this.doc.defaultView.top.document.getElementById("content").contentWindow.document;

I think the approach is good, but this functionality should be moved to the server, to make sure it works for remote devices.  See the inspectorFront.getImageDataFromURL call elsewhere in this file for a similar implementation.  And the server changes will go in http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2521.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8423223 [details] [diff] [review]
987797-canvas.patch

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

::: browser/devtools/shared/widgets/Tooltip.js
@@ +795,4 @@
>      let vbox = this.doc.createElement("vbox");
>      vbox.setAttribute("flex", "1");
>  
> +    let previewer = this.doc.createElementNS(XHTML_NS, "iframe");

Also, you could probably reuse this.setImageContent() with the data url instead of building this dom here.  May need to add an option to tell it to not to add a label below

Comment 12

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

Attachment: 987797-canvas.patch

Moved the functionality over to the server. I am passing in the font family name and getting back the dataURL from the LongStringActor. In addition, Tooltip.setImageContent() is used to render the returned dataURL. I still need to make some changes to Tooltip.setImageContent() to not display the size and style the canvas, and documentations.

Comment 13

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

Attachment: 987797-canvas.patch

Added an extra option to setImageContent to not add the dimension label, and styled the canvas. It currently does not handle font families that are inherited. I am sure we want to have a special case for that. I am wondering if there is a function that will allow you to get the computed font family in such a case, but I will look further into it. The rendered image after getting the dataURL is not the best. I also looked into font smoothing, and these features are on by default.

Comment 14

[Brian Grinstead [:bgrins]]

I wonder if the bad font quality in canvas is related to bug 890733?  I see relatively bad results for canvas text in this simple demo: http://jsfiddle.net/bgrins/bBxCS/.  This is especially noticeable if the page is zoomed when loaded.

Comment 15

[Brian Grinstead [:bgrins]]

I found a technique here: http://blog.headspin.com/?p=464, that I think results in better font quality: http://jsfiddle.net/bgrins/bBxCS/3/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8424262 [details] [diff] [review]
987797-canvas.patch

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

As you mentioned, the quality of the rendered fonts isn't great.  I've mentioned a possible workaround in Comment 15

::: browser/devtools/shared/widgets/Tooltip.js
@@ -790,5 @@
> -    // Display the font family previewer
> -    let previewer = this.doc.createElement("description");
> -    previewer.setAttribute("flex", "1");
> -    previewer.style.fontFamily = font;
> -    previewer.classList.add("devtools-tooltip-font-previewer-text");

Remove references to devtools-tooltip-font-previewer-text in css files

::: browser/devtools/styleinspector/test/browser_styleinspector_tooltip-shorthand-fontfamily.js
@@ +56,5 @@
>    yield assertHoverTooltipOn(ruleView.previewTooltip, valueSpan);
>  
> +  let images = panel.getElementsByTagName("image");
> +  is(images.length, 1, "Tooltip contains an image");
> +  ok(images[0].getAttribute("src").startsWith("data:"), "Tooltip contains a data-uri image as expected");

I'm not sure if there is a much better way to test this beyond just making sure that there is an image than what you are doing here.  But maybe we could draw our own canvas with the known font and compare data urls?

::: toolkit/devtools/server/actors/inspector.js
@@ +2560,5 @@
> +
> +  /**
> +   * Returns the dataURL for a canvas that rendered the given font family name.
> +   */
> +  getFontFamilyDataURL: method(function(font) {

This will need to work with dark theme also.  We could allow the fillStyle color to be passed in as an argument.

@@ +2566,5 @@
> +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> +    canvas.width = 160;
> +    canvas.height = 35;
> +    let ctx = canvas.getContext("2d");
> +    ctx.font = "18px " + font.replace(/"/g, "'").replace(" !important", "");

This feels like something that should be sanitized on the frontend since the details of what string will be coming into the font tooltip depends on the rule/computed view.  I think we can treat this like getImageDataFromURL and assume that it is just a valid font string coming in here.

Comment 17

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

Attachment: 987797-canvas.patch

https://hg.mozilla.org/try/rev/83254fe0f711

This addressed the following:
- Remove references to devtools-tooltip-font-previewer-text in css files
- Added better unit tests: construct the font tooltip canvas and compares the dataURL
- fillStyle colours for the different themes
- Better font tooltip quality
- Currently, the tooltip does not display if the font family is inherit, unset or initial. This also applies to computed view until bug 1013467 is fixed.

Comment 18

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

Attachment: 987797-canvas.patch

Updated new option handling of addDimensionLabel for setImageContent().

https://tbpl.mozilla.org/?tree=Try&rev=4d79215a0be4

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: tooltip-misalignment.png

If you move the browser window far to the right of the screen and hover over the font, the tooltip arrow ends up in the wrong place.  Not sure if this is an issue in general with the Tooltip class or not

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8426122 [details] [diff] [review]
987797-canvas.patch

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

Looking good!

::: browser/devtools/shared/widgets/Tooltip.js
@@ +791,5 @@
>        return;
>      }
>  
> +    font = font.replace(/"/g, "'");
> +    font = font.replace(" !important", "");

Should probably just replace "!important" then do a trim separately.  When we have authored styles in rule view I don't think we can assume single spacing here

::: browser/devtools/styleinspector/test/browser_styleinspector_tooltip-longhand-fontfamily.js
@@ +79,3 @@
>  }
> +
> +// Draws the font family tooltip content onto a canvas and returns the dataURL.

I can't decide if it is better to copy/paste the code from the actor here or to call the actor method directly.  The downside to having it here is that any future changes need to be copy/pasted across.  The downside to using the method is that if a bug got introduced in the drawing there, the test would faithfully pass.  In reality though, any bug that did get introduced there would probably be copy/pasted here as well.

If you do keep this here, move it to head.js so that it can be shared across the tests.

::: toolkit/devtools/server/actors/inspector.js
@@ +2605,5 @@
> +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> +    let ctx = canvas.getContext("2d");
> +
> +    // Get the correct preview text measurements and set the canvas dimensions
> +    ctx.font = "20px " + font;

Shouldn't the font set for measurement be the same as the one for fillText?  May as well just store the full "20px " + font + ", serif" string in a variable and use it in both places

@@ +2616,5 @@
> +    ctx.textBaseline = "top";
> +    ctx.scale(2, 2);
> +    ctx.fillText(FONT_FAMILY_PREVIEW_TEXT, 0, 7);
> +
> +    canvas.style.width = textWidth + "px";

I don't think these lines will do anything in our case since the canvas is not visible

Comment 21

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

Attachment: 987797-canvas.patch

https://tbpl.mozilla.org/?tree=Try&rev=3ca10cfdd3af
(In reply to Brian Grinstead [:bgrins] from comment #20)
> Comment on attachment 8426122 [details] [diff] [review]
> 987797-canvas.patch
> 
> Review of attachment 8426122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +791,5 @@
> >        return;
> >      }
> >  
> > +    font = font.replace(/"/g, "'");
> > +    font = font.replace(" !important", "");
> 
> Should probably just replace "!important" then do a trim separately.  When
> we have authored styles in rule view I don't think we can assume single
> spacing here
> 
Fixed.

> :::
> browser/devtools/styleinspector/test/browser_styleinspector_tooltip-longhand-
> fontfamily.js
> @@ +79,3 @@
> >  }
> > +
> > +// Draws the font family tooltip content onto a canvas and returns the dataURL.
> 
> I can't decide if it is better to copy/paste the code from the actor here or
> to call the actor method directly.  The downside to having it here is that
> any future changes need to be copy/pasted across.  The downside to using the
> method is that if a bug got introduced in the drawing there, the test would
> faithfully pass.  In reality though, any bug that did get introduced there
> would probably be copy/pasted here as well.
> 
> If you do keep this here, move it to head.js so that it can be shared across
> the tests.
> 
Fixed. The unit tests get the dataURL via the server method and makes the comparison.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +2605,5 @@
> > +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> > +    let ctx = canvas.getContext("2d");
> > +
> > +    // Get the correct preview text measurements and set the canvas dimensions
> > +    ctx.font = "20px " + font;
> 
> Shouldn't the font set for measurement be the same as the one for fillText? 
> May as well just store the full "20px " + font + ", serif" string in a
> variable and use it in both places
> 
Fixed.

> @@ +2616,5 @@
> > +    ctx.textBaseline = "top";
> > +    ctx.scale(2, 2);
> > +    ctx.fillText(FONT_FAMILY_PREVIEW_TEXT, 0, 7);
> > +
> > +    canvas.style.width = textWidth + "px";
> 
> I don't think these lines will do anything in our case since the canvas is
> not visible

We need to style the content of the canvas prior to converting it into a dataURL. Otherwise, the tooltip will appear cutoff from the top.

Comment 22

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

Attachment: 987797-canvas.patch

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=c069f1336531

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8426419 [details] [diff] [review]
987797-canvas.patch

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

Patrick, how would you recommend handling backwards compat with this new inspector actor method and its use in Tooltip.js?

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8426419 [details] [diff] [review]
987797-canvas.patch

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

This looks very nice, and looks almost ready.  I have a few notes below, and have asked for some additional feedback from pbrosset.  Also, on your next patch, can you update the commit message?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +641,2 @@
>     */
> +  setImageContent: function(imageUrl, options) {

The default value for options got removed here, is this intentional?

::: toolkit/devtools/server/actors/inspector.js
@@ +2600,5 @@
> +   * Returns a dataURL for the image content of the font family tooltip that is
> +   * drawn using a canvas with the given font family name and fill style.
> +   */
> +  getFontFamilyDataURL: method(function(font, fillStyle) {
> +    let doc = this.window.document;

I think this will not work with iframes, since this.window is the tabActor window.  In order to make this work I think you would have to pass a reference to a node or a window.  Patrick may have some feedback about this too.

@@ +2603,5 @@
> +  getFontFamilyDataURL: method(function(font, fillStyle) {
> +    let doc = this.window.document;
> +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> +    let ctx = canvas.getContext("2d");
> +    let fontValue = "20px " + font + ", serif";

I'm not sure if it would make any practical difference in quality here, but we could just use a 40px font and forget the scaling and textWidth * 2, then just return textWidth/2 as the size.  Either way is fine with me

@@ +2612,5 @@
> +    canvas.width = textWidth * 2;
> +    canvas.height = 60;
> +
> +    ctx.font = fontValue;
> +    ctx.fillStyle = fillStyle;

I'm not sure what the behavior is for an undefined fillStyle, but this seems like something that should be optional (and the Arg :nullable).  You could always provide a default like black or something

Comment 26

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

Attachment: 987797-canvas.patch

(In reply to Brian Grinstead [:bgrins] from comment #25)
> Comment on attachment 8426419 [details] [diff] [review]
> 987797-canvas.patch
> 
> Review of attachment 8426419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks very nice, and looks almost ready.  I have a few notes below, and
> have asked for some additional feedback from pbrosset.  Also, on your next
> patch, can you update the commit message?
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +641,2 @@
> >     */
> > +  setImageContent: function(imageUrl, options) {
> 
> The default value for options got removed here, is this intentional?
> 

This was intentional. There are a number of places where setImageContent is used, and passes their own options (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1581). This would override the default value for addDimensionLabel. Rather than adding the addDimensionLabel option to each of these calls, options.hideDimensionLabel is checked to determine whether or not the label should be added. If it is undefined, it will add the label by default. This was changed from addDimensionLabel from the previous patch.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +2600,5 @@
> > +   * Returns a dataURL for the image content of the font family tooltip that is
> > +   * drawn using a canvas with the given font family name and fill style.
> > +   */
> > +  getFontFamilyDataURL: method(function(font, fillStyle) {
> > +    let doc = this.window.document;
> 
> I think this will not work with iframes, since this.window is the tabActor
> window.  In order to make this work I think you would have to pass a
> reference to a node or a window.  Patrick may have some feedback about this
> too.
> 

Waiting more feedback for this change.

> @@ +2603,5 @@
> > +  getFontFamilyDataURL: method(function(font, fillStyle) {
> > +    let doc = this.window.document;
> > +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> > +    let ctx = canvas.getContext("2d");
> > +    let fontValue = "20px " + font + ", serif";
> 
> I'm not sure if it would make any practical difference in quality here, but
> we could just use a 40px font and forget the scaling and textWidth * 2, then
> just return textWidth/2 as the size.  Either way is fine with me
> 

I gave this a try, and it didn't work for me. I would like to stick with this approach since it works without tweaking anymore.

> @@ +2612,5 @@
> > +    canvas.width = textWidth * 2;
> > +    canvas.height = 60;
> > +
> > +    ctx.font = fontValue;
> > +    ctx.fillStyle = fillStyle;
> 
> I'm not sure what the behavior is for an undefined fillStyle, but this seems
> like something that should be optional (and the Arg :nullable).  You could
> always provide a default like black or something

Done. I added a default to black and nullable:string to the Arg.

Fixed commit msg https://tbpl.mozilla.org/?tree=Try&rev=d7fbaf43f4ed

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong (:gl) from comment #26)
> > 
> > ::: browser/devtools/shared/widgets/Tooltip.js
> > @@ +641,2 @@
> > >     */
> > > +  setImageContent: function(imageUrl, options) {
> > 
> > The default value for options got removed here, is this intentional?
> > 
> 
> This was intentional. There are a number of places where setImageContent is
> used, and passes their own options
> (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/
> netmonitor-view.js#1581). This would override the default value for
> addDimensionLabel. Rather than adding the addDimensionLabel option to each
> of these calls, options.hideDimensionLabel is checked to determine whether
> or not the label should be added. If it is undefined, it will add the label
> by default. This was changed from addDimensionLabel from the previous patch.
> 

I believe you can use syntax like:

function foo(bar = {baz:false}) {

}

Or at least do options = options || {} to prevent an error when calling without an obj.

> > @@ +2603,5 @@
> > > +  getFontFamilyDataURL: method(function(font, fillStyle) {
> > > +    let doc = this.window.document;
> > > +    let canvas = doc.createElementNS(XHTML_NS, "canvas");
> > > +    let ctx = canvas.getContext("2d");
> > > +    let fontValue = "20px " + font + ", serif";
> > 
> > I'm not sure if it would make any practical difference in quality here, but
> > we could just use a 40px font and forget the scaling and textWidth * 2, then
> > just return textWidth/2 as the size.  Either way is fine with me
> > 
> 
> I gave this a try, and it didn't work for me. I would like to stick with
> this approach since it works without tweaking anymore.

SGTM

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> (In reply to Gabriel Luong (:gl) from comment #26)
> > > 
> > > ::: browser/devtools/shared/widgets/Tooltip.js
> > > @@ +641,2 @@
> > > >     */
> > > > +  setImageContent: function(imageUrl, options) {
> > > 
> > > The default value for options got removed here, is this intentional?
> > > 
> > 
> > This was intentional. There are a number of places where setImageContent is
> > used, and passes their own options
> > (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/
> > netmonitor-view.js#1581). This would override the default value for
> > addDimensionLabel. Rather than adding the addDimensionLabel option to each
> > of these calls, options.hideDimensionLabel is checked to determine whether
> > or not the label should be added. If it is undefined, it will add the label
> > by default. This was changed from addDimensionLabel from the previous patch.
> > 
> 
> I believe you can use syntax like:
> 
> function foo(bar = {baz:false}) {
> 
> }

Never mind, I see what you mean now.


> Or at least do options = options || {} to prevent an error when calling
> without an obj.

You can just do this.

Comment 29

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

Attachment: 987797-canvas.patch

(In reply to Brian Grinstead [:bgrins] from comment #28)
> (In reply to Brian Grinstead [:bgrins] from comment #27)
> > (In reply to Gabriel Luong (:gl) from comment #26)
> > > > 
> > > > ::: browser/devtools/shared/widgets/Tooltip.js
> > > > @@ +641,2 @@
> > > > >     */
> > > > > +  setImageContent: function(imageUrl, options) {
> > > > 
> > > > The default value for options got removed here, is this intentional?
> > > > 
> > > 
> > > This was intentional. There are a number of places where setImageContent is
> > > used, and passes their own options
> > > (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/
> > > netmonitor-view.js#1581). This would override the default value for
> > > addDimensionLabel. Rather than adding the addDimensionLabel option to each
> > > of these calls, options.hideDimensionLabel is checked to determine whether
> > > or not the label should be added. If it is undefined, it will add the label
> > > by default. This was changed from addDimensionLabel from the previous patch.
> > > 
> > 
> > I believe you can use syntax like:
> > 
> > function foo(bar = {baz:false}) {
> > 
> > }
> 
> Never mind, I see what you mean now.
> 
> 
> > Or at least do options = options || {} to prevent an error when calling
> > without an obj.
> 
> You can just do this.

Ah I just realized what you meant from your earlier comments. Indeed, the default value of options={} in setImageContent was actually unintentionally removed. I immediately thought of the default value of { addDimensionLabel: true } that was in earlier patches.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8427401 [details] [diff] [review]
987797-canvas.patch

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

Find some feedback below.
The backward compatibility isn't going to be much of a problem, but we have to decide what we do in case we are debugging an older target (fall back to the previous tooltip?)
As for iframes, I made a suggestion regarding moving the method to the NodeActor instead. Thinking again about it, it doesn't really belong there, but it's one way of doing it. The other way would be to leave it in the InspectorActor but passing a NodeActor along in the method call.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +782,5 @@
>     * for more info.
>     * @param {String} font The font family value.
> +   * @param {object} inspectorFront
> +   *        The InspectorActor that will used to retrieve the dataURL for the
> +   *        font family tooltip contents.

This method is async, you should add a "@return a promise that resolves when ..." comment in the jsdoc comment block here.

@@ +787,2 @@
>     */
> +  setFontFamilyContent: Task.async(function*(font, inspectorFront) {

+1 for the use of Task.async here!
It's quite important for tooltip set<something>Content method that are async to return a promise when done as it helps the rule-view/computed-view to know when the tooltip should be shown.

@@ +796,3 @@
>  
> +    let fillStyle = (Services.prefs.getCharPref("devtools.theme") === "light") ?
> +      "black" : "white";

This is probably a remark for a future bug, but at some stage we'll need to be able to access theme variables from JS. You need it here, but there are other places we may need it too.

@@ +799,2 @@
>  
> +    let {data, size} = yield inspectorFront.getFontFamilyDataURL(font, fillStyle);

This is where you'll need to test for the presence of the method.
If the toolbox is used to inspect a target running an older version of the devtools server, then this will fail (a common use case is using firefox nightly to debug a B2G device running a previous version of firefox, or debugging fennec).
One simple way to test this is the following:
- Open Aurora/Beta/Release
- Open the command line
- Type 'listen 6000'
- Open your build of fx-team with your changes
- Go to the "connect" screen in the "tools" menu
- Connect to localhost/6000
You'll be running a client-side (toolbox) with your latest code against a server-side (the debuggee firefox process) that doesn't have your code.

Perhaps we want to fallback to the font preview tooltip we had before in this case? Even if it doesn't work for web fonts, it's better than nothing.

::: browser/devtools/styleinspector/test/head.js
@@ +762,5 @@
>    return def.promise;
>  }
> +
> +/* *********************************************
> + * STYLE-INSPECTOR

If the helper functions you are adding here are common to the style-inspector (rule AND computed view) in general, as it seems to be the case, then you should move it up at the end of the 'UTILS' part.

::: toolkit/devtools/server/actors/inspector.js
@@ +62,5 @@
>  const {Class} = require("sdk/core/heritage");
>  const {PageStyleActor} = require("devtools/server/actors/styles");
>  const {HighlighterActor} = require("devtools/server/actors/highlighter");
>  
> +const FONT_FAMILY_PREVIEW_TEXT = "The quick brown fox jumps over the lazy dog";

This should probably go to a i18n property file. Perhaps in a follow-up bug.
Having said this, it's not obvious that this should be translated in all languages.
It's not because someone uses Firefox in Arabic that the person will want font preview tooltips to show Arabic characters.

@@ +2596,5 @@
>      response: RetVal("imageData")
> +  }),
> +
> +  /**
> +   * Returns a dataURL for the image content of the font family tooltip that is

nit: although it's good to say that one example usage for this method is the font family tooltip in the rule-view, the comment should be rather generic about what the method does, not making assumptions to what users are allowed to do with it or not.

@@ +2599,5 @@
> +  /**
> +   * Returns a dataURL for the image content of the font family tooltip that is
> +   * drawn using a canvas with the given font family name and fill style.
> +   */
> +  getFontFamilyDataURL: method(function(font, fillStyle="black") {

This is a new method in an existing actor, so managing backward compatibility is a lot simpler than when adding a new actor.
Indeed, actor methods are part of the "actor spec" which is used by protocol.js to implement the same list of methods on the front part. Which means it's easy for the client-side (the toolbox and panels) to know if a method exists or not.

On the client-side, you may do something like `if ("getFontFamilyDataURL" in myInspectorFront) {...}`

@@ +2600,5 @@
> +   * Returns a dataURL for the image content of the font family tooltip that is
> +   * drawn using a canvas with the given font family name and fill style.
> +   */
> +  getFontFamilyDataURL: method(function(font, fillStyle="black") {
> +    let doc = this.window.document;

The font might not always be available in `this.window`.
On the server-side, tab-level actors (like the inspector) receive a `TabActor` when they get initialized, and they use it to access the debuggee browser and content window.
But you can't assume that the debuggee content page doesn't have iframes.
And with iframes come scoping of styles, etc.

So in your case, it would be better to access the document by using the currently selected node.
To achieve this, I would suggest moving this method from the InspectorActor to the NodeActor. 
Once there, you'll be able to do `let doc = this.rawNode.ownerDocument`.

This obviously means you'll need to do some changes to the client-side part, in Tooltip.js, rule-view.js and computed-view.js:

Instead of passing the inspectorFront along, just pass `this.inspector.selection.nodeFront` in rule-view and computed-view

And in tooltip: `yield nodeFront.getFontFamilyDataURL(...)` instead of `yield inspectorFront.getFontFamilyDataURL(...)`

@@ +2609,5 @@
> +    // Get the correct preview text measurements and set the canvas dimensions
> +    ctx.font = fontValue;
> +    let textWidth = ctx.measureText(FONT_FAMILY_PREVIEW_TEXT).width;
> +    canvas.width = textWidth * 2;
> +    canvas.height = 60;

Why 60? Better move this number as a `const` with a self-explanatory name at the top of the file.

@@ +2615,5 @@
> +    ctx.font = fontValue;
> +    ctx.fillStyle = fillStyle;
> +    ctx.textBaseline = "top";
> +    ctx.scale(2, 2);
> +    ctx.fillText(FONT_FAMILY_PREVIEW_TEXT, 0, 7);

The last 3 lines aren't obvious to someone reading the code. I'm sure they're very much needed, but I suggest adding a comment before to explain what's going on.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8427401 [details] [diff] [review]
987797-canvas.patch

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

::: browser/devtools/shared/widgets/Tooltip.js
@@ +799,2 @@
>  
> +    let {data, size} = yield inspectorFront.getFontFamilyDataURL(font, fillStyle);

Regarding the fallback behavior, I wouldn't be opposed to just not doing anything if the server doesn't support it.  The issue I see with leaving the current behavior in there is that we will not easily be able to add test coverage for it AFAIK, and it is exactly the kind of thing that we could accidentally break without noticing.

::: toolkit/devtools/server/actors/inspector.js
@@ +62,5 @@
>  const {Class} = require("sdk/core/heritage");
>  const {PageStyleActor} = require("devtools/server/actors/styles");
>  const {HighlighterActor} = require("devtools/server/actors/highlighter");
>  
> +const FONT_FAMILY_PREVIEW_TEXT = "The quick brown fox jumps over the lazy dog";

We discussed this earlier on IRC - it's not clear to me that this is a string that we want internationalized.  I suppose matters more based on the language you are using on the page that you are developing rather than the language you have set as your default.  Also, do many web fonts support non-english alphabets?  Either way, I would vote for this as a separate bug - the current preview tooltip only uses these characters anyway: (ABCabc123&@%)

@@ +2609,5 @@
> +    // Get the correct preview text measurements and set the canvas dimensions
> +    ctx.font = fontValue;
> +    let textWidth = ctx.measureText(FONT_FAMILY_PREVIEW_TEXT).width;
> +    canvas.width = textWidth * 2;
> +    canvas.height = 60;

The main magic number here that could be a const is 20 (the size of the font in px).  I think 60 and 7 can be derived from this inside of the function

Comment 32

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

Attachment: 987797-canvas.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Comment on attachment 8427401 [details] [diff] [review]
> 987797-canvas.patch
> 
> Review of attachment 8427401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Find some feedback below.
> The backward compatibility isn't going to be much of a problem, but we have
> to decide what we do in case we are debugging an older target (fall back to
> the previous tooltip?)
> As for iframes, I made a suggestion regarding moving the method to the
> NodeActor instead. Thinking again about it, it doesn't really belong there,
> but it's one way of doing it. The other way would be to leave it in the
> InspectorActor but passing a NodeActor along in the method call.
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +782,5 @@
> >     * for more info.
> >     * @param {String} font The font family value.
> > +   * @param {object} inspectorFront
> > +   *        The InspectorActor that will used to retrieve the dataURL for the
> > +   *        font family tooltip contents.
> 
> This method is async, you should add a "@return a promise that resolves when
> ..." comment in the jsdoc comment block here.
> 

Fixed. Added @return for Tooltip.setFontFamilyContent.

> @@ +787,2 @@
> >     */
> > +  setFontFamilyContent: Task.async(function*(font, inspectorFront) {
> 
> +1 for the use of Task.async here!
> It's quite important for tooltip set<something>Content method that are async
> to return a promise when done as it helps the rule-view/computed-view to
> know when the tooltip should be shown.
> 
> @@ +796,3 @@
> >  
> > +    let fillStyle = (Services.prefs.getCharPref("devtools.theme") === "light") ?
> > +      "black" : "white";
> 
> This is probably a remark for a future bug, but at some stage we'll need to
> be able to access theme variables from JS. You need it here, but there are
> other places we may need it too.
> 

TODO: File a bug for this in the future.

> @@ +799,2 @@
> >  
> > +    let {data, size} = yield inspectorFront.getFontFamilyDataURL(font, fillStyle);
> 
> This is where you'll need to test for the presence of the method.
> If the toolbox is used to inspect a target running an older version of the
> devtools server, then this will fail (a common use case is using firefox
> nightly to debug a B2G device running a previous version of firefox, or
> debugging fennec).
> One simple way to test this is the following:
> - Open Aurora/Beta/Release
> - Open the command line
> - Type 'listen 6000'
> - Open your build of fx-team with your changes
> - Go to the "connect" screen in the "tools" menu
> - Connect to localhost/6000
> You'll be running a client-side (toolbox) with your latest code against a
> server-side (the debuggee firefox process) that doesn't have your code.
> 
> Perhaps we want to fallback to the font preview tooltip we had before in
> this case? Even if it doesn't work for web fonts, it's better than nothing.
> 

[1] Fixed. Added check for the method. I did not add back the fallback behavior based on Brian's feedback. Perhaps this should be a new bug if we decide to add a fallback behavior.

> ::: browser/devtools/styleinspector/test/head.js
> @@ +762,5 @@
> >    return def.promise;
> >  }
> > +
> > +/* *********************************************
> > + * STYLE-INSPECTOR
> 
> If the helper functions you are adding here are common to the
> style-inspector (rule AND computed view) in general, as it seems to be the
> case, then you should move it up at the end of the 'UTILS' part.
> 

Fixed. Moved getFontFamilyDataURL to UTILs.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +62,5 @@
> >  const {Class} = require("sdk/core/heritage");
> >  const {PageStyleActor} = require("devtools/server/actors/styles");
> >  const {HighlighterActor} = require("devtools/server/actors/highlighter");
> >  
> > +const FONT_FAMILY_PREVIEW_TEXT = "The quick brown fox jumps over the lazy dog";
> 
> This should probably go to a i18n property file. Perhaps in a follow-up bug.
> Having said this, it's not obvious that this should be translated in all
> languages.
> It's not because someone uses Firefox in Arabic that the person will want
> font preview tooltips to show Arabic characters.
> 

[2] TODO: Add a follow up bug regarding the internationalized of FONT_FAMILY_PREVIEW_TEXT


> @@ +2596,5 @@
> >      response: RetVal("imageData")
> > +  }),
> > +
> > +  /**
> > +   * Returns a dataURL for the image content of the font family tooltip that is
> 
> nit: although it's good to say that one example usage for this method is the
> font family tooltip in the rule-view, the comment should be rather generic
> about what the method does, not making assumptions to what users are allowed
> to do with it or not.
> 

Fixed. Revised the documentation to be more generic. 

> @@ +2599,5 @@
> > +  /**
> > +   * Returns a dataURL for the image content of the font family tooltip that is
> > +   * drawn using a canvas with the given font family name and fill style.
> > +   */
> > +  getFontFamilyDataURL: method(function(font, fillStyle="black") {
> 
> This is a new method in an existing actor, so managing backward
> compatibility is a lot simpler than when adding a new actor.
> Indeed, actor methods are part of the "actor spec" which is used by
> protocol.js to implement the same list of methods on the front part. Which
> means it's easy for the client-side (the toolbox and panels) to know if a
> method exists or not.
> 
> On the client-side, you may do something like `if ("getFontFamilyDataURL" in
> myInspectorFront) {...}`
> 

Fixed. See [1].

> @@ +2600,5 @@
> > +   * Returns a dataURL for the image content of the font family tooltip that is
> > +   * drawn using a canvas with the given font family name and fill style.
> > +   */
> > +  getFontFamilyDataURL: method(function(font, fillStyle="black") {
> > +    let doc = this.window.document;
> 
> The font might not always be available in `this.window`.
> On the server-side, tab-level actors (like the inspector) receive a
> `TabActor` when they get initialized, and they use it to access the debuggee
> browser and content window.
> But you can't assume that the debuggee content page doesn't have iframes.
> And with iframes come scoping of styles, etc.
> 
> So in your case, it would be better to access the document by using the
> currently selected node.
> To achieve this, I would suggest moving this method from the InspectorActor
> to the NodeActor. 
> Once there, you'll be able to do `let doc = this.rawNode.ownerDocument`.
> 
> This obviously means you'll need to do some changes to the client-side part,
> in Tooltip.js, rule-view.js and computed-view.js:
> 
> Instead of passing the inspectorFront along, just pass
> `this.inspector.selection.nodeFront` in rule-view and computed-view
> 
> And in tooltip: `yield nodeFront.getFontFamilyDataURL(...)` instead of
> `yield inspectorFront.getFontFamilyDataURL(...)`
> 

Fixed. Moved the getFontFamilyDataURL method to the server side of the NodeActor from the InspectorActor,
and fixed in rule-view, computed-view and tests.

> @@ +2615,5 @@
> > +    ctx.font = fontValue;
> > +    ctx.fillStyle = fillStyle;
> > +    ctx.textBaseline = "top";
> > +    ctx.scale(2, 2);
> > +    ctx.fillText(FONT_FAMILY_PREVIEW_TEXT, 0, 7);
> 
> The last 3 lines aren't obvious to someone reading the code. I'm sure
> they're very much needed, but I suggest adding a comment before to explain
> what's going on.

Fixed. Added some comments to explain the last 3 lines.

(In reply to Brian Grinstead [:bgrins] from comment #31)
> Comment on attachment 8427401 [details] [diff] [review]
> 987797-canvas.patch
> 
> Review of attachment 8427401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +799,2 @@
> >  
> > +    let {data, size} = yield inspectorFront.getFontFamilyDataURL(font, fillStyle);
> 
> Regarding the fallback behavior, I wouldn't be opposed to just not doing
> anything if the server doesn't support it.  The issue I see with leaving the
> current behavior in there is that we will not easily be able to add test
> coverage for it AFAIK, and it is exactly the kind of thing that we could
> accidentally break without noticing.
> 

Perhaps as a follow up bug. See [1]

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +62,5 @@
> >  const {Class} = require("sdk/core/heritage");
> >  const {PageStyleActor} = require("devtools/server/actors/styles");
> >  const {HighlighterActor} = require("devtools/server/actors/highlighter");
> >  
> > +const FONT_FAMILY_PREVIEW_TEXT = "The quick brown fox jumps over the lazy dog";
> 
> We discussed this earlier on IRC - it's not clear to me that this is a
> string that we want internationalized.  I suppose matters more based on the
> language you are using on the page that you are developing rather than the
> language you have set as your default.  Also, do many web fonts support
> non-english alphabets?  Either way, I would vote for this as a separate bug
> - the current preview tooltip only uses these characters anyway:
> (ABCabc123&@%)
> 

See [2]

> @@ +2609,5 @@
> > +    // Get the correct preview text measurements and set the canvas dimensions
> > +    ctx.font = fontValue;
> > +    let textWidth = ctx.measureText(FONT_FAMILY_PREVIEW_TEXT).width;
> > +    canvas.width = textWidth * 2;
> > +    canvas.height = 60;
> 
> The main magic number here that could be a const is 20 (the size of the font
> in px).  I think 60 and 7 can be derived from this inside of the function

Fixed. Added a const for the font family preview text size (20).

Comment 33

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

https://tbpl.mozilla.org/?tree=Try&rev=ef1ad2bd6d21

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8429411 [details] [diff] [review]
987797-canvas.patch

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

Feedback has been addressed, looks good.  Just applied and played with it, it is working with iframes as well.

::: toolkit/devtools/server/actors/inspector.js
@@ +371,5 @@
> +    // Get the correct preview text measurements and set the canvas dimensions
> +    ctx.font = fontValue;
> +    let textWidth = ctx.measureText(FONT_FAMILY_PREVIEW_TEXT).width;
> +    canvas.width = textWidth * 2;
> +    canvas.height = 60;

Nit: shouldn't the remaining magic numbers in this function be derived as well?  60 would be FONT_FAMILY_PREVIEW_TEXT_SIZE * 3 and 7 something like math.round(FONT_FAMILY_PREVIEW_TEXT_SIZE / 3).  Those seems like reasonable defaults that would adjust well when changing the font size

Comment 35

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

Attachment: 987797-canvas.patch

(In reply to Brian Grinstead [:bgrins] from comment #34)
> Comment on attachment 8429411 [details] [diff] [review]
> 987797-canvas.patch
> 
> Review of attachment 8429411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Feedback has been addressed, looks good.  Just applied and played with it,
> it is working with iframes as well.
> 
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +371,5 @@
> > +    // Get the correct preview text measurements and set the canvas dimensions
> > +    ctx.font = fontValue;
> > +    let textWidth = ctx.measureText(FONT_FAMILY_PREVIEW_TEXT).width;
> > +    canvas.width = textWidth * 2;
> > +    canvas.height = 60;
> 
> Nit: shouldn't the remaining magic numbers in this function be derived as
> well?  60 would be FONT_FAMILY_PREVIEW_TEXT_SIZE * 3 and 7 something like
> math.round(FONT_FAMILY_PREVIEW_TEXT_SIZE / 3).  Those seems like reasonable
> defaults that would adjust well when changing the font size

Fixed!

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8429667 [details] [diff] [review]
987797-canvas.patch

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

Add r=bgrins to the end of the commit message and reupload with r+.  Try looks good, so you can mark checkin-needed after that

Comment 37

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

Attachment: 987797-canvas.patch

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a09f09462bcc