Closed Bug 458878 Opened 12 years ago Closed 12 years ago

{inc}@font-face: <length>'s specified in 'ex' unit are incorrect, using metrics from fallback font

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: phiw2, Assigned: dbaron)

References

Details

(Keywords: fixed1.9.1)

Attachments

(4 files, 5 obsolete files)

17.40 KB, text/html
Details
4.13 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
10.89 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
9.53 KB, patch
jtd
: review+
Details | Diff | Splinter Review
http://dev.l-c-n.com/css3/font-face/ex-ahem_a.html
test case with downloadable font, requires Ahem installed

In the linked test case, within each grey block: the first red line uses @font-face, the second line uses the installed copy of Ahem. Both red lines should be identical.

Properties affected (or tested so far…):
padding, margin, line-height, border, -moz-border-radius, text-shadow, width/height (does strange things, sometimes the width is '0'), letter-spacing, word-spacing.

Tested on OS X 10.5.5
Attached file test case (with Ahem)
Same as linked test in comment 0, but with data url.

In both cases, the fall-back font is Times.

Aspect ratio for Times is 0.454, for Ahem: 0.8.
Blocks: 70132
Flags: wanted1.9.1?
OS: Mac OS X → All
Hardware: PC → All
So I think the problem here is that when a user font set changes, we need to recompute style data using a clean root rule node rather than caching old computations in the rule tree, since cached computations of 'ex' and 'ch' in the rule tree are no longer valid.  I'd meant to file a bug about this, but I forgot; I'm glad somebody else caught it.

I think this should be a reasonably simple patch; I'll try writing it in a bit.
Assignee: nobody → dbaron
Component: GFX: Thebes → Style System (CSS)
QA Contact: thebes → style-system
Summary: @font-face: <length>'s specified in 'ex' unit are incorrect, using metrics from fallback font → {inc}@font-face: <length>'s specified in 'ex' unit are incorrect, using metrics from fallback font
Flags: blocking1.9.1?
Attachment #350581 - Flags: superreview?(bzbarsky)
Attachment #350581 - Flags: review?(bzbarsky)
Right now, I think this is probably no more efficient as doing the StyleChangeReflow separately at the caller.  However, I think it gives us a clearer path to optimize in the future.
Attachment #350582 - Flags: superreview?(bzbarsky)
Attachment #350582 - Flags: review?(bzbarsky)
... and the actual fix
Attachment #350583 - Flags: superreview?(bzbarsky)
Attachment #350583 - Flags: review?(jdaggett)
Comment on attachment 350583 [details] [diff] [review]
patch 3: rebuild style data when user font set changes

Actually, hold on a bit...
Attachment #350583 - Flags: superreview?(bzbarsky)
Attachment #350583 - Flags: review?(jdaggett)
I need to figure out why this has made layout/reftests/font-face/cross-iframe-1.html not display correctly until just *after* onload (thus making the reftest fail).

