1439629 - InspectorUtils.getUsedFontFaces should not report fonts for hidden elements

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Playing around with a version of the DevTools font-inspector patch from bug 1435989 comment 18, I notice that getUsedFontFaces() is sometimes reporting fonts that don't appear anywhere on the page (or in the inspected element) because they are used by elements that are currently hidden (e.g. toolbars, menus, etc., that are part of the DOM, ready to be "expanded").

I think the usefulness of this API (and the dev tool built on it) would be improved by omitting fonts (and corresponding DOM ranges) for elements that are currently hidden (in the sense that the 'visibility' property is not 'visible').

This won't cover all cases of "invisible" elements: frames may be hidden from the user in lots of ways, e.g. by positioning them far off-screen, or behind other opaque elements, or by giving them 0% opacity, or size zero (and overflow:hidden), etc. But nevertheless, I think checking the explicit 'visibility' property is worthwhile, and should improve the user experience in many cases.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: When collecting used-fontFace information, skip frames where IsVisible() is currently false

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8952406 [details] [diff] [review]
When collecting used-fontFace information, skip frames where IsVisible() is currently false

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

r=me

Comment 3

[Daniel Holbert [:dholbert]]

Could you add a bit of test code for this new behavior? (e.g. to test_fontFaceRanges.xul)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Add testcase to check that element with visibility:hidden is skipped when reporting used font faces

This reports a bunch of failures without the patch, because of the visibility:hidden element that uses an additional font face (capitals) and adds more ranges for the other faces used.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8952442 [details] [diff] [review]
Add testcase to check that element with visibility:hidden is skipped when reporting used font faces

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

Thanks! There's one more edge case that'd be worth testing here though...

::: layout/inspector/tests/chrome/test_fontFaceRanges.xul
@@ +246,5 @@
>            <a href="#foo">linked</a>
>            <!-- comment -->
>        world!
>    </div>
> +  <div class="gentium" id="test6">hello <a href="#foo" style="visibility:hidden">Non-Visible</a> world!</div>

Could you add one more level of nesting here, to test visible-inside-of-invisible?  (just to validate that we don't turn off entire subtrees that do have some visible descendants)

e.g. maybe add a child with style="visibility:visible", inside of the <a> element?

(That'll make this line super-long -- if you like, you could ameliorate that by adding selectors like...
  vis { visibility: visible }
  nonvis { visibility: hidden  }.
...and using <nonvis> instead of <a ...> and then using <vis> as the child that I'm suggesting.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Add testcase to check that element with visibility:hidden is skipped when reporting used font faces

Added visible child of the hidden element, as suggested. And yes, that was a problem with the patch; update coming.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: When collecting used-fontFace information, skip textframes where IsVisible() is currently false

Fixed patch to not skip frames that could have visible descendants.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8952482 [details] [diff] [review]
When collecting used-fontFace information, skip textframes where IsVisible() is currently false

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

r=me

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8952480 [details] [diff] [review]
Add testcase to check that element with visibility:hidden is skipped when reporting used font faces

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

r=me

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28552a0c965f
When collecting used-fontFace information, skip textframes where IsVisible() is currently false. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cdb6db37198
Add testcase to check that element with visibility:hidden is skipped when reporting used font faces. r=dholbert

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/28552a0c965f
https://hg.mozilla.org/mozilla-central/rev/4cdb6db37198