Closed Bug 702577 Opened 13 years ago Closed 10 years ago

Show font-family tooltip

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: miker, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

When a URL points to a font declaration a font display tooltip should be displayed.
Bug triage, filter on PEGASUS.
Whiteboard: [styleinspector] → [computedview][ruleview]
triage. Filter on PINKISBEAUTIFUL
Component: Developer Tools → Developer Tools: Inspector
Attached image 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.
Font family capturing regex:
/font-family: (.+);?/gi
Summary: Style Inspector: show font-family tooltip → Show font-family tooltip
Hi,
would love to work on this?
(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.
Depends on: 918716
QA Contact: gabriel.luong
Assignee: nobody → gabriel.luong
QA Contact: gabriel.luong
Attached patch 702577.patch (obsolete) — Splinter Review
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.
Attachment #8379526 - Flags: feedback?(mratcliffe)
Added a screenshot of how it currently looks. I should note if there are multiple fonts provided, it defaults to the first one.
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?
Attachment #8379526 - Flags: feedback?(mratcliffe) → feedback+
Attached patch 702577.patch (obsolete) — Splinter Review
Added tooltip to computed view and test cases! Thanks for the feedback and demoing during the work week!
Attachment #8379526 - Attachment is obsolete: true
Attachment #8381100 - Flags: review?(mratcliffe)
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.
Attachment #8381100 - Flags: review?(mratcliffe)
Thanks for the review Mike! I will try to submit a revised patch sometime today.
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.
(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.
Status: NEW → ASSIGNED
(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 ?
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.
Attached patch 702577.patch (obsolete) — Splinter Review
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.
Attachment #8381100 - Attachment is obsolete: true
Attachment #8387387 - Flags: review?(mratcliffe)
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.
According to http://dabblet.com/help/index.html, it is distributed under NPOSL-3.0 license (http://www.opensource.org/licenses/NPOSL-3.0).
(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() ?
(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 on attachment 8387387 [details] [diff] [review]
702577.patch

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

Awesome work!
Attachment #8387387 - Flags: review?(mratcliffe) → review+
Whiteboard: [computedview][ruleview] → [land-in-fx-team]
Keywords: checkin-needed
Whiteboard: [land-in-fx-team]
(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
Keywords: checkin-needed
(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
###################################
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.
Attached patch 702577.patchSplinter Review
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.
Attachment #8387387 - Attachment is obsolete: true
Attachment #8388472 - Flags: review?(mratcliffe)
Comment on attachment 8388472 [details] [diff] [review]
702577.patch

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

Perfect
Attachment #8388472 - Flags: review?(mratcliffe) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/77bc8b1e43e7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [land-in-fx-team]
Target Milestone: --- → Firefox 30
Blocks: 987797
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.