702577 - Show font-family tooltip

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

When a URL points to a font declaration a font display tooltip should be displayed.

Comment 1

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

Bug triage, filter on PEGASUS.

Comment 2

[Paul Rouget [:paul]]

triage. Filter on PINKISBEAUTIFUL

Comment 3

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

Attachment: Font Family Tooltip

It should look something like this (screenshot courtesy of Lea Verou).

The code she users for her previews is at https://github.com/LeaVerou/dabblet/blob/master/code/previewers.js

She says that if we use her code (with changes) we should credit her in the file header.

Comment 4

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

Font family capturing regex:
/font-family: (.+);?/gi

Comment 5

[Hubert B Manilla]

Hi,
would love to work on this?

Comment 6

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

(In reply to Hubert B Manilla from comment #5)
> Hi,
> would love to work on this?

Unfortunately, we really need to finish bug 918716 before starting work on this.

Comment 7

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

Attachment: 702577.patch

Hi Mike, I did some quick hacking to get the font-family tooltip to work using a similar approach by Lea Verou. Right now, it doesn't parse the individual font-family properties into their own tokens (eg, font-family: arial, cursive => tokens: 'arial', 'cursive') and displays the token being hovered on. I am looking for some early feedback on how to proceed.

The approach I took is the following:
- Added a check for font-family property when a tooltip target is hovered on
- Construct a description element with some predefined text content and set the style to the given font-family value.

Comment 8

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

Attachment: Font-family tooltip preview

Added a screenshot of how it currently looks. I should note if there are multiple fonts provided, it defaults to the first one.

Comment 9

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

Comment on attachment 8379526 [details] [diff] [review]
702577.patch

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

Looks great so far. Can you also add this for the computed view?

Comment 10

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

Attachment: 702577.patch

Added tooltip to computed view and test cases! Thanks for the feedback and demoing during the work week!

Comment 11

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

Comment on attachment 8381100 [details] [diff] [review]
702577.patch

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

Looking good, just a couple more tweaks.

::: browser/devtools/styleinspector/rule-view.js
@@ +1145,5 @@
> +    // Test for font family
> +    if (property && property.name === "font-family") {
> +      this.previewTooltip.setFontFamilyContent(property.value).then(def.resolve);
> +      hasTooltip = true;
> +    }

Seems like our implementation is buggy here. There are two types of properties, shorthand and longhand. The tooltips should work on both e.g. when font: arial is used, expanded and it's font-family is hovered. Try something like this, it should work on both:
    // Test for font family
    let propertyRoot = target.parentNode;
    let propertyNameNode = propertyRoot.querySelector(".ruleview-propertyname");

    if (!propertyNameNode) {
      propertyRoot = propertyRoot.parentNode;
      propertyNameNode = propertyRoot.querySelector(".ruleview-propertyname");
    }

    let propertyName;
    if (propertyNameNode) {
      propertyName = propertyNameNode.textContent;
    }

    if (propertyName === "font-family" &&
        target.classList.contains("ruleview-propertyvalue")) {
      this.previewTooltip.setFontFamilyContent(target.textContent).then(def.resolve);
      hasTooltip = true;
    }

::: browser/devtools/styleinspector/test/browser_bug702577_fontfamily_tooltip.js
@@ +27,5 @@
> +    contentDoc = content.document;
> +    waitForFocus(createDocument, content);
> +  }, true);
> +
> +  content.location = "data:text/html,rule view font family tooltip test";

content.location = "data:text/html;charset=utf-8,rule view font family tooltip test";

Not your fault, we do this wrong almost everywhere.

Comment 12

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

Thanks for the review Mike! I will try to submit a revised patch sometime today.

Comment 13

[Tim Nguyen :ntim]

I don't understand why would someone want this. I mean it's just a duplication of the current font inspector tool. I think a font swatch linking to the font inspector would be more appropriate.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #13)
> I don't understand why would someone want this. I mean it's just a
> duplication of the current font inspector tool. I think a font swatch
> linking to the font inspector would be more appropriate.

It follows the UI pattern of showing contextual information about a CSS value when hovering over it - we do the same with a few other properties already.  The font inspector can be used if you want more information about the fonts being used, but this will be very useful to get a quick look at each font in the stack.

Comment 15

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> (In reply to Tim Nguyen [:ntim] from comment #13)
> > I don't understand why would someone want this. I mean it's just a
> > duplication of the current font inspector tool. I think a font swatch
> > linking to the font inspector would be more appropriate.
> 
> It follows the UI pattern of showing contextual information about a CSS
> value when hovering over it - we do the same with a few other properties
> already.  The font inspector can be used if you want more information about
> the fonts being used, but this will be very useful to get a quick look at
> each font in the stack.

Well, I think the font-family tooltip is less important. So maybe adding a 500ms delay to avoid accidental hover ?

Comment 16

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

gluong: For now let's not special case any tooltips so no added delay etc. Just address my comments and we can go from there.

Comment 17

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

Attachment: 702577.patch

Finally had a chance to work on this :)

