Closed Bug 1157064 Opened 9 years ago Closed 8 years ago

experimental implementation of font-display CSS @font-face descriptor

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox40 --- wontfix
firefox46 --- fixed

People

(Reporter: jtd, Assigned: jtd)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 2 obsolete files)

Implement support for the experimental 'font-loading-display' property to enable authors to control the display behavior of downloadable fonts while the fonts are downloading. Firefox/Chrome currently shows blank text, then a fallback font after a timeout, before finally displaying the downloadable font when the load completes.

Proposed syntax:

  font-loading-display: auto | fallback | blank-fallback | blank

Description:

  https://lists.w3.org/Archives/Public/www-style/2015Apr/0216.html

Initial implementation will be behind a pref until consensus within the CSSWG is reached.
Chrome has implemented a 'font-display' descriptor for @font-face rules (landed 11/24, Chrome 49 behind experimental flag).

  font-display: auto | block | swap | fallback | optional

where the values mean:

  auto - UA decides, typically like 'block'

  block - blank for 3s, then fallback font. use font when download completes

  swap - show fallback font immediately. use font when download completes

  fallback - blank for 100ms, then fallback font. use font if download completes within 3s

  optional - blank for 100ms, then fallback font. use font if download completes within 100ms

Both 'fallback' and 'optional' will potentially alter the final rendered page, since a font is to be ignored if it doesn't arrive within a finite time limit.

Tab's unofficial spec page for this:
https://tabatkins.github.io/specs/css-font-display/
Blocks: 1229634
Seems like we should extend the IDL for FontFace if adding an additional @font-face descriptor:

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/FontFace.webidl#15

Cam, what do you think?
Summary: experimental implementation of font-loading-display CSS property → experimental implementation of font-display CSS property
(In reply to John Daggett (:jtd) from comment #2)
> Seems like we should extend the IDL for FontFace if adding an additional
> @font-face descriptor:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/FontFace.webidl#15
> 
> Cam, what do you think?

Yes, I agree.  Maybe this can be mentioned in Tab's spec?  (And if the descriptor is going to live behind a pref initially we can do that for the FontFace interface's new IDL attribute too.)
Implementing font-display is a fairly easy extension of existing behavior. The behavior of auto/block are precisely existing behavior. For swap/fallback/optional, the initial timeout during which invisible text is displayed is reduced to 100ms (from 3000ms). For this I've set up a separate pref, "gfx.downloadable_fonts.fallback_delay_short", that defaults to 100. For fallback, at font completion the load time is checked. If more than the allowed 3000ms has passed, set the status to LOADING_TIMED_OUT to avoid using the font. For optional, after the timeout set the status to LOADING_TIMED_OUT.
These test only the "final" result in situations where font-display is used. They only test this for short font loads vs. long font loads. I tried to come up with tests of the intermediate behavior (i.e. blank text vs fallback text) but that's so sensitive to timing that it's hard to come up with something reasonable. For now, I think just testing final behavior is fine.
Attachment #8696385 - Flags: review?(jfkthame)
Attachment #8695142 - Flags: review?(dbaron)
Attachment #8695143 - Flags: review?(cam)
Comment on attachment 8695143 [details] [diff] [review]
patch, font downloads respect font-display behavior

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

Some comments and a couple of questions.

