1435469 - Font editor does not support input box

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Mahmoud Al-Qudsi]

Attachment: no fonts for input boxes.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171218174357

Steps to reproduce:

1) Activate dev tools
2) Switch to fonts tab
3) Select an input text box in the DOM 


Actual results:

The fonts explorer reports that there is no font info to display for the selected element.


Expected results:

The font styling should show up for the input box just as it would for other elements on the page.

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
20180202220135

Comment 2

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

This seems to have been fixed and is now reported in the Other fonts in page section.

Comment 3

[Mahmoud Al-Qudsi]

It's still not fixed. Set a font-family property for the input (e.g. in under element CSS rules in the inspector). The font does not appear associated with the element (though it does appear in other fonts).

Other Fonts is useless, even if I know what fonts are loaded in the document, I selected this element and inspected its properties to find out what font *this element* **in particular** is using. And Firefox does not show me that.

Comment 4

[Patrick Brosset <:pbro>]

The platform API we use to retrieve the list of fonts used on a given element is InspectorUtils.getUsedFontFaces(range).
You can see DevTools' usage of this API here:
https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/devtools/server/actors/styles.js#293-296

const rng = contentDocument.createRange();
rng.selectNodeContents(actualNode);
const fonts = InspectorUtils.getUsedFontFaces(rng);

Let's say actualNode is an <input> element. In that case, calling rng.selectNodeContents(actualNode) will return a collapsed range (startOffset:0 endOffset:0). In turn, InspectorUtils.getUsedFontFaces(rng) will return 0 fonts.

If actualNode is an ancestor of the <input> element however, then the range is correct and getUsedFontFaces does return the font used for the content of that input element.

So the problem to fix here is finding a way to provide a range object that getUsedFontFaces can use.

Comment 5

[Patrick Brosset <:pbro>]

Switching:
rng.selectNodeContents(actualNode);

to:
rng.selectNode(actualNode);

seems to fix it.
Now. I don't really understand the difference between the 2 methods. And I'm not sure if we can safely switch unconditionally, or if we should only do it for input fields.

The other problem I noticed is that the font highlighter (which appears when you hover over font faces in the font inspector) does not highlight input field content.
I think this might be trickier to fix because the text we are trying to highlight here does not exist in the DOM for real. This is only anonymous content that the engine injected. And I suspect the trick we use to highlight text in the page just doesn't work with anonymous content (this is done here https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/devtools/server/actors/highlighters/fonts.js#64-73).

Comment 7

[Vincent]

I would like to work on this bug if possible.

I dug a bit to understand where the current selectNodeContent is coming from.
It was originally a selectNode call but was replaced while adding support for before/after pseudo elements 4 years ago:
https://bugzilla.mozilla.org/show_bug.cgi?id=920141
There is no hint/comment about why exactly it was replaced.

