Closed Bug 1440855 Opened 6 years ago Closed 6 years ago

Reveal font usage in the page on hover

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: pbro, Assigned: pbro)

Details

Attachments

(1 file)

Bug 1440855 - New font text-run highlighter used from the font inspector;
59 bytes, text/x-review-board-request
gl
: review+
Details 
This is the first step for implementing the font-reveal behaviour specified in this mockup: https://mozilla.invisionapp.com/share/TUE5VV8MV#/screens/278367863

The plan for this bug is to implement the highlight on hover only.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8953702 [details]
Bug 1440855 - New font text-run highlighter used from the font inspector;

https://reviewboard.mozilla.org/r/222934/#review228932

::: devtools/client/inspector/fonts/actions/index.js:12
(Diff revision 1)
>  const { createEnum } = require("devtools/client/shared/enum");
>  
>  createEnum([
>  
> +  // Set a given font as being currently revealed in the page.
> +  "SET_REVEALED_FONT",

s/SET_REVEALED_FONT/UPDATE_FONT_HIGHLIGHTED

::: devtools/client/inspector/fonts/fonts.js:163
(Diff revision 1)
> +   * Reveal a font's usage in the page.
> +   *
> +   * @param  {String} font
> +   *         The name of the font to be revealed in the page.
> +   * @param  {Boolean} show
> +   *         Whether to reveal or unreveal the font.

s/Whether/Whether or not to reveal the font.

::: devtools/client/inspector/fonts/fonts.js:165
(Diff revision 1)
> +   * @param  {String} font
> +   *         The name of the font to be revealed in the page.
> +   * @param  {Boolean} show
> +   *         Whether to reveal or unreveal the font.
> +   * @param  {Boolean} isForCurrentElement
> +   *         Whether to reveal the font for the current element selection or not.

Would prefer this to be reworded so that it is:
Whether or not to reveal the font for the current element selection.

::: devtools/client/inspector/fonts/fonts.js:167
(Diff revision 1)
> +   * @param  {Boolean} show
> +   *         Whether to reveal or unreveal the font.
> +   * @param  {Boolean} isForCurrentElement
> +   *         Whether to reveal the font for the current element selection or not.
> +   */
> +  async onRevealFont(font, show, isForCurrentElement) {

I think this would be a bit more obvious about what the function is doing if it was called onToggleFontHighlight.
Attachment #8953702 - Flags: review?(gl) → review+
We should probably add some tooltip for the mouseover as well since we get different behaviour whether or not the font is for the current element.
Comment on attachment 8953702 [details]
Bug 1440855 - New font text-run highlighter used from the font inspector;

I added a pref to hide this for now, as discussed with various people. 
So asking for review again.

With the pref we can tell some people to try it and give us feedback about it.
The UI will likely need to be made better. But it's a good first step.
Attachment #8953702 - Flags: review+ → review?(gl)
Attachment #8953702 - Flags: review?(gl)
Comment on attachment 8953702 [details]
Bug 1440855 - New font text-run highlighter used from the font inspector;

https://reviewboard.mozilla.org/r/222934/#review245738


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/fonts/actions/fonts.js:8
(Diff revision 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const {
> +  SET_HIGHLIGHTED_FONT,

Error: 'set_highlighted_font' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/inspector/fonts/components/Font.js:149
(Diff revision 4)
>        )
>      );
>    }
>  
>    renderFontName(name) {
> +    if (Services.prefs.getBoolPref(FONT_HIGHLIGHTER_PREF)) {

Error: 'services' is not defined. [eslint: no-undef]

::: devtools/client/inspector/fonts/test/browser_fontinspector_reveal-in-page.js:53
(Diff revision 4)
> +    ok(true, "1 selectionchange events detected on mouseout");
> +  }
> +});
> +
> +async function waitForNSelectionEvents(tab, numberOfTimes) {
> +  await ContentTask.spawn(tab.linkedBrowser, numberOfTimes, async function (n) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]
Comment on attachment 8953702 [details]
Bug 1440855 - New font text-run highlighter used from the font inspector;

https://reviewboard.mozilla.org/r/222934/#review245742


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/fonts/components/Font.js:149
(Diff revision 5)
>        )
>      );
>    }
>  
>    renderFontName(name) {
> +    if (Services.prefs.getBoolPref(FONT_HIGHLIGHTER_PREF)) {

Error: 'services' is not defined. [eslint: no-undef]

::: devtools/client/inspector/fonts/fonts.js:355
(Diff revision 5)
> +          name: font.name,
> +        });
> +      } else {
> +        await this.fontsHighlighter.hide();
> +      }
> +    } catch(e) {

Error: Expected space(s) after "catch". [eslint: keyword-spacing]

::: devtools/client/inspector/fonts/test/browser_fontinspector_reveal-in-page.js:53
(Diff revision 5)
> +    ok(true, "1 selectionchange events detected on mouseout");
> +  }
> +});
> +
> +async function waitForNSelectionEvents(tab, numberOfTimes) {
> +  await ContentTask.spawn(tab.linkedBrowser, numberOfTimes, async function (n) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]
Comment on attachment 8953702 [details]
Bug 1440855 - New font text-run highlighter used from the font inspector;

https://reviewboard.mozilla.org/r/222934/#review246310

::: devtools/client/inspector/fonts/fonts.js:331
(Diff revision 5)
> +   *         Whether or not to reveal the font for the current element selection.
> +   */
> +  async onToggleFontHighlight(font, show, isForCurrentElement) {
> +    if (!this.fontsHighlighter) {
> +      try {
> +        this.fontsHighlighter = await this.inspector.toolbox.highlighterUtils

I think we should add a showFontHighlighter/toggleFontHighligher in HighlightersOverlay, but I won't block the experiment on this.
Attachment #8953702 - Flags: review?(gl) → review+
This bug will only add this new feature behind a flag. I won't be turning it on by default for any users in this bug. So I think the dev-doc-needed flag is unnecessary for now. I want to gather feedback first from some selected early users before turning this into a real feature.
Keywords: dev-doc-needed
I had to rebase over some changes that impacted a file I had modified. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=596e83e2bfb621eb3fd9d4e8c1a6d596f3976f88&group_state=expanded
Nightly 62 is here now. I will rebase and then land this commit.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d699d5ea5e2
New font text-run highlighter used from the font inspector;r=gl
https://hg.mozilla.org/mozilla-central/rev/0d699d5ea5e2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: