1435989 - Help devtools highlight glyphs rendered using a specific font in the page

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Description

[Patrick Brosset <:pbro>]

Here's the context: there's a fonts panel in devtools which, currently, lists fonts used to render content on the page (either all fonts, or fonts used inside a given element).
The idea is for devtools to provide an easy way to reveal where a specific font was used in the page. This could be a button or on hover, or anything.
It would especially be interesting when a text node was rendered with multiple fonts, for people to know which of the glyphs caused a fallback font to be used.

Jonathan Kew and I talked about it on IRC today. Here's one possible approach that came out of this discussion:

--> An API that returns the list of runs of text and fonts for a given text node.

This way we could then iterate over text nodes (rather than individual characters) in the page (or under a given element).

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

Here's a possible approach: this patch extends the font-face objects that are returned by InspectorUtils.getUsedFontFaces() to have a 'ranges' attribute, which will be an array of Range objects corresponding to ranges of text where this font was used. So you can then hand those ranges to Selection.addRange(), query them for getClientRects(), etc. You'll only get ranges returned if you pass a second argument (maxRanges) to getUsedFontFaces, and it will return up to maxRanges separate ranges for each face. I'd suggest using a fairly modest limit, to avoid creating many thousands of Range objects when dealing with a large document. Anyhow, take a look and see if this seems useful for your purposes.

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8950986 [details] [diff] [review]
Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

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

This sounds really amazing and I can already see how we could put that to good use in DevTools. Thank you Jonathan.