I have a tentative patch (+unit test) to replace selectNodeContent with selectNode.
I added a non-regression test and ran inspector/fonts tests too.
I did some local testing (here: https://codepen.io/anon/pen/YRwrMQ) to see if the change breaks the fonts tab, everything looks good.

Could we run my patch on TRY to see if it has any unexpected side effects?
As well, what's the best way to get a good module owner for the review?

Thanks!

Comment 8

[Vincent]

Attachment: replace_select_node_contents_for_fonts.patch

Comment 9

[Patrick Brosset <:pbro>]

Thanks Vincent for working on this and digging into code archaeology to find why selectNodeContent was used. I'll take a look at bug 920141 see if I remember some specifics from back then.
I'll assign the bug to you now and will push your patch to TRY.

As for finding the right owner for a code review, it's a bit hard unfortunately.
Originally, the Mozilla project was structured around modules (still is actually).
Each module would have an owner and often several peers. Owners and peers for a module are the people who can do code reviews for things that are changed in that module. When controversial changes happen, or big changes are discussed, the owner is usually the one taking the final decision.

Anyway, the entire list of modules with owners and peers is available here:
https://wiki.mozilla.org/Modules/All

You will see there's an entry on this page for DevTools: https://wiki.mozilla.org/Modules/All#DevTools
The only problem with DevTools here is that it's a rather large module which could really use to be split in sub-modules. Indeed, in reality some people work on the debugger, others on the netmonitor, others on the inspector, but rarely do people know how to review code changes across multiple tools and layers of DevTools.

We attempted to make this more fine grained on our site: https://firefox-dev.tools/ (scroll down to "People and Modules") but this list has unfortunately gotten out of date (I'll submit a PR to update it just after I'm done writing here).

What I suggest doing is looking for who touched the code you are modifying the most.
I personally use a command line utility `hg who` which gives me stats about any file.
Example for the file you touched:

> hg who devtools/server/actors/styles.js
>  476 Tom Tromey
>  341 Dave Camp
>  198 Razvan Caliman
>  163 Gabriel Luong
>  135 Julian Descottes
>  118 Heather Arthur
>   87 Patrick Brosset
>   72 Brian Grinstead
>   52 Alexandre Poirot
>   50 J. Ryan
>   30 Cameron McCormack
>   20 Anurag Chaudhury
>   17 Nicolas Chevobbe
>   17 Mark Banner
>   14 Michael Ratcliffe
>   12 Matteo Ferretti
>   10 Jennifer Fong
>   ...

I've got it set up in my ~/.hgrc file like so:

[alias]
who = !$HG anno -u $1 | awk '{print $$1" "$$2}' | sort | uniq -c | sort -r

Now, from this list of people, you can then go to the list of peers at https://wiki.mozilla.org/Modules/All#DevTools and cross-check from there to make sure the person you're intending to ask for review is indeed (still) a peer on that module.

In this case Razvan Caliman, Gabriel Luong, Julian Descottes and myself would be good reviewers for this.

Comment 10

[Patrick Brosset <:pbro>]

We originally switched from rng.selectNode to rng.selectNodeContents when working on bug 920141 as you said. The reason at the time was that the Font inspector panel needed to call this to get the list of fonts for a given node selected in the inspector.
Before that bug, only "normal" nodes could be selected in the inspector. After that bug pseudo elements could also be selected.
And it turns out that calling rng.selectNode on a pseudo-element fails with 
"InvalidNodeTypeError: The supplied node is incorrect or has an incorrect ancestor for this operation".

Now, since then, we also added another feature that depends on this code in bug 994559. This underlines the used font in font-family declarations in the Rules panel.
So, changing back to rng.selectNode will break this too.

In fact test browser_rules_highlight-used-fonts.js should fail, only it won't now because it doesn't seem to be testing pseudo-elements. I think we should add a test case for pseudo-elements there.

For the font editor case, it's not too bad because we don't support pseudo-elements there anymore (the font editor can't write to their `style` attribute. Until we fix bug 1462591 we are unable to support anything that doesn’t have a HTMLElement superclass).

So, I think the way to go here is to use selectNode or selectNodeContents depending on what the current node is.
That means that for pseudo-elements it will keep on using selectNodeContents. And for everything else will be using selectNode which will make it work with input elements too.

I think this would work to test if the node is a pseudo:
const isPseudo = !!CssLogic.getBindingElementAndPseudo(actualNode).pseudo;

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 9023470 [details] [diff] [review]
replace_select_node_contents_for_fonts.patch

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

Thanks Vincent, this looks good. Let's address the pseudo-element failures. I've also made a couple of minor comments below.
I'll push the new patch to try whenever you have time to attach it here.

::: devtools/client/inspector/fonts/test/browser.ini
@@ +30,4 @@
>  [browser_fontinspector_reveal-in-page.js]
>  [browser_fontinspector_text-node.js]
>  [browser_fontinspector_theme-change.js]
> +[browser_fontinspector_input-element-used-font.js]

We usually try to sort the list of tests in alphabetical order. Could you please move to the right spot?

::: devtools/client/inspector/fonts/test/browser_fontinspector_input-element-used-font.js
@@ +3,5 @@
> +"use strict";
> +
> +const TEST_URI = URL_ROOT + "doc_browser_fontinspector.html";
> +
> +// Verfiy that a styled input field element is showing proper font information

typo: Verfiy

::: devtools/server/actors/styles.js
@@ +297,4 @@
>      const contentDocument = actualNode.ownerDocument;
>      // We don't get fonts for a node, but for a range
>      const rng = contentDocument.createRange();
> +    rng.selectNode(actualNode);

See my previous comment about why this will fail with pseudo elements.

Comment 12

[Vincent]

Attachment: replace_select_node_contents_for_fonts.patch

Thanks for the context. I applied the previous review comments.

As well:
 - I added a new test for pseudo element highlighted font in browser_rules_highlight-used-fonts.js (it did break with my previous change btw :))
 - I added the special range selection logic for pseudo elements (all new tests are passing now).

Please take another look to the updated patch.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 9023810 [details] [diff] [review]
replace_select_node_contents_for_fonts.patch

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

Looks great to me. Thanks for this second patch, for addressing my comments and for adding the new test case.
I only have one very minor comment, but no need to ask for a new review for this one (you can simply just push a new patch and mark it as R+ yourself since I already gave R+ here).
In the meantime I'll push this patch to TRY to see how it behaves there.

::: devtools/client/inspector/rules/test/browser_rules_highlight-used-fonts.js
@@ +84,3 @@
>      const onFontHighlighted = view.once("font-highlighted");
> +
> +    if (!!selectBeforePseudoElement) {

nit: the !! doesn't seem necessary here. selectBeforePseudoElement is either a boolean or null (so falsy), so we should be good just doing if (selectBeforePseudoElement)

Comment 14

[Patrick Brosset <:pbro>]

Here is the TRY push URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=601175c2c3ca0a3c653b2f4de3f803b02d920dde

And one more thing I forgot to mention, we use eslint to ensure common formatting of JS code throughout the codebase. ESlint also runs on CI. I noticed while running it locally that the new test you added didn't conform to the eslint rules we currently use.
Here are the warnings:

browser_fontinspector_input-element-used-font.js
  11:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  13:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  14:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  16:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  17:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  19:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  20:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  22:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  23:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  24:5   error  Expected indentation of 2 spaces but found 4.   indent-legacy (eslint)
  24:34  error  Strings must use doublequote.                   quotes (eslint)
  26:4   error  Newline required at end of file but not found.  eol-last (eslint)

Looks like ESlint also caught the !! thing I commented about:

browser_rules_highlight-used-fonts.js
  86:10  error  Redundant double negation.  no-extra-boolean-cast (eslint)

If you want to run ESLint locally to avoid having to wait for results to show on TRY, you can execute ./mach eslint --setup , and then just ./mach eslint /path/to/directory/or/file

Comment 15

[Vincent]

Attachment: replace_select_node_contents_for_fonts.patch

Fixed eslint issues.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 9023891 [details] [diff] [review]
replace_select_node_contents_for_fonts.patch

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

Thanks Vincent. ESlint didn't report any errors when I tested this patch locally. Also TRY shows that all tests passed fine. So let's land this!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1e73f6e59d
Replace selectNodeContents by selectNode to compute font faces in font tab. r=pbro

Comment 18

[Stefan Hindli [:stefan_hindli]]

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