All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jrmuizel, Assigned: bas.schouten)

Tracking

unspecified
x86
Windows XP
Points:
---

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Comment 1

8 years ago
Looks like something might be up with our text measuring
(Reporter)

Updated

8 years ago
OS: Mac OS X → Windows 7
(Reporter)

Comment 2

8 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

8 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

8 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

8 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

8 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

8 years ago
Created attachment 466094 [details] [diff] [review]
Only pad horizontal glyph metrics

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

8 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

8 years ago
Also, perhaps we should just switch to using a GlyphRunAnalysis as it should actually give us the information we want.

Comment 11

8 years ago
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

8 years ago
http://hg.mozilla.org/mozilla-central/rev/16aa8f0aa149
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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

8 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

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