Visual glitches on hover with @font-face font

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sgarrity, Assigned: roc)

Tracking

1.9.1 Branch
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9.2 -
blocking1.9.1.1 -

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 wanted)

Details

Attachments

(5 attachments)

(Reporter)

Description

8 years ago
Created attachment 380438 [details]
Screenshot of glitch during :hover

Using a custom font with the (awesome) new @font-face feature, I'm seeing a glitch. If you change the font color on :hover, most of the font changes, except for part of the font at the very top (ascenders?). These same parts of the letters that are not changing color also appear to not be clickable.

Test case coming...
(Reporter)

Comment 1

8 years ago
Created attachment 380439 [details]
Simple HTML test-case
(Reporter)

Comment 2

8 years ago
Created attachment 380440 [details]
True Type Font used in test-case

Here's the font used in the test-case. I'm not sure if it's specific to this font or not.
(Reporter)

Comment 3

8 years ago
I should have pointed out, this is with the Shiretoko nightly, 20090529, on both Linux and Mac OS X.
Component: General → Layout: View Rendering
Product: Firefox → Core
QA Contact: general → layout.view-rendering
Version: 3.5 Branch → 1.9.1 Branch
This is because by default we don't compute exact glyph bounds, for performance reasons, when the font size is less than 20 pixels. You can work around this by adding text-rendering:optimizeLegibility to your content.

I suppose it would be a good idea to also take the slow path (which also enables Opentype features on Windows and Linux) for all @font-face text. John, what do you think?
As mentioned here: http://weblogs.mozillazine.org/roc/archives/2007/07/status_3.html

Steven, if this fixes your problem let us know.

We should document this - seems pretty important.
Keywords: dev-doc-needed
Flags: wanted1.9.1.x+
Flags: blocking1.9.2-
Keywords: relnote
Created attachment 380667 [details] [diff] [review]
fix

This patch marks gfxFontEntries that correspond to user fonts (@font-face), and when we measure text that uses those fonts, we always get precise glyph bounds. That should fix this bug on all platforms. It's very simple and safe IMHO.
Assignee: nobody → roc
Attachment #380667 - Flags: review?(jdaggett)
Whiteboard: [needs review]
Component: Layout: View Rendering → GFX: Thebes
QA Contact: layout.view-rendering → thebes
Created attachment 380668 [details] [diff] [review]
Force Windows to take Uniscribe path for @font-face fonts

I had also been thinking it would be a good idea to take the shaping path for all gfxFontGroups that contain a user font, on the assumption that authors want us to use typographic features of those fonts. On Mac this is a non-issue, we take the shaping path for all fonts. On GTK I need to talk to Karl to figure out what to do.

