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
: WebKit
SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
: EasyFix
:
:
  Show dependency treegraph
 
Reported: 2008-10-04 01:03 PST by
Modified: 2008-10-06 01:30 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-04 01:03:02 PST
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 From 2008-10-04 01:05:15 PST -------
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 From 2008-10-04 01:06:17 PST -------
Specifically, it looks like SVGFontFaceElement::descent() is looking at the wrong element.
------- Comment #3 From 2008-10-04 02:52:26 PST -------
Created an attachment (id=24087) [details]
WebCore changes

 WebCore/ChangeLog                  |   13 +++++++++++++
 WebCore/svg/SVGFontFaceElement.cpp |    4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)
------- Comment #4 From 2008-10-04 02:57:25 PST -------
Created an attachment (id=24088) [details]
LayoutTests changes
------- Comment #5 From 2008-10-04 02:58:16 PST -------
(From update of attachment 24087 [details])
Is it still correct to return early if m_fontElement is NULL?
------- Comment #6 From 2008-10-04 03:06:02 PST -------
(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 From 2008-10-04 03:10:58 PST -------
(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 From 2008-10-04 08:22:05 PST -------
(From update of attachment 24087 [details])
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 From 2008-10-04 10:45:49 PST -------
(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 From 2008-10-05 00:58:43 PST -------
(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 From 2008-10-06 01:30:22 PST -------
Wow.  That took a full 30 minutes to commit.  Slooooow svn.  r37328