philippe (part-time) Reporter
	Description • 16 years ago

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

	philippe (part-time) Reporter
	Comment 1 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 2 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 4 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 5 • 16 years ago

... and the actual fix

	David Baron :dbaron: Assignee
	Comment 6 • 16 years ago

Comment on attachment 350583 [details] [diff] [review]
patch 3: rebuild style data when user font set changes

Actually, hold on a bit...

	David Baron :dbaron: Assignee
	Comment 7 • 16 years ago

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...

	David Baron :dbaron: Assignee
	Comment 8 • 16 years ago

(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.

	Bill Gianopoulos [:WG9s]
	Comment 9 • 16 years ago

(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.

	David Baron :dbaron: Assignee
	Comment 10 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 11 • 16 years ago

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.)

	David Baron :dbaron: Assignee
	Comment 12 • 16 years ago

With tests.

There are still some problems with patch 2, though, although it passes all the @font-face tests.

	David Baron :dbaron: Assignee
	Comment 13 • 16 years ago

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...

	Bill Gianopoulos [:WG9s]
	Comment 15 • 16 years ago

(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 :-) )

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 16 years ago

Comment on attachment 350581 [details] [diff] [review]
patch 1: remove some stub units code

r+sr=bzbarsky

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 16 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 16 years ago

Comment on attachment 350596 [details] [diff] [review]
patch 3: rebuild style data when user font set changes

sr=bzbarsky

	David Baron :dbaron: Assignee
	Comment 19 • 16 years ago

(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.

	John Daggett (:jtd)
	Comment 20 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 21 • 16 years ago

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).

	David Baron :dbaron: Assignee
	Comment 22 • 16 years ago

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

	John Daggett (:jtd)
	Comment 23 • 16 years ago

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.

	David Baron :dbaron: Assignee
	Comment 24 • 16 years ago

(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

	David Baron :dbaron: Assignee
	Comment 25 • 16 years ago

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

	David Baron :dbaron: Assignee
	Comment 26 • 16 years ago

Landed first two patches on 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3f702644de36
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d0875f1a7c53

	David Baron :dbaron: Assignee
	Comment 27 • 16 years ago

Third patch landed on mozilla-central, thus fixed there:
http://hg.mozilla.org/mozilla-central/rev/727b7a33350c

	David Baron :dbaron: Assignee
	Comment 28 • 16 years ago

Third patch landed on mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fa0f66c94ac3

	philippe (part-time) Reporter
	Comment 29 • 16 years ago

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)

	cmtalbert
	Comment 30 • 15 years ago

(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).

	David Baron :dbaron: Assignee
	Comment 31 • 15 years ago

(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.