Closed Bug 548648 Opened 13 years ago Closed 13 years ago

Reftest inline--position-relative--01-ref.xhtml fails on n810, n900, mac

Categories

(Core :: SVG, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

Attachments

(2 files, 4 obsolete files)

The reftest
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/sizing/inline--position-relative--01.xhtml
fails on mac (as noted in its reftest.list), as well as on fennec on maemo (both n900 & n810).

In each failure, there's a thin 1px-wide partly-translucent strip, just inside the solid blue border, on the left or bottom edge.

jrmuizel says this is likely because...
 - the SVG content is positioned relative to text
 - on mac & maemo, the default font has font glyphs that aren't exact pixel-widths.
 - we don't snap SVG content to pixels, while we do snap borders & images (and the reference case uses a static image)

fantasai suggested making the test use the 'ahem' font for its blocks of text -- in that font, each glyph is a 1em-by-1em square. (This would solve the issue as long as we set 'font-size' to an exact pixel amount).

However, if we were to do that, we might as well just get rid of the text altogether, since it's being used for exact-pixel-padding.

If the reftest is trying to test pixel-snapping, then the test probably should be removed, since that's not a property we guarantee for SVG content AFAIK.  However, if it's just testing relative positioning in general, then the 'ahem' font would probably be a good workaround for this subpixel issue.
Here's a screenshot that mfinkle sent me, from fennec on a maemo device.

It has a 1px-wide vertical sliver of translucency, 1px inwards from the left edge of the blue block.
Is there any other information we need to figure this out?
I don't think so... Re-reading comment 0, I think using 'ahem' is correct.  I think this test is just testing relative-positioning, not pixel-snapping.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached patch fix v1 (obsolete) — Splinter Review
Here's a patch to fix the test.  mfinkle or jmaher, mind testing this in a reftest run on maemo?

(Note: this needs testing as part of an actual reftest run, rather than just a viewing of the static test, since it references a parent directory to get to the "ahem" font.  That doesn't work when viewing the static file, and it only works in a reftest run due to the "HTTP(../..)" that I added in reftest.list)
Attachment #435677 - Flags: review?(jwatt)
I will give it a run on maemo...thanks for the patch.
this passes on my n900 maemo device.
but I should note that there are 2 other similar bugs which are continuing to fail:
tests/layout/reftests/svg/sizing/inline--display-inline-block--01.xhtml 
tests/layout/reftests/svg/sizing/inline--display-inline--01.xhtml
Attached patch fix v2 (obsolete) — Splinter Review
Ok, this version separates out the new style into an "ahem.css" file, which we'll now use in all the reftests in that directory that were marked as failing on Mac. (which are the reftests with subpixel-font-size issues, I presume)

So: this patch fixes the original reftest here and the two mentioned in comment 7, as well as the test "inline--display-inline-block--01.xhtml", so that they'll all pass on Mac & maemo. I'm running it through tryserver to test it on Mac.
Attachment #435677 - Attachment is obsolete: true
Attachment #435917 - Flags: review?
Attachment #435677 - Flags: review?(jwatt)
Attachment #435917 - Flags: review? → review?(jwatt)
Attached patch fix v2a (obsolete) — Splinter Review
(Last patch had two unintended whitespace-only changes; nixed those in this version.)
Attachment #435917 - Attachment is obsolete: true
Attachment #435926 - Flags: review?(jwatt)
Attachment #435917 - Flags: review?(jwatt)
Attached patch fix v2b (obsolete) — Splinter Review
Previous patch worked on mac, but failed on Windows, apparently due to a fractional-pixel-value for line-height.
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1269965653.1269975481.3175.gz

This new patch version specifies font-size and line-height as exact pixel values to fix that problem.  Trying it on Try Server now.
Attachment #435926 - Attachment is obsolete: true
Attachment #435990 - Flags: review?(jwatt)
Attachment #435926 - Flags: review?(jwatt)
Attached patch fix v2cSplinter Review
...and of course that should be a line-height of 12px (or anything integral > 10px), not 2px... fixed here.
Attachment #435990 - Attachment is obsolete: true
Attachment #435990 - Flags: review?(jwatt)
Attachment #436133 - Flags: review?(jwatt)
Comment on attachment 436133 [details] [diff] [review]
fix v2c

r=jwatt. Appologies for missing this review request earlier.
Attachment #436133 - Flags: review?(jwatt) → review+
No worries -- thanks for the review! :) Pushed:
http://hg.mozilla.org/mozilla-central/rev/fd883c55f992
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.