I should also add an individual reftest...
(In reply to comment #7)
> I need to figure out why this has made
> layout/reftests/font-face/cross-iframe-1.html not display correctly until just
> *after* onload (thus making the reftest fail).

Er, sorry, it's a problem with cross-iframe-1-ref.html, and it's permanent.

> I should also add an individual reftest...

by which I meant a test for this bug.
(In reply to comment #5)
> Created an attachment (id=350583) [details]
> patch 3: rebuild style data when user font set changes

Adding these patches to my Linux builds, which already included the patches for bug 458169 and bug 467084, results in the testcases for this bug, bug 467804 and bug 458863 all passing.

However, the top row of magenta dots is back in the Acid3 test, at least under Linux.
Ah -- I need to go back to my original idea for passing the extra hint.  Associating it as a forced change for the root element doesn't work because that ignores absolute and fixed positioned elements that are not descendants of the root's primary frame.
Attachment #350583 - Attachment is obsolete: true
Attachment #350582 - Attachment is obsolete: true
Attachment #350582 - Flags: superreview?(bzbarsky)
Attachment #350582 - Flags: review?(bzbarsky)
This gets rid of the extra restyle posted to the root and fixes the issue in the previous comments.  (Though there might be other bugs lurking due to the underlying issue.)
Attachment #350590 - Flags: superreview?(bzbarsky)
Attachment #350590 - Flags: review?(bzbarsky)
With tests.

There are still some problems with patch 2, though, although it passes all the @font-face tests.
Attachment #350590 - Flags: superreview?(bzbarsky)
Attachment #350590 - Flags: review?(bzbarsky)
Instead of assuming that I'm the only caller of nsPresShell::ReconstructStyleDataInternal and changing what it does, I'll just stop calling it and do what I need.

I'm going to wait a little bit to request reviews this time...
Attachment #350590 - Attachment is obsolete: true
Attachment #350595 - Flags: superreview?(bzbarsky)
Attachment #350595 - Flags: review?(bzbarsky)
Attachment #350596 - Flags: superreview?(bzbarsky)
Attachment #350596 - Flags: review?(jdaggett)
(In reply to comment #13)
> Created an attachment (id=350595) [details]
> patch 2: ability to post a rebuild with an extra hint
> 
> Instead of assuming that I'm the only caller of
> nsPresShell::ReconstructStyleDataInternal and changing what it does, I'll just
> stop calling it and do what I need.
> 
> I'm going to wait a little bit to request reviews this time...

That seems much better!

(I was wondering where those other 11 points went :-) )
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P2
Attachment #350581 - Flags: superreview?(bzbarsky)
Attachment #350581 - Flags: superreview+
Attachment #350581 - Flags: review?(bzbarsky)
Attachment #350581 - Flags: review+
Comment on attachment 350581 [details] [diff] [review]
patch 1: remove some stub units code

r+sr=bzbarsky
Comment on attachment 350595 [details] [diff] [review]
patch 2: ability to post a rebuild with an extra hint

>+++ b/layout/base/nsCSSFrameConstructor.cpp
> nsCSSFrameConstructor::ProcessPendingRestyles()
>   if (mRebuildAllStyleData) {
>     // We probably wasted a lot of work up above, but this seems safest
>     // and it should be rarely used.

Want to file a followup to skip the stuff above if mRebuildAllStyleData and just clear the pending restyle hashtable instead in that case?  I guess either way is fine.

>+++ b/layout/base/nsCSSFrameConstructor.h
>+  void PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint);

Should probably document what aExtraHint does.

>+++ b/layout/base/nsPresContext.h
>   void RebuildAllStyleData(nsChangeHint aExtraHint);
>-  void PostRebuildAllStyleDataEvent();
>+  void PostRebuildAllStyleDataEvent(nsChangeHint aExtraHint);

Same here (and for the existing RebuildAllStyleData too, ideally).

r+sr=bzbarsky
Attachment #350595 - Flags: superreview?(bzbarsky)
Attachment #350595 - Flags: superreview+
Attachment #350595 - Flags: review?(bzbarsky)
Attachment #350595 - Flags: review+
Comment on attachment 350596 [details] [diff] [review]
patch 3: rebuild style data when user font set changes

sr=bzbarsky
Attachment #350596 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #17)
> (From update of attachment 350595 [details] [diff] [review])
> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> > nsCSSFrameConstructor::ProcessPendingRestyles()
> >   if (mRebuildAllStyleData) {
> >     // We probably wasted a lot of work up above, but this seems safest
> >     // and it should be rarely used.
> 
> Want to file a followup to skip the stuff above if mRebuildAllStyleData and
> just clear the pending restyle hashtable instead in that case?  I guess either
> way is fine.

We actually can't do that safely if there are hints in the hashtable.

That said, there's some coalescing we could do both between these two parts and among ancestor/descendant relationships in the hashtable.

I filed bug 467452.
Is this bug dependent on the changes for bug 467084?  Pulling from mozilla-central, patches 2+3 don't apply cleanly.  Patch 3 specifically looks like it was done on top of changes for bug 467084.
Yes, the patch is on top of the patches from bug 467084 (and others, but that may be the only one that it conflicts with).
Except the one that's attached is actually on top of an old version of the bug 467084 patch; the current version is at http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/tip/rebuild-style-data-for-user-font-set-change
OK, I fiddled with trying to apply patches but failed.

I tried this sequence of patches on top of moz-central tip:

* patch for bug 467084 (doesn't apply cleanly but simple to fix)
* patch 1 from this bug
* patch 'rebuild-style-data-for-user-font-set-change'

This sequence applies but doesn't compile, you need the change from 'post-rebuild-with-extra-hint'.  But that doesn't apply because it depends upon changes made for 'suppress-synth-mouse-move-loop', the addition of mHoverGeneration in nsCSSFrameConstructor.

Patch 3 looks fine but I wanted to play with it in the debugger to make sure I really understand what's going on.  

If it's easier for you just to point me at a repository with the patches applied, that's fine too.
(In reply to comment #23)
> If it's easier for you just to point me at a repository with the patches
> applied, that's fine too.

hg clone http://hg.mozilla.org/mozilla-central/ destdir
cd destdir/.hg/
hg clone http://hg.mozilla.org/users/dbaron_mozilla.com/patches/
cd ..
hg up -r5f86a34bc7981dd999ad7f9e2aac1471e11bde14
hg qpush run-mozilla-no-core                # multiple-backgrounds is broken now

will get you a tree with all my patches applied
I've landed the rest of the patches that this depends on, so here's an updated patch 3 that should apply directly to current mozilla-central.

Speaking of which, the first two patches are now landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/5da43688eee4
http://hg.mozilla.org/mozilla-central/rev/f8b47378d8ea
Attachment #351390 - Flags: review?(jdaggett)
Attachment #350596 - Attachment is obsolete: true
Attachment #350596 - Flags: review?(jdaggett)
Attachment #351390 - Flags: review?(jdaggett) → review+
Whiteboard: [fixed1.9.1]
Whiteboard: [fixed1.9.1]
Third patch landed on mozilla-central, thus fixed there:
http://hg.mozilla.org/mozilla-central/rev/727b7a33350c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Third patch landed on mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fa0f66c94ac3
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Flags: in-testsuite+
Verified fixed with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081212 Minefield/3.2a1pre

(now to lobby WebKit to fix their issues in this case - which are much worse
https://bugs.webkit.org/show_bug.cgi?id=21327)
Status: RESOLVED → VERIFIED
(In reply to comment #8)
> (In reply to comment #7)
> > I need to figure out why this has made
> > layout/reftests/font-face/cross-iframe-1.html not display correctly until just
> > *after* onload (thus making the reftest fail).
> 
> Er, sorry, it's a problem with cross-iframe-1-ref.html, and it's permanent.

Dbaron, I'm working on the greener-tinderbox stuff, and some of those boxes also failed to load cross-iframe-1. This comment is the only mention of that test failing in bugzilla.

What did you mean by "it's permanent?" and was this addressed or should I open another bug for that issue (if I can figure out how to reproduce it).
(In reply to comment #30)
> Dbaron, I'm working on the greener-tinderbox stuff, and some of those boxes
> also failed to load cross-iframe-1. This comment is the only mention of that
> test failing in bugzilla.

That failure was fixed before any patches were committed, as described in comment 10.

> What did you mean by "it's permanent?" and was this addressed or should I open
> another bug for that issue (if I can figure out how to reproduce it).

By "it's permanent" I meant that it wasn't a timing issue relating to when the display updates vs. when onload fires (as I initially thought), but rather that the display was not updating at all.
You need to log in before you can comment on or make changes to this bug.