John Daggett (:jtd) Reporter
	Description • 10 years ago

When working on improving reflow behavior for downloadable fonts I ran into a number of problems that happen when the font used for a reftest switched during the reftest. In several cases, one or two pixel clumps of the initial font used would remain at the top and/or sides of the frame.

After discussing this with Cam, I set up a reftest that initially renders with one font and then switches to another before clearing the reftest-wait flag. If I run it with enough variations, I can reproduce the "text turd" problem on all platforms. Never more than a few pixels of difference but enough that I'm sure this is a cause of reftest flakiness on platforms like B2G.

These tests happen to use a downloadable font but I think it should be fairly easy to reproduce the same problem when switching between platform fonts.

	David Baron :dbaron:
	Comment 1 • 10 years ago

Seems like it could be related to insufficient overflow areas.

Does inflating the result of RoundOut(textMetrics.mBoundingBox) in nsTextFrame::ReflowText before adding it to aMetrics.VisualOverflow() and... er, wait a second, why does nsTextFrame::UpdateOverflow not do something equivalent to what ReflowText does???  That could be a problem.

	David Baron :dbaron:
	Comment 2 • 10 years ago

... to fix that, nsTextFrame::UpdateOverflow should probably use most of the code that nsTextFrame::RecomputeOverflow uses.  We conveniently already have the code!

(It looks like this has been buggy since bug 524925 landed.)

	David Baron :dbaron:
	Comment 3 • 10 years ago

... in any case, I filed bug 1140486 for that problem.  There's a decent chance it'll fix this, but I didn't want to clutter up this bug with it in case this is something else.

	David Baron :dbaron:
	Comment 4 • 10 years ago

In any case, to revise and extend comment 1: if bug 1140486 doesn't fix it, then it's worth seeing if (with its patches applied), changing the RoundOut() function in nsTextFrame.cpp (for all callers except ComputeTightBounds, probably) to inflate things further fixes it.  If it does, then that's a sign that the text metrics that we're using to set the visual overflow are insufficient.

	David Baron :dbaron:
	Comment 5 • 9 years ago

Did bug 1140486 fix this?

It seems worth getting these tests landed.

	John Daggett (:jtd) Reporter
	Comment 6 • 9 years ago

(In reply to David Baron [:dbaron] ⌚️UTC+11 (busy, returning 8 February) from comment #5)
> Did bug 1140486 fix this?
> 
> It seems worth getting these tests landed.

Nope.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d70a4ecc8951

You want me to annotate these and land them? Lots of orange there, in most cases the test/ref difference is limited to a small number of pixels.

	David Baron :dbaron:
	Comment 7 • 9 years ago

The pattern of failures seems pretty similar between Linux and Mac.  I think ideally one of us would just debug the failure and figure out how to fix it.  The experiment in comment 4 may still be useful.

	David Baron :dbaron:
	Comment 8 • 9 years ago

And the results of the experiment in comment 4 are that this diff:

 static nsRect
 RoundOut(const gfxRect& aRect)
 {
   nsRect r;
-  r.x = NSToCoordFloor(aRect.X());
-  r.y = NSToCoordFloor(aRect.Y());
-  r.width = NSToCoordCeil(aRect.XMost()) - r.x;
-  r.height = NSToCoordCeil(aRect.YMost()) - r.y;
+  r.x = NSToCoordFloor(aRect.X()) - 1;
+  r.y = NSToCoordFloor(aRect.Y()) - 1;
+  r.width = NSToCoordCeil(aRect.XMost()) - r.x + 1;
+  r.height = NSToCoordCeil(aRect.YMost()) - r.y + 1;
   return r;
 }

does in fact make the failures go away locally.

(It's actually somewhat impressive that that's the case, since I *think* the difference is just 1/60 of a CSS pixel on each side.)

	David Baron :dbaron:
	Comment 9 • 8 years ago

Jonathan, any chance I could interest you in debugging this?