Addressed shorthand properties with this new patch. Added shorthand property unit test - needed to click on the font expander, and query the computed list to get the font-family inside the shorthand property of font. 

Also, I wanted to make sure Lea Verou gets a special mention if this eventually lands. I basically adapted her work from her previewer.

Comment 18

[Joe Walker [:jwalker]]

Would probably be fair to give Lea a hat tip in a future hacks post. I'm nominating Brian to handle that...

Gabriel: Do you know what license Lea's work is under? https://github.com/LeaVerou/dabblet doesn't have any obvious license.

Comment 19

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

According to http://dabblet.com/help/index.html, it is distributed under NPOSL-3.0 license (http://www.opensource.org/licenses/NPOSL-3.0).

Comment 20

[Joe Walker [:jwalker]]

(In reply to Gabriel L (:gluong) from comment #17)
> Also, I wanted to make sure Lea Verou gets a special mention if this
> eventually lands. I basically adapted her work from her previewer.

What of the patch is based on Lea's work? Is it just setFontFamilyContent() ?

Comment 21

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

(In reply to Joe Walker [:jwalker] from comment #20)
> (In reply to Gabriel L (:gluong) from comment #17)
> > Also, I wanted to make sure Lea Verou gets a special mention if this
> > eventually lands. I basically adapted her work from her previewer.
> 
> What of the patch is based on Lea's work? Is it just setFontFamilyContent() ?

Yes, setFontFamilyContent() would be based on Lea's work. However, no particular code was reused here, but the methodology she used was custom tailored for our case - using XUL element to display a description label and setting the text content and font family style.
In addition, the CSS styles in .devtools-tooltip-font-previewer-text (common.css) was actually reused from Lea's work.

Hope that helps!

Comment 22

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

I will review after we have a green try:
https://tbpl.mozilla.org/?tree=Try&rev=605afb1541b9

Comment 23

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

Comment on attachment 8387387 [details] [diff] [review]
702577.patch

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

Awesome work!

Comment 24

[Joe Walker [:jwalker]]

(In reply to Gabriel L (:gluong) from comment #21)
> (In reply to Joe Walker [:jwalker] from comment #20)
> > (In reply to Gabriel L (:gluong) from comment #17)
> > > Also, I wanted to make sure Lea Verou gets a special mention if this
> > > eventually lands. I basically adapted her work from her previewer.
> > 
> > What of the patch is based on Lea's work? Is it just setFontFamilyContent() ?
> 
> Yes, setFontFamilyContent() would be based on Lea's work. However, no
> particular code was reused here, but the methodology she used was custom
> tailored for our case - using XUL element to display a description label and
> setting the text content and font family style.
> In addition, the CSS styles in .devtools-tooltip-font-previewer-text
> (common.css) was actually reused from Lea's work.

I've heard a number of people say "10 lines" as a point where we need to think about copyright, and "based on" is legally part of copying. If that's right, this is somewhat borderline from a copyright POV.

There's also the 'doing the right thing' POV, and adding a mention of Lea's work to the comments for setFontFamilyContent seems fair, and doesn't fall into the issues of the original BSD license [1].

So I propose:

1. Gabriel adds a comment to setFontFamilyContent that says "This is based on Lea Verou's Dablet. See https://github.com/LeaVerou/dabblet for more info" or something like that.

2. We needinfo Lea to check that she's happy with that.

3. If we have trouble, we ask Gerv.

I took off the checkin-needed flag while we do this.

[1]: https://www.gnu.org/philosophy/bsd.html

Comment 25

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

(In reply to Joe Walker [:jwalker] from comment #24)
> So I propose:
> 
> 1. Gabriel adds a comment to setFontFamilyContent that says "This is based
> on Lea Verou's Dablet. See https://github.com/LeaVerou/dabblet for more
> info" or something like that.
> 
> 2. We needinfo Lea to check that she's happy with that.
> 

I have contacted here previously about this. Her response:
###################################
Hi Mike,

As long as I'm properly credited, feel free to use whatever, code, ideas, screenshots, anything :)

Cheers,
Lea
###################################

Comment 26

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

So Gabriel, can you add a comment to setFontFamilyContent that says "This is based on Lea Verou's Dablet. See https://github.com/LeaVerou/dabblet for more info" and then we should be fine to land it.

Comment 27

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

Attachment: 702577.patch

Thanks for the help Mike and Joe! I wanted to make sure proper credits was given to Lea. I added the credit to the comments of setFontFamilyContent in the updated patch.

Comment 28

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

Comment on attachment 8388472 [details] [diff] [review]
702577.patch

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

Perfect

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77bc8b1e43e7