There's just one edge case that I'm wondering about: what if one of the ranges returned is transformed via CSS (say it's in an element that is rotated or skewed)?

getClientRects will only return the bigger rectangle that contains the rotated or skewed range, and we won't easily be able to just highlight that range exactly.
For our other highlighters, we usually rely on el.getBoxQuads for this, and use the p1, p2, p3, p4 points that this function returns in order to draw the right shape.
Looking at the API exposed on Ranges, I don't think there's anything similar we could do. And I think we could live with this limitation. It is an edge case after all.

Comment 3

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset <:pbro> from comment #2)
> Comment on attachment 8950986 [details] [diff] [review]
> There's just one edge case that I'm wondering about: what if one of the
> ranges returned is transformed via CSS (say it's in an element that is
> rotated or skewed)?
> 
> getClientRects will only return the bigger rectangle that contains the
> rotated or skewed range, and we won't easily be able to just highlight that
> range exactly.
> For our other highlighters, we usually rely on el.getBoxQuads for this, and
> use the p1, p2, p3, p4 points that this function returns in order to draw
> the right shape.
> Looking at the API exposed on Ranges, I don't think there's anything similar
> we could do. And I think we could live with this limitation. It is an edge
> case after all.

That sounds like a use case to let the Range interface implement getBoxQuads() (or the whole GeometryUtils interface[1]). Therefore I've suggested that to the CSSWG[2].

A workaround would be to duplicate the page fragment containing the range, wrapping the range into an inline element and then call getBoxQuads() on it to get the correct shape. This is admittedly hacky and surely doesn't cover all edge cases, but better than nothing, I guess.

Sebastian

[1] https://drafts.csswg.org/cssom-view/#the-geometryutils-interface
[2] https://github.com/w3c/csswg-drafts/issues/2311

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Test for the InspectorFontFace.ranges attribute

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8951241 [details] [diff] [review]
Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

>+  [NewObject, Throws] sequence<InspectorFontFace> getUsedFontFaces(
>+      Range range,
>+      optional unsigned long maxRanges = 0);

This needs documentation.  What does the "maxRanges" argument do?  It's sort of documented down by the "ranges" attribute, but that's a long way from here.

Also need to document this in the various nsLayoutUtils.h methods where it's used.

>+  // Note that this is NOT live: it indicates how the document was rendered at the time of

It probably makes sense to file a followup to make these StaticRange once we support that.  Or just make them dictionaries, either way...  In any case, there is no such thing as a "non-live" Range.

>+      nsCOMPtr<nsIDOMNode> container = do_QueryInterface(aContent);
>+      if (container) {

This bit doesn't make sense.  If aContent is non-null, it will always QI to nsIDOMNode.  But why do you need nsIDOMNode anyway?  All the nsIDOMNode version of CreateRange will do is QI to nsINode and call the nsINode version.  So just call the nsINode version directly.

r=me with that fixed.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> >+  // Note that this is NOT live: it indicates how the document was rendered at the time of
> 
> It probably makes sense to file a followup to make these StaticRange once we
> support that.  Or just make them dictionaries, either way...  In any case,
> there is no such thing as a "non-live" Range.

I've rephrased the comment here. I didn't mean the Ranges themselves were non-live, but the list of them in the .ranges attribute, which is fixed at the time getUsedFontFaces() walks the document and won't change even if the doc is then restyled so that the face is no longer being used for those fragments of text.

Making them StaticRange sounds good; I'll file a followup.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

Updated for review comments; carrying forward r=bz, but :dholbert you're welcome to take a look too. :)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8951242 [details] [diff] [review]
Test for the InspectorFontFace.ranges attribute

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

::: layout/inspector/tests/chrome/test_fontFaceRanges.xul
@@ +30,5 @@
> +
> +  // Test element contains Latin & Chinese
> +  elem = document.getElementById("test1");
> +  rng.selectNode(elem);
> +  fonts = InspectorUtils.getUsedFontFaces(rng, 10);

Could you add a test somewhere to validate that maxRanges is actually respected, in cases where there are more matches than the passed-in maxRanges?

(For example: maybe copypaste this whole "test1" chunk, but pass 1 to getUsedFontFaces instead of passing 10. I'm guessing that the expectRanges(...) calls in this repasted chunk would all take an array with just the first entry.)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8951242 [details] [diff] [review]
Test for the InspectorFontFace.ranges attribute

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

::: layout/inspector/tests/chrome/test_fontFaceRanges.xul
@@ +37,5 @@
> +    var f = fonts[i];
> +    switch (true) {
> +    case f.CSSFamilyName == "capitals":
> +      expectRanges(f, ["H", "W"]);
> +      break;

This switch-based testing feels a little problematic/inadvertantly-permissive...  My evil not-too-contrived scenario: suppose "fonts" happens to contain 4 duplicate copies of the same object (say, 4 copies of the "capitals" result that you check for here). In that scenario, we'd still pass this test without complaining at all, and we'd be giving the implication that we found "lowercase", "gentium", and Chinese results, even though we really didn't!

One possible quick-and-easy way to address this: maybe you could declare an (initially-empty) dictionary of observed font names, and insert |f.CSSFamilyName| into it on each loop iteration here, and then assert that its length is 4 at the end of the loop, with "found all expected families" as the message or something like that. And then clear that out just before each subsequent loop, & repeat. (I can think of more robust ways, too, but they'd require more rewriting that's harder to justify.)

Comment 11

[Jonathan Kew [:jfkthame]]

Good point, it's not really strict enough. I'll re-write, thanks.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Test for the InspectorFontFace.ranges attribute

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8951316 [details] [diff] [review]
Test for the InspectorFontFace.ranges attribute

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

r=me, one nit:

::: layout/inspector/tests/chrome/test_fontFaceRanges.xul
@@ +133,5 @@
> +      expectRanges(f, [" ", " ", "!"]);
> +      break;
> +    default:
> +      // There shouldn't be any other font used
> +      is(f.CSSFamilyName, undefined, "unexpected font");

This is(...) comparison feels a bit bogus.  Superficially, it suggests that we're actually expecting an "f" with its CSSFamilyName property left undefined -- and I think this will produce mochitest output with "expected value: undefined" or something like that, which looks a bit confusing/broken.

IIRC "ok(false,...)" is the right idiom for unreachable-code sorts of things in mochitests. So, this would maybe be clearer:
  ok(false, "Found unexpected font, with family name '" + f.CSSFamilyName + "'");

@@ +161,5 @@
> +      expectRanges(f, ["!"]);
> +      break;
> +    default:
> +      // There shouldn't be any other font used
> +      is(f.CSSFamilyName, undefined, "unexpected font");

(Same here.)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8951282 [details] [diff] [review]
Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font

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

r=me with one nit

(Remember to add r=dholbert to commit message before landing, too -- right now it just has r=bz)

::: layout/inspector/InspectorFontFace.h
@@ +44,5 @@
>    void AddMatchType(uint8_t aMatchType) { mMatchType |= aMatchType; }
>  
> +  void AddRange(nsRange* aRange);
> +  uint32_t RangeCount() const {
> +    return mRanges.Length();

Nit: this RangeCount method should return size_t, not uint32_t (because nsTArray::Length() returns size_t these days)

Otherwise this will be truncating the returned Length() value. (Granted -- in practice, this is fine, because nsTArrays limit themselves to having no more than UINT32_MAX entries, but still better to be type-correct.)

(This will mean that at this method's callsite in nsLayoutUtils.cpp, we'll be doing a size_t vs. uint32_t "less-than" comparison.  That's fine, because if size_t happens to be wider than uint32_t, we'll just upconvert the uint32_t value for the comparison, and we'll still get the correct result.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df945031ffcd
Add a 'ranges' attribute to the InspectorFontFace object, to expose ranges of text that were rendered with a specific font. r=bz,dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ed4ca56120b
Test for the InspectorFontFace.ranges attribute. r=dholbert

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/df945031ffcd
https://hg.mozilla.org/mozilla-central/rev/3ed4ca56120b

Comment 17

[Patrick Brosset <:pbro>]

Thank you Jonathan, this is great.
I'm currently writing some code in DevTools that utilizes this API, but I'm hitting a browser crash.

I do something like this:

    // Find all font faces under a given node
    let r = this.doc.createRange();
    r.selectNodeContents(someNodeOnThePage);
    let fonts = InspectorUtils.getUsedFontFaces(r, MAX_TEXT_RANGES);

    // Find just one that we're interested in
    let font = fonts.find(f => f.name === someFontFaceName);

    // Log the ranges
    console.log(font.ranges);

It doesn't do it for every font name it seems, but for a lot of them, the tab just crashes.
I'm seeing some logs:

[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
[Child 2133, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/dom/base/nsRange.cpp, line 323
Assertion failure: value, at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1092
#01: mozilla_dump_image[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25354c5]
#02: mozilla_dump_image[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2566f50]
#03: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc15df]
#04: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc125d]
#05: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc27af]
#06: thread-local wrapper routine for js::Mutex::HeldMutexStack[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57919dd]
#07: thread-local wrapper routine for js::Mutex::HeldMutexStack[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57921bf]
#08: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55e1c16]
#09: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55c842d]
#10: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55cfd69]
#11: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x55cfa67]
#12: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x54e4668]
#13: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc6475]
#14: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fb6657]
#15: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fafa47]
#16: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc120f]
#17: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fb8377]
#18: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fafa47]
#19: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc120f]
#20: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc1bed]
#21: thread-local wrapper routine for js::Mutex::HeldMutexStack[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5725ff7]
#22: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc15df]
#23: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc125d]
#24: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fb8377]
#25: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fafa47]
#26: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4fc120f]
#27: thread-local wrapper routine for js::oom::threadType[/Users/pbrosset/dev/mozilla-central/objdir-artifact.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x51174a6]

I don't have a proper full debug build, so I unfortunately can't provide much more than this.
I'll attach the patch and instructions to reproduce this though.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: highlight-ranges.diff

See my previous comment.
Here's how to reproduce the crash:
- apply this patch and build
- open https://www.mozilla.org/en-US/
- open the inspector, and switch to the Fonts tab in the sidebar
- hover over the name of the first font in the Fonts tab (Open Sans Regular)
--> the tab should crash.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Are we hitting a case where CreateRange() is failing and we're putting null into the array?

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> Are we hitting a case where CreateRange() is failing and we're putting null
> into the array?

That doesn't seem to be the case: I added an assertion to check that the range is non-null, and didn't hit it.

The first clear sign of trouble seems to be when we hit the "Bad wrapper flag bits" assertion in HasWrapperFlag(), being called from somewhere in InspectorFontFaceBinding::get_ranges.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Well, the assert in comment 17 is what happens when you try to convert nullptr to an interface type that bindings were told can't be null, hence my question.

Looking at this in rr, after the GetRanges() call on the InspectorFontFaces, we have an nsTArray of length 10 with null at indices 1, 4, 5.

Where are those nulls coming from?  Well, we called InspectorFontFace::AddRange with null.  And that happened because CreateRange returned 0x80530001 which is NS_ERROR_DOM_INDEX_SIZE_ERR.  So it is in fact failing.

When that happens, aContent is an nsTextNode, start == 0, end == 6.  The length of the textnode is 2.  So the end index passed to CreateRange is past the end of the textnode, which is why the CreateRange call fails.

In this case, iter.GetStringStart() == 0 and iter.GetStringEnd() == 1.  aSkipIter.ConvertSkippedToOriginal() converts the 1 to 6.

In case it matters, the contents of our textnode are "\n\n".  

Are the indices returned from aSkipIter.ConvertSkippedToOriginal() supposed to be indices into a textnode or into a textrun?  Because a single textrun can span multiple textnodes, of course.

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #20)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #19)
> > Are we hitting a case where CreateRange() is failing and we're putting null
> > into the array?
> 
> That doesn't seem to be the case: I added an assertion to check that the
> range is non-null, and didn't hit it.

Forget what I said there, that was user error on my side. :( As you note, that is in fact where we're failing. I'll look at this later on, no time this morning.

Comment 23

[Patrick Brosset <:pbro>]

Thank you Jonathan, Boris and Daniel. I see that this lead to 2 bugs that are already fixed!