Bug 80360

Summary: 1ex should equal .5em when the font has no x-height metric
Product: WebKit Reporter: Tab Atkins <tabatkins>
Component: CSSAssignee: Tab Atkins <tabatkins>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cmarcelo, darin, davidbarr, efidler, eric, hyatt, macpherson, menard, mitz, senorblanco, thorton, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Tab Atkins 2012-03-05 17:24:42 PST 
Testcase
========

<!doctype html>
<svg xmlns = 'http://www.w3.org/2000/svg' style='display:none'>
  <defs>
    <font id="Font1">
      <font-face font-family="block" />
    </font>
  </defs>
</svg>
<style>
.em { height: .5em; width: .5em; }
.ex { height: 1ex; width: 1ex; }
.cover { background: green; }
.back { background: red; }
.one { left: 10px; }
.two { left: 70px; }
#test > div { font-family: block; font-size: 100px; position: absolute; top: 10px; }
</style>
<div id='test'>
  <div class='back em one'></div>
  <div class='cover ex one'></div>

  <div class='back ex two'></div>
  <div class='cover em two'></div>
</div>


Expected result
===============
Because the SVG font doesn't define an x-height metric, the 'ex' unit for elements that use it should be sized to half the 'em' unit.

Actual result
=============
The 'ex' unit is sized to 0.  This also occurs with TrueType fonts that have been modified to not have an x-height metric.
Comment 1 Tab Atkins 2012-03-05 17:41:55 PST 
Created attachment 130249 [details]
Patch
Comment 2 Tab Atkins 2012-03-05 17:49:01 PST 
The above patch definitely works with cr-linux, but I'm unsure of what the other platforms return from their various font libraries.  I need to see bot results before I can be sure that my patch is complete.  :/
Comment 3 Tim Horton 2012-03-06 14:04:16 PST 
Comment on attachment 130249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130249&action=review

> Source/WebCore/platform/wx/wxcode/win/fontprops.cpp:61
> -        m_xHeight = m_ascent * 0.56f;  // Best guess for xHeight for non-Truetype fonts.
> -        m_lineGap = lroundf(tm.tmExternalLeading);
> +        m_xHeight = m_ascent * 0.56f; // Best guess for xHeight for non-Truetype fonts.0;lroundf(tm.tmExternalLeading);

Did you mean to remove the assignment to m_lineGap and put its associated computation at the end of a comment?
Comment 4 Tab Atkins 2012-03-06 14:54:28 PST 
...huh.  I *thought* that comment looked weird.  I have no idea how I managed to accidentally do that.

I'm about to be OOO for the week; I'll fix that and resubmit when I get back next week.  Thanks!
Comment 5 Hajime Morrita 2012-03-15 01:54:19 PDT 
Comment on attachment 130249 [details]
Patch

R-minusing based on Tim's comment to shorten the review queue. I'm looking for your next patch ;-)
Comment 6 David Barr 2012-03-25 19:48:43 PDT 
Created attachment 133724 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-04-19 15:26:11 PDT 
This looks reasonable to me.
Comment 8 Eric Seidel (no email) 2012-04-19 15:26:58 PDT 
Comment on attachment 133724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133724&action=review

> Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:109
> +        m_fontMetrics.setHasXHeight(false);

You might want to explain this line here with a comment.
Comment 9 Tab Atkins 2012-10-04 13:44:01 PDT 
Created attachment 167168 [details]
Patch for landing
Comment 10 Eric Seidel (no email) 2012-10-04 13:48:51 PDT 
Comment on attachment 167168 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=167168&action=review

> Source/WebCore/platform/graphics/FontMetrics.h:133
> +    bool m_hasXHeight;

Do we care about the size of this object such that we should be stealing a bit from another member instead of adding a whole int/byte/word/whatever
Comment 11 Tab Atkins 2012-10-04 13:51:27 PDT 
(In reply to comment #10)
> (From update of attachment 167168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167168&action=review
> 
> > Source/WebCore/platform/graphics/FontMetrics.h:133
> > +    bool m_hasXHeight;
> 
> Do we care about the size of this object such that we should be stealing a bit from another member instead of adding a whole int/byte/word/whatever

I have no idea!  We shouldn't have many FontMetrics objects in a page, though, so I doubt it's much of an issue (unless I'm wrong).
Comment 12 Eric Seidel (no email) 2012-10-04 13:56:36 PDT 
Comment on attachment 167168 [details]
Patch for landing

We'll have one for every instance of
    DataRef<StyleInheritedData> inherited;
each element which has a different value for an inherited property than it's parent will have a unique StyleInheritedData object.

I suspect this isn't a huge deal.  But I do suspect it will have a small memory effect.  The solution here is for someone to look at the overall memory usage of the style tree and make it smaller if needed.

I don't think that needs to hold up this patch however.
Comment 13 WebKit Review Bot 2012-10-04 17:35:47 PDT 
Comment on attachment 167168 [details]
Patch for landing

Clearing flags on attachment: 167168

Committed r130437: <http://trac.webkit.org/changeset/130437>
Comment 14 WebKit Review Bot 2012-10-04 17:35:51 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Adler 2012-10-05 17:08:59 PDT 
(In reply to comment #0)
> Because the SVG font doesn't define an x-height metric, the 'ex' unit for elements that use it should be sized to half the 'em' unit.

Where does that idea come from? A specification?
Comment 16 Tab Atkins 2012-10-05 18:33:23 PDT 
(In reply to comment #15)
> (In reply to comment #0)
> > Because the SVG font doesn't define an x-height metric, the 'ex' unit for elements that use it should be sized to half the 'em' unit.
> 
> Where does that idea come from? A specification?

Yes: http://dev.w3.org/csswg/css3-values/#ex-unit