Please get a DOM peer's review for the FontFace.webidl change.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +754,5 @@
>          free((void*)aFontData);
>      }
>  
>      // error occurred, load next src
> +    if (mFontDataLoadingState != LOADING_TIMED_OUT) {

Can you comment in here that we don't need to bother looking at the next src if we know we've passed the timeout?

::: gfx/thebes/gfxUserFontSet.h
@@ +595,5 @@
>      gfxCharacterMap* GetUnicodeRangeMap() const {
>          return mCharacterMap.get();
>      }
>  
> +    uint8_t GetFontDisplay() { return mFontDisplay; }

This can be const.

@@ +682,5 @@
>      // methods of nsFontFaceLoader this reference is nulled out.
>      nsFontFaceLoader* MOZ_NON_OWNING_REF mLoader; // current loader for this entry, if any
>      gfxUserFontSet*          mFontSet; // font-set which owns this userfont entry
>      nsCOMPtr<nsIPrincipal>   mPrincipal;
> +    uint8_t                  mFontDisplay; // timing of userfont fallback

May as well move this up next to mUnsupportedFormat to get better packing.

Since we're just starting to migrate CSS property value constants in nsStyleConsts.h to use enum classes rather than #defines and uint8_t, I wonder if we could do the same for CSS descriptor values (in the previous patch) and then use that instead of uint8_t here and in the various method arguments.

::: layout/style/FontFace.cpp
@@ +358,5 @@
> +void
> +FontFace::GetDisplay(nsString& aResult)
> +{
> +  mFontFaceSet->FlushUserFontSet();
> +  GetDesc(eCSSFontDesc_Display, eCSSProperty_UNKNOWN, aResult);

I think you'll need to adjust FontFace::GetDesc to handle eCSSFontDesc_Display, both to serialize the default "auto" value and to call something like nsStyleUtil::AppendFontDisplay since we can't use nsCSSValue::AppendToString.

::: layout/style/nsFontFaceLoader.cpp
@@ +134,5 @@
> +    case NS_FONT_DISPLAY_SWAP:
> +      ufe->mFontDataLoadingState = gfxUserFontEntry::LOADING_SLOWLY;
> +      break;
> +    case NS_FONT_DISPLAY_FALLBACK:
> +    {

Brace should go on the previous line, according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures.

@@ +135,5 @@
> +      ufe->mFontDataLoadingState = gfxUserFontEntry::LOADING_SLOWLY;
> +      break;
> +    case NS_FONT_DISPLAY_FALLBACK:
> +    {
> +      if (ufe->mFontDataLoadingState == gfxUserFontEntry::LOADING_STARTED) {

Can you explain what other values of ufe->mFontDataLoadingState we might get here?  I'm trying to understand why we sometimes would set to LOADING_SLOWLY and sometimes to LOADING_TIMED_OUT.

@@ +144,5 @@
> +      }
> +      break;
> +    }
> +    case NS_FONT_DISPLAY_OPTIONAL:
> +      ufe->mFontDataLoadingState = gfxUserFontEntry::LOADING_TIMED_OUT;

Does this mean that we will never download the font if we do need to download, but we can still get it out of the font cache?

@@ +191,5 @@
>    TimeDuration downloadTime = doneTime - mStartTime;
>    uint32_t downloadTimeMS = uint32_t(downloadTime.ToMilliseconds());
>    Telemetry::Accumulate(Telemetry::WEBFONT_DOWNLOAD_TIME, downloadTimeMS);
>  
> +  if (GetFontDisplay() == NS_FONT_DISPLAY_FALLBACK) {

From this block of code I take it that if we are loading a font and we go past the 3s timeout, we don't make any effort to cancel the load even when font-display is fallback.  Is that the right thing to do?  Do we want to continue the download so that the next time we load this/another page, we'll have the font ready?

@@ +193,5 @@
>    Telemetry::Accumulate(Telemetry::WEBFONT_DOWNLOAD_TIME, downloadTimeMS);
>  
> +  if (GetFontDisplay() == NS_FONT_DISPLAY_FALLBACK) {
> +    uint32_t loadTimeout =
> +      Preferences::GetInt("gfx.downloadable_fonts.fallback_delay", 3000);

Since you get this pref in a couple of places, can you abstract it out into a static inline function so we don't have to repeat the pref name and default value?
Attachment #8695143 - Flags: review?(cam)
> ::: layout/style/FontFace.cpp
> @@ +358,5 @@
> > +void
> > +FontFace::GetDisplay(nsString& aResult)
> > +{
> > +  mFontFaceSet->FlushUserFontSet();
> > +  GetDesc(eCSSFontDesc_Display, eCSSProperty_UNKNOWN, aResult);
> 
> I think you'll need to adjust FontFace::GetDesc to handle
> eCSSFontDesc_Display, both to serialize the default "auto" value and
> to call something like nsStyleUtil::AppendFontDisplay since we can't
> use nsCSSValue::AppendToString.

Hmmm, not sure I see what you're saying there. I tested this and all values properly serialize.

> @@ +135,5 @@
> > +      ufe->mFontDataLoadingState = gfxUserFontEntry::LOADING_SLOWLY;
> > +      break;
> > +    case NS_FONT_DISPLAY_FALLBACK:
> > +    {
> > +      if (ufe->mFontDataLoadingState == gfxUserFontEntry::LOADING_STARTED) {
> 
> Can you explain what other values of ufe->mFontDataLoadingState we
> might get here?  I'm trying to understand why we sometimes would set
> to LOADING_SLOWLY and sometimes to LOADING_TIMED_OUT.

Added a comment for this above the switch statement.

> Since we're just starting to migrate CSS property value constants in
> nsStyleConsts.h to use enum classes rather than #defines and uint8_t,
> I wonder if we could do the same for CSS descriptor values (in the
> previous patch) and then use that instead of uint8_t here and in the
> various method arguments.

Sounds reasonable. If dbaron thinks that's the way to go will do that.

> >    TimeDuration downloadTime = doneTime - mStartTime;
> >    uint32_t downloadTimeMS = uint32_t(downloadTime.ToMilliseconds());
> >    Telemetry::Accumulate(Telemetry::WEBFONT_DOWNLOAD_TIME, downloadTimeMS);
> >  
> > +  if (GetFontDisplay() == NS_FONT_DISPLAY_FALLBACK) {
> 
> From this block of code I take it that if we are loading a font and we
> go past the 3s timeout, we don't make any effort to cancel the load
> even when font-display is fallback.  Is that the right thing to do? 
> Do we want to continue the download so that the next time we load
> this/another page, we'll have the font ready?

In Tab's spec the only difference between 'fallback' and 'optional' is the value of the timeout. For 'fallback' it's basically 3s, for 'optional' it's 100ms. No description is given concerning the state of the font load past the 3s timeout for the 'fallback' value. For the 'optional' value, the spec leaves this somewhat unspecified:

"If the font is not retrieved before the two durations expire, the user agent may choose to abort the font download, or download it with a very low priority. If the user agent believes it would be useful for the user, it may avoid even starting the font download, and proceed immediately to using a fallback font."

For both 'fallback' and 'optional', if the font load is always aborted and the load never completes, the font will *never* be used, particularly in the 'optional' case. I pointed this out previously but I don't think Tab ever addressed that.

The Chrome implementation of 'optional' sniffs the network cache and only loads the font if it's in the network cache. So unless there is some other usage that explicitly loads the font (e.g. FontFace.load() or link rel="xxx"), 'optional' will effectively be 'never use this font'. Not sure what the Chrome implementation does for the 'fallback' value.
Attachment #8695143 - Attachment is obsolete: true
Attachment #8697917 - Flags: review?(cam)
Summary: experimental implementation of font-display CSS property → experimental implementation of font-display CSS @font-face descriptor
Comment on attachment 8695142 [details] [diff] [review]
patch, parse font-display descriptor for @font-face rules

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>@@ -3927,17 +3927,19 @@ CSSParserImpl::ParseFontDescriptor(nsCSS
>   nsCSSFontDesc descID = nsCSSProps::LookupFontDesc(descName);
>   nsCSSValue value;
> 
>-  if (descID == eCSSFontDesc_UNKNOWN) {
>+  if (descID == eCSSFontDesc_UNKNOWN ||
>+      (descID == eCSSFontDesc_Display &&
>+       !Preferences::GetBool("layout.css.font-display.enabled"))) {

Instead of putting the pref check here, please put it in both versions of nsCSSProps::LookupFontDesc, so that we hit it every time we convert a string to a descriptor ID rather than just this one place.

>+const KTableEntry nsCSSProps::kFontDisplayTable[] = {

For consistency, please call this kFontDisplayKTable (note K before Table).

r=dbaron with that


Will there be any need to add a fontDisplay property to the FontFace object or to CSS2Properties?  That doesn't need to happen now, but it might need to before we ship.  Could you file appropriate bugs (both on enabling the preference and on fixiing that if needed, with blocking set appropriately, including making this bug block the bug on enabling)?
Attachment #8695142 - Flags: review?(dbaron) → review+
(In reply to John Daggett (:jtd) from comment #9)
> > I think you'll need to adjust FontFace::GetDesc to handle
> > eCSSFontDesc_Display, both to serialize the default "auto" value and
> > to call something like nsStyleUtil::AppendFontDisplay since we can't
> > use nsCSSValue::AppendToString.
> 
> Hmmm, not sure I see what you're saying there. I tested this and all values
> properly serialize.

I hit the assert at the top of FontFace::GetDesc:

  Assertion failure: aDescID == eCSSFontDesc_UnicodeRange || aPropID != eCSSProperty_UNKNOWN (only pass eCSSProperty_UNKNOWN for eCSSFontDesc_UnicodeRange), at /z/moz/c/layout/style/FontFace.cpp:602

with this document:

<!DOCTYPE html>
<style>
@font-face {
  font-family: TestFont;
  src: url(x);
  font-display: fallback;
}
</style>
<script>
var f = [...document.fonts][0];
alert(f.display);
</script>

since aPropID is eCSSProperty_UNKNOWN.

> > @@ +135,5 @@
> > > +      ufe->mFontDataLoadingState = gfxUserFontEntry::LOADING_SLOWLY;
> > > +      break;
> > > +    case NS_FONT_DISPLAY_FALLBACK:
> > > +    {
> > > +      if (ufe->mFontDataLoadingState == gfxUserFontEntry::LOADING_STARTED) {
> > 
> > Can you explain what other values of ufe->mFontDataLoadingState we
> > might get here?  I'm trying to understand why we sometimes would set
> > to LOADING_SLOWLY and sometimes to LOADING_TIMED_OUT.
> 
> Added a comment for this above the switch statement.

OK, so when ufe->mFontDataLoadingState == gfxUserFontEntry::LOADING_STARTED we know that this is the first (shorter) timeout callback, and so we want to display the fallback immediately, but in the else case it's the second (longer) timeout callback, and so we want to prevent the downloaded font from showing.  If that's right, can you just comment that that's what the mFontDataLoadingState check achieves, since I think it's slightly non-obvious.

> The Chrome implementation of 'optional' sniffs the network cache and only
> loads the font if it's in the network cache. So unless there is some other
> usage that explicitly loads the font (e.g. FontFace.load() or link
> rel="xxx"), 'optional' will effectively be 'never use this font'. Not sure
> what the Chrome implementation does for the 'fallback' value.

OK.  I worry we might be forced to converge on the same behaviour, but we'll see.
Comment on attachment 8697917 [details] [diff] [review]
patch p2 - font downloads respect font-display behavior

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

r- just for the FontFace DOM property serialization issues.

::: gfx/thebes/gfxUserFontSet.cpp
@@ +118,5 @@
>        mFontDataLoadingState(NOT_LOADING),
>        mUnsupportedFormat(false),
>        mLoader(nullptr),
> +      mFontSet(aFontSet),
> +      mFontDisplay(aFontDisplay)

Move this to just below mUnsupportedFormat to avoid an out-of-order-initialization warning.

::: layout/style/nsFontFaceLoader.cpp
@@ +31,5 @@
>  #define LOG_ENABLED() MOZ_LOG_TEST(gfxUserFontSet::GetUserFontsLog(), \
>                                    LogLevel::Debug)
>  
> +static uint32_t
> +GetFallbackDelay() {

Nit: brace on next line (and in the next function).
Attachment #8697917 - Flags: review?(cam) → review-
Fix assertion problem.
Attachment #8697917 - Attachment is obsolete: true
Attachment #8704421 - Flags: review?(cam)
Will ask r? to bz for the WebIDL change once he clears the "no reviews" flag... ;)
Attachment #8696385 - Flags: review?(jfkthame) → review?(m_kato)
Comment on attachment 8704421 [details] [diff] [review]
patch p2 - font downloads respect font-display behavior

r? khuey for the change to FontFace.webidl
Attachment #8704421 - Flags: review?(khuey)
Comment on attachment 8704421 [details] [diff] [review]
patch p2 - font downloads respect font-display behavior

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

Can you extend test_font_loading_api.html with the new descriptor?  I think you should just be able to extend the variables with the descriptor names/values at the top of the test, and pushPrefEnv the pref when calling runTest() at the bottom.  Also, in part 1 should you have added the new descriptor to descriptor_database.js?  r=me with that.

::: dom/webidl/FontFace.webidl
@@ +37,5 @@
>    [SetterThrows] attribute DOMString stretch;
>    [SetterThrows] attribute DOMString unicodeRange;
>    [SetterThrows] attribute DOMString variant;
>    [SetterThrows] attribute DOMString featureSettings;
> +  [SetterThrows] attribute DOMString display;

I realized this should probably have a [Pref="..."] on it so that it isn't exposed unless the pref is set.
Attachment #8704421 - Flags: review?(cam) → review+
Comment on attachment 8704421 [details] [diff] [review]
patch p2 - font downloads respect font-display behavior

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

r=me with heycam's suggestion.
Attachment #8704421 - Flags: review?(khuey) → review+
Comment on attachment 8696385 [details] [diff] [review]
patch, font-display reftests

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

cancel review until new test file (font-display-2.html) is added
Attachment #8696385 - Flags: review?(m_kato)
As per review comments, add more test coverage and a couple follow-up fixes.
Attachment #8704512 - Flags: review?(cam)
Attachment #8704512 - Flags: review?(cam) → review+
Fixed the missing files. Use a slight delay on the 'optional' font in the first test, as opt builds loading fonts locally load the font "too quickly", causing the test to fail on some platforms.
Attachment #8704932 - Flags: review?(m_kato)
Attachment #8704932 - Flags: review?(m_kato) → review+
See Also: → 1296373
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: