Closed Bug 1439629 Opened 4 years ago Closed 4 years ago

InspectorUtils.getUsedFontFaces should not report fonts for hidden elements

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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
Attachment #8952406 - Flags: review?(dholbert) → review+
Could you add a bit of test code for this new behavior? (e.g. to test_fontFaceRanges.xul)
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.
Attachment #8952442 - Flags: review?(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.)
Added visible child of the hidden element, as suggested. And yes, that was a problem with the patch; update coming.
Attachment #8952480 - Flags: review?(dholbert)
Attachment #8952442 - Attachment is obsolete: true
Attachment #8952442 - Flags: review?(dholbert)
Fixed patch to not skip frames that could have visible descendants.
Attachment #8952482 - Flags: review?(dholbert)
Attachment #8952406 - Attachment is obsolete: true
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
Attachment #8952482 - Flags: review?(dholbert) → review+
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
Attachment #8952480 - Flags: review?(dholbert) → review+
Pushed by jkew@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/28552a0c965f
https://hg.mozilla.org/mozilla-central/rev/4cdb6db37198
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.