[d2d] TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_bbox.xhtml | a.getBBox().height - got 19, expected 19.453125

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jrmuizel, Assigned: bas.schouten)

Tracking

unspecified
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

Attachments

(1 attachment)

Reporter

Description

9 years ago
No description provided.
Reporter

Comment 1

9 years ago
Looks like something might be up with our text measuring
Reporter

Updated

9 years ago
OS: Mac OS X → Windows 7
Reporter

Comment 2

9 years ago
This is happening because getBBox() is returning a different height for the bounding box of <text>y</text> than <text>a</text>. I'm not sure what the correct behavior here should be. Every browser I've tested returns matching heights except for IE9.
OS: Windows 7 → Windows XP
From the SVG specification, section 7.11
(http://www.w3.org/TR/SVG/coords.html#ObjectBoundingBox), the full
glyph cell should be accounted for:

"For 'text' elements, for the purposes of the bounding box calculation, each
glyph is treated as a separate graphics element. The calculations assume that
all glyphs occupy the full glyph cell. For example, for horizontal text, the
calculations assume that each glyph extends vertically to the full ascent and
descent values for the font."
Reporter

Comment 4

9 years ago
Here are the metrics differences from MeasureText in gdi and d2d
d2d: 
+	pos	{x=-58.000000000000000 y=-960.00000000000000 }	gfxPoint
+	size	{width=628.00000000000000 height=1245.0000000000000 }	gfxSize

gdi

+	pos	{x=-64.000000000000000 y=-960.00000000000000 }	gfxPoint
+	size	{width=640.00000000000000 height=1216.0000000000000 }	gfxSize
Reporter

Comment 5

9 years ago
The problem here is that we are getting metrics that extend beyond the descent. I think this may also explain our intermittent failures as it seems we can get different extents based on NeedsGlyphExtents().
Reporter

Comment 6

9 years ago
(In reply to comment #5)
> I think this may also explain our intermittent failures as it seems we can get
> different extents based on NeedsGlyphExtents().

I think this second part is wrong. I think NeedsGlyphExtents() is true in both the GDI and DWrite cases. Perhaps it has something to do with detail glyphs?
Assignee

Comment 7

9 years ago
This is caused by the fix to bug 568191. We need to think about how we're going to fix both.
Assignee

Comment 8

9 years ago
This mimics the behavior on GDI by only padding the horizontal metrics. It did not cause any new test failures on test runs on birch, and it fixes this mochitest failure.
Assignee: nobody → bas.schouten
Status: NEW → ASSIGNED
Attachment #466094 - Flags: review?(jmuizelaar)
Reporter

Comment 9

9 years ago
Comment on attachment 466094 [details] [diff] [review]
Only pad horizontal glyph metrics

Jonathan is probably a better reviewer for this.
Attachment #466094 - Flags: review?(jmuizelaar) → review?(jfkthame)
Reporter

Comment 10

9 years ago
Also, perhaps we should just switch to using a GlyphRunAnalysis as it should actually give us the information we want.
Comment on attachment 466094 [details] [diff] [review]
Only pad horizontal glyph metrics

I think this is probably okay but I'm still a little queasy, it seems odd that you would tweak the width but not the height.  Plus'ing for now.

Jonathan, can you double-check this and reopen if needed?
Attachment #466094 - Flags: review?(jfkthame) → review+
blocking2.0: --- → beta4+
Assignee

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/16aa8f0aa149
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
I haven't done an updated build yet, but I suspect this will regress the rendering of attachment 447506 [details] (from bug 568191), which illustrated the need to pad cairo's glyph extents vertically as well as horizontally under directwrite. In particular, see lines 2 and 5 of that example.

AFAIK, we don't currently have a reftest that will detect this (though we could try to construct one), so from that point of view we can get away with the change for now, but it's not really satisfactory - the glyph clipping illustrated in attachment 447507 [details] (screenshot of attachment 447506 [details] without padding extents, before bug 568191 landed) is quite ugly.
Assignee

Comment 14

9 years ago
(In reply to comment #13)
> I haven't done an updated build yet, but I suspect this will regress the
> rendering of attachment 447506 [details] (from bug 568191), which illustrated the need to
> pad cairo's glyph extents vertically as well as horizontally under directwrite.
> In particular, see lines 2 and 5 of that example.
> 
> AFAIK, we don't currently have a reftest that will detect this (though we could
> try to construct one), so from that point of view we can get away with the
> change for now, but it's not really satisfactory - the glyph clipping
> illustrated in attachment 447507 [details] (screenshot of attachment 447506 [details] without
> padding extents, before bug 568191 landed) is quite ugly.

Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite + GDI perhaps?
(In reply to comment #14)
> (In reply to comment #13)
> > I haven't done an updated build yet, but I suspect this will regress the
> > rendering of attachment 447506 [details] [details] (from bug 568191), which illustrated the need to
> > pad cairo's glyph extents vertically as well as horizontally under directwrite.
> > In particular, see lines 2 and 5 of that example.
> > 
> > AFAIK, we don't currently have a reftest that will detect this (though we could
> > try to construct one), so from that point of view we can get away with the
> > change for now, but it's not really satisfactory - the glyph clipping
> > illustrated in attachment 447507 [details] [details] (screenshot of attachment 447506 [details] [details] without
> > padding extents, before bug 568191 landed) is quite ugly.
> 
> Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite
> + GDI perhaps?

Yes, it was. So maybe going to D2D will mitigate that issue.

Out of curiosity, have you checked whether this regresses any tests when running with DW+GDI? Or are we going to treat that case as (relatively) unimportant? I realize tinderbox isn't currently testing it, at least.
Assignee

Comment 16

9 years ago
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I haven't done an updated build yet, but I suspect this will regress the
> > > rendering of attachment 447506 [details] [details] [details] (from bug 568191), which illustrated the need to
> > > pad cairo's glyph extents vertically as well as horizontally under directwrite.
> > > In particular, see lines 2 and 5 of that example.
> > > 
> > > AFAIK, we don't currently have a reftest that will detect this (though we could
> > > try to construct one), so from that point of view we can get away with the
> > > change for now, but it's not really satisfactory - the glyph clipping
> > > illustrated in attachment 447507 [details] [details] [details] (screenshot of attachment 447506 [details] [details] [details] without
> > > padding extents, before bug 568191 landed) is quite ugly.
> > 
> > Fwiw, this test case looks fine with D2D. Was the screenshot taking with DWrite
> > + GDI perhaps?
> 
> Yes, it was. So maybe going to D2D will mitigate that issue.
> 
> Out of curiosity, have you checked whether this regresses any tests when
> running with DW+GDI? Or are we going to treat that case as (relatively)
> unimportant? I realize tinderbox isn't currently testing it, at least.

I haven't. I don't think it's unimportant at all, but I do think it can wait :). So as you say, relatively, I'd say it's unimportant.
You need to log in before you can comment on or make changes to this bug.