This patch does it for Windows, but we have the problem that Uniscribe completely barfs on CFF fonts, so this patch has the side effect of *completely disabling* use of CFF fonts with @font-face, on Windows --- such as the font Steven is using in his testcase. OTOH it looks like Steven's example doesn't work on trunk already --- John, are CFF fonts already completely disabled in @font-face on Windows? If so, we could take this patch since it won't make anything worse.
Karl, see comment #7. The problem on GTK/Pango is that at least for the first time we use a fontgroup, we don't build the list of fonts until after we've chosen which path to take. So some code restructuring is required.
(In reply to comment #7)
> This patch does it for Windows, but we have the problem that Uniscribe
> completely barfs on CFF fonts,

Dynamically loaded fonts, that is.
(Reporter)

Comment 10

8 years ago
Yeah, text-rendering:optimizeLegibility will do the trick for me for now. Thanks.

Comment 11

8 years ago
(In reply to comment #7)
> This patch does it for Windows, but we have the problem that Uniscribe
> completely barfs on CFF fonts, so this patch has the side effect of *completely
> disabling* use of CFF fonts with @font-face, on Windows --- such as the font
> Steven is using in his testcase. OTOH it looks like Steven's example doesn't
> work on trunk already --- John, are CFF fonts already completely disabled in
> @font-face on Windows? If so, we could take this patch since it won't make
> anything worse.

CFF user fonts can only be rendered with the GDI path, Uniscribe doesn't like CFF fonts loaded via AddFontMemResourceEx.  So they will render currently but without kerning/ligatures:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxWindowsFonts.cpp#1805

But even if you force the Uniscribe shaping path, things should still work, since the mForceGDI flag should catch this case.  So I'm puzzled why your patch would disable CFF fonts.
I don't know, but I can't get Steven's font to work with @font-face in my Windows build.
(In reply to comment #7)
> I had also been thinking it would be a good idea to take the shaping path for
> all gfxFontGroups that contain a user font, on the assumption that authors want
> us to use typographic features of those fonts.

(In reply to comment #8)
> Karl, see comment #7. The problem on GTK/Pango is that at least for the first
> time we use a fontgroup, we don't build the list of fonts until after we've
> chosen which path to take. So some code restructuring is required.

Yes, in the code for GTK/Pango, it is not so easy to determine whether a user font might get used (without writing extra code to work it out).

One thing to consider is just testing mUserFontSet.
That would be simple but would mean the shaping path would be used for the whole document.

Note that attachment 380668 [details] [diff] [review] will cause the shaping path to be used even if the user font is only a fallback font (and doesn't actually get used).  So it also can change the rendering with system fonts.  Also, IIUC text using @font-face { src: local(); } will render differently to text using system "font-family"s.
I don't know whether that's bad, but something to consider.

With the Pango shaping path, I don't know whether its the font-selection (pango_itemize) or the shaping (pango_shape) that makes the path slower.
If it is (only) the shaping, then the fast path could be implemented as a really-basic shaper (instead of pango_shape) within CreateGlyphRunsItemizing.
(In reply to comment #13)
> Note that attachment 380668 [details] [diff] [review] will cause the shaping path to be used
> even if the user font is only a fallback font (and doesn't actually get used).
> So it also can change the rendering with system fonts.  Also, IIUC text using
> @font-face { src: local(); } will render differently to text using system
> "font-family"s. I don't know whether that's bad, but something to consider.

Yeah. That wouldn't be bad IMHO.
As a data point, I got the try server to run Tp with the fast path completely disabled with GTK/Pango.  This increased Tp from 224 to 230, a 2-3% increase.

Comment 16

8 years ago
(In reply to comment #12)
> I don't know, but I can't get Steven's font to work with @font-face in my
> Windows build.

The TTLoadEmbeddedFont API is returning an error for this font but Webkit/Opera10 load this fine.  So that's a different bug...

Updated

8 years ago
Attachment #380667 - Flags: review?(jdaggett) → review+

Comment 17

8 years ago
Comment on attachment 380667 [details] [diff] [review]
fix

Looks fine, couple small cleanup details.

> +    PRPackedBool     mIsUserFont  : 1;

There's already a mUserFont in the Windows FontEntry class, that needs to be trimmed out.

>          if (IsType1())
>              mForceGDI = PR_TRUE;
> +        mIsUserFont = aUserFontData != nsnull;

This is already set in Windows code so this can be trimmed I think.
http://hg.mozilla.org/mozilla-central/rev/97f8d01a5585
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Flags: blocking1.9.1.1?
Wanted, but I don't see any reason for it to block 1.9.1.1.
Flags: blocking1.9.1.1? → blocking1.9.1.1-

Updated

8 years ago
status1.9.1: --- → ?
status1.9.1: ? → wanted
Flags: wanted1.9.1.x+
This looks like the patch here undoes the need for any documentation change as specified in comment #5; the patch makes Gecko automatically calculate glyph sizes so that optimizeLegibility is no longer necessary, correct?

If that's true, I think I can simply remove the doc needed tag from this.
Yes.
Keywords: dev-doc-needed
Is this still wanted for 1.9.1? If so, can we get a patch made for that branch? (Or if the current one applies, please request approval.)

(For drivers: Note that this is a known issue we're listing in our release notes.)
Removing relnote
status1.9.2: --- → beta1-fixed
Keywords: relnote

Comment 24

8 years ago
This bug states that relnote was removed, but it is still appearing in the 3.5.7 release notes (http://en-us.www.mozilla.com/en-US/firefox/3.5.7/releasenotes/). Shouldn't it have been removed?

Comment 25

8 years ago
It was removed from the 3.6 release notes and no one backported that change to the 3.5.x release notes. Feel free to file a Websites::www.mozilla.com bug if you care, but it'll be fixed in 3.6.
You need to log in before you can comment on or make changes to this bug.