WebKit supports <font descent= but should support <font-face descent=!

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebKit supports <font descent= but should support <font-face decent=!

Somehow we seem to have implemented support on the wrong element. :(

http://www.w3.org/TR/SVG11/fonts.html#FontFaceElement

[Eric Seidel (no email)]

This came from discussions with mitz this evening while trying to understand and fix:
https://bugs.webkit.org/show_bug.cgi?id=20420

Specifically, it looks like SVGFontFaceElement::descent() is looking at the wrong element.

[Eric Seidel (no email)]

Created attachment 24087 [details]
WebCore changes

 WebCore/ChangeLog                  |   13 +++++++++++++
 WebCore/svg/SVGFontFaceElement.cpp |    4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

[Eric Seidel (no email)]

Created attachment 24088 [details]
LayoutTests changes

Comment on attachment 24087 [details]
WebCore changes

Is it still correct to return early if m_fontElement is NULL?

[Eric Seidel (no email)]

(In reply to comment #5)
> (From update of attachment 24087 [details] [edit])
> Is it still correct to return early if m_fontElement is NULL?
> 

I don't know what the effect would be of moving the check below this logic.  I chose to leave it above so as not to change behavior.  I'm not sure how you'd ever get a SVGFontFaceElement to be asked for its ascent/descent w/o having a wrapping <font> element, but I don't know the SVG font code very well.

[Eric Seidel (no email)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 24087 [details] [edit] [edit])
> > Is it still correct to return early if m_fontElement is NULL?
> > 
> 
> I don't know what the effect would be of moving the check below this logic.  I
> chose to leave it above so as not to change behavior.  I'm not sure how you'd
> ever get a SVGFontFaceElement to be asked for its ascent/descent w/o having a
> wrapping <font> element, but I don't know the SVG font code very well.
> 

I'd rather leave the !NULL check at the top of each of these functions so that it's the same as all the rest.  Right now if the SVGFontFaceElement has not SVGFontElement associated with it, then all calls return 0.0f

[Darin Adler]

Comment on attachment 24087 [details]
WebCore changes

Looks fine, but I would have put this and the test changes into a single patch.

+    AtomicString value = getAttribute(ascentAttr);

To avoid a little reference count churn, this really should be const AtomicString& value.

r=me

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 24087 [details] [edit] [edit] [edit])
> > > Is it still correct to return early if m_fontElement is NULL?
> > > 
> > 
> > I don't know what the effect would be of moving the check below this logic.  I
> > chose to leave it above so as not to change behavior.  I'm not sure how you'd
> > ever get a SVGFontFaceElement to be asked for its ascent/descent w/o having a
> > wrapping <font> element, but I don't know the SVG font code very well.
> > 
> 
> I'd rather leave the !NULL check at the top of each of these functions so that
> it's the same as all the rest.  Right now if the SVGFontFaceElement has not
> SVGFontElement associated with it, then all calls return 0.0f
> 

This seems wrong and means that the descent attribute will have no effect on out-of-document SVG fonts. It's not like "all the rest". See unitsPerEm() and xHeight().

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 24087 [details] [edit])
> Looks fine, but I would have put this and the test changes into a single patch.
> 
> +    AtomicString value = getAttribute(ascentAttr);
> 
> To avoid a little reference count churn, this really should be const
> AtomicString& value.
> 
> r=me

Oh, they'll land as a single patch.  That's just teh magic of git. :)  I broke them out to make them easier to review.
> 

[Eric Seidel (no email)]

Wow.  That took a full 30 minutes to commit.  Slooooow svn.  r37328