Bug 21365 - WebKit supports <font descent= but should support <font-face descent=!
: WebKit supports <font descent= but should support <font-face descent=!
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
: EasyFix
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-04 01:03 PDT by Eric Seidel
Modified: 2008-10-06 01:30 PDT (History)
2 users (show)

See Also:


Attachments
WebCore changes (2.12 KB, patch)
2008-10-04 02:52 PDT, Eric Seidel
darin: review+
Details | Formatted Diff | Diff
LayoutTests changes (583.29 KB, patch)
2008-10-04 02:57 PDT, Eric Seidel
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2008-10-04 01:03:02 PDT
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
Comment 1 Eric Seidel 2008-10-04 01:05:15 PDT
This came from discussions with mitz this evening while trying to understand and fix:
https://bugs.webkit.org/show_bug.cgi?id=20420
Comment 2 mitz@webkit.org 2008-10-04 01:06:17 PDT
Specifically, it looks like SVGFontFaceElement::descent() is looking at the wrong element.
Comment 3 Eric Seidel 2008-10-04 02:52:26 PDT
Created attachment 24087 [details]
WebCore changes

 WebCore/ChangeLog                  |   13 +++++++++++++
 WebCore/svg/SVGFontFaceElement.cpp |    4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)
Comment 4 Eric Seidel 2008-10-04 02:57:25 PDT
Created attachment 24088 [details]
LayoutTests changes
Comment 5 mitz@webkit.org 2008-10-04 02:58:16 PDT
Comment on attachment 24087 [details]
WebCore changes

Is it still correct to return early if m_fontElement is NULL?
Comment 6 Eric Seidel 2008-10-04 03:06:02 PDT
(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.
Comment 7 Eric Seidel 2008-10-04 03:10:58 PDT
(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
Comment 8 Darin Adler 2008-10-04 08:22:05 PDT
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
Comment 9 mitz@webkit.org 2008-10-04 10:45:49 PDT
(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().
Comment 10 Eric Seidel 2008-10-05 00:58:43 PDT
(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.
> 
Comment 11 Eric Seidel 2008-10-06 01:30:22 PDT
Wow.  That took a full 30 minutes to commit.  Slooooow svn.  r37328