Bug 21365

Summary: WebKit supports <font descent= but should support <font-face descent=!
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz, zimmermann
Priority: P2 Keywords: EasyFix
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
WebCore changes
darin: review+
LayoutTests changes darin: review+

Eric Seidel (no email)
Reported 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
Attachments
WebCore changes (2.12 KB, patch)
2008-10-04 02:52 PDT, Eric Seidel (no email)
darin: review+
LayoutTests changes (583.29 KB, patch)
2008-10-04 02:57 PDT, Eric Seidel (no email)
darin: review+
Eric Seidel (no email)
Comment 1 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
mitz
Comment 2 2008-10-04 01:06:17 PDT
Specifically, it looks like SVGFontFaceElement::descent() is looking at the wrong element.
Eric Seidel (no email)
Comment 3 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(-)
Eric Seidel (no email)
Comment 4 2008-10-04 02:57:25 PDT
Created attachment 24088 [details] LayoutTests changes
mitz
Comment 5 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?
Eric Seidel (no email)
Comment 6 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.
Eric Seidel (no email)
Comment 7 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
Darin Adler
Comment 8 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
mitz
Comment 9 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().
Eric Seidel (no email)
Comment 10 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. >
Eric Seidel (no email)
Comment 11 2008-10-06 01:30:22 PDT
Wow. That took a full 30 minutes to commit. Slooooow svn. r37328
Note You need to log in before you can comment on or make changes to this bug.