Closed Bug 1139893 Opened 9 years ago Closed 9 years ago

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1062108-1.html | image comparison (==), max difference: 255, number of differing pixels: 32


(Firefox OS Graveyard :: Runtime, defect)

Not set


(firefox39 fixed)

2.2 S8 (20mar)
Tracking Status
firefox39 --- fixed


(Reporter: gerard-majax, Assigned: gerard-majax)



(Whiteboard: [systemsfe])


(4 files, 3 obsolete files)

So we have this test failing on Mulet, and it's not something skipped on B2G/B2G Desktop
Attached image reference screenshot
I do have the exact same behavior locally. Hacking fonts is good visually.
Comment on attachment 8573955 [details] [diff] [review]
Fixing fonts for reftest bugs/1062108-1.html r=...

Augmenting the font family fixes the issue. Asking review to assert if you think it's the proper way to fix this :)
Attachment #8573955 - Flags: review?(jdaggett)
On which platform was this failing? Mulet on Linux? I'm just curious which font it was picking up such that width(nbsp) != width(x). I think Jonathan picked Droid Mono because there was no bold, so you get fake bolding. I think switching to DejaVu Sans Mono will pick up a real bold face so that's changing the nature of the test.
Yes, Mulet on Linux.
Does the font on the screenshot looks like the good one? This is what I had locally, too. Ubuntu 14.04 laptop when testing this.

The other fonts in the list, monano and monospace, were giving worse results. Would it mean there is a real bug ? If so, where should I start looking ?
Flags: needinfo?(jdaggett)
Right, I've investigated a little. So far, locally, it looks like I have the expected font (according to :janx). I've noticed that changing the zoom level (150% and 300%) gets me the expected match between both.
Jonathan, any ideas on why this would be failing on Mulet on Linux? Maybe picking up monospace fonts that don't support nbsp?
Flags: needinfo?(jdaggett) → needinfo?(jfkthame)
(In reply to John Daggett (:jtd) from comment #11)
> Jonathan, any ideas on why this would be failing on Mulet on Linux? Maybe
> picking up monospace fonts that don't support nbsp?

That shouldn't be a problem, afaik ... I believe we "fake"   support by just using the font's space character anyway. See

I wonder if it might be an issue with device-pixel rounding of the glyph advance working differently for the space (for which we use the space-glyph width that's cached in the font) and for the sequence of "x"s (which will go through the usual shaping and measurement codepath). But I haven't attempted to dig deeper to see if that's really relevant or not.
Flags: needinfo?(jfkthame)
Jonathan, how could I test your hypothesis ?
Flags: needinfo?(jfkthame)
Or maybe bug 1062108 is actually still present on Linux desktop, and just didn't show up because the monospace fonts used there have true bold faces; but running Mulet, we get the Droid Mono face, so synthetic bold kicks in, and the bug surfaces.
(In reply to Alexandre LISSY :gerard-majax from comment #13)
> Jonathan, how could I test your hypothesis ?

If comment 12 is right, it should be possible to demonstrate that the problem goes away at font sizes where the glyph advance is an exact number of pixels; testing a "waterfall" of incrementally-varying sizes should show the bug appearing and disappearing according to how the rounding is working out.

At that point, I think I'd want to run it under a debugger and confirm exactly what glyph metrics are being used for the <space> and <x> glyphs involved, to see what discrepancy there is.
Flags: needinfo?(jfkthame)
Like we documented in comment 10 ?
Flags: needinfo?(jfkthame)
(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Like we documented in comment 10 ?

That's certainly suggestive of a pixel-rounding-related issue, though a waterfall test covering a series of sizes such as 10px, 10.5px, 11px, .... 16px or something like that might be clearer and more precise.

Incidentally, a simple test such as

  data:text/html,<pre style="font:12px droid sans mono">hello world<br><b>hello world

on Firefox/Linux (on a system with Droid Sans Mono installed) shows that we are handling synthetic-bold poorly on desktop Linux; the bold line is consistently wider than the regular one. The same test on OS X (again, with Droid Sans Mono installed) has both lines the same width. This doesn't directly account for the test failure here, afaics, but it does suggest that behavior on desktop Linux systems is significantly different from other platforms.
Flags: needinfo?(jfkthame)
I've also noticed taskcluster reftests failures on fonts-related tests, that we do not have on try.
I just checked on my firefox nightly coming from mozilla directly (i.e., not self-built) and I'm having the exact same discrepancy than the one I see locally on Mulet.
That's from 8px to 18.5px for font-size, with 0.5px increment. So 12px and 12.5px would look good.
It's pretty clear from the images there -- noting that the discrepancy is sometimes in one direction, sometimes the other -- that the issue does relate to pixel-rounding of fractional glyph advances at some level.

I'm curious what you get if you try an example like

  data:text/html,<div style="font:18.5px droid sans mono"><b>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</b>

and see what document.querySelector("b").getBoundingClientRect() returns; then compare

  data:text/html,<div style="font:18.5px droid sans mono"><b>xxxxx</b>

and see whether one of the results seems to be pixel-rounded, but not the other? In theory, they'd be expected to match.
They do not match :)
You're missing a space in the font name in one of those, so it hasn't used the correct font.
Flags: needinfo?(jfkthame)
Good catch. The rounding error is more obvious this way.

With the nbsp;:
> DOMRect { x: 8, y: 8, width: 58.83332824707031, height: 23, top: 8, right: 66.83332824707031, bottom: 31, left: 8 }

With the x:
> DOMRect { x: 8, y: 8, width: 60, height: 23, top: 8, right: 68, bottom: 31, left: 8 }
Attachment #8575258 - Attachment is obsolete: true
Attachment #8575259 - Attachment is obsolete: true
OK, so it looks like in this example, we're using an unrounded advance width for the space, but a pixel-snapped advance for normal glyphs. Hence the mismatch.

I wonder if we need to check GetRoundOffsetsToPixels somewhere like gfxTextRun::SetSpaceGlyphIfSimple and potentially apply pixel rounding there. Note that we can't simply round the space width when caching it in the font, as rounding is not applied in all situations (e.g. with transformations in effect).

John, care to pursue this? I'm not sure how soon I'll find time to dig further...
Flags: needinfo?(jdaggett)
Should I proceed skipping this test for now and file a follow up for you or john to fix it properly ?
Flags: needinfo?(jfkthame)
Yeah, I think that's fine. Looks like it's showing a real bug, so we shouldn't just fudge the test to hide the problem; better to explicitly skip it on the problem platform(s), and file a bug to fix the underlying issue. Include the new bug number in the reftest manifest when adding the skip annotation, to help us keep track.
Flags: needinfo?(jfkthame)
Blocks: 1141535
There is a real issue underlying, tracked in bug 1141535
Attachment #8573955 - Attachment is obsolete: true
Attachment #8573955 - Flags: review?(jdaggett)
Attachment #8575284 - Flags: review?(jfkthame)
Attachment #8575284 - Flags: review?(jfkthame) → review+
Keywords: checkin-needed
I'll take a look when I get a chance.
Flags: needinfo?(jdaggett)
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe]
Target Milestone: --- → 2.2 S8 (20mar)
You need to log in before you can comment on or make changes to this bug.