Bug 16362 - Incorrect implementation of 'ex' unit
Summary: Incorrect implementation of 'ex' unit
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Nobody
URL: http://www.w3.org/TR/CSS21/syndata.ht...
Keywords:
: 16363 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-12-09 00:41 PST by Sam Stigler
Modified: 2011-07-18 16:24 PDT (History)
8 users (show)

See Also:


Attachments
Bug in WebKit r28565 (59.37 KB, image/png)
2007-12-09 00:45 PST, Sam Stigler
no flags Details
Behaves correctly in Firefox 2.0.0.11 (101.63 KB, image/png)
2007-12-09 00:47 PST, Sam Stigler
no flags Details
patch 2 (2.14 KB, patch)
2008-05-07 22:50 PDT, Anatoli Papirovski
no flags Details | Formatted Diff | Diff
patch (updated) (3.16 KB, patch)
2008-05-08 07:13 PDT, Anatoli Papirovski
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Stigler 2007-12-09 00:41:55 PST
Overview description:
   The 8 December 2007 nightly build of WebKit fails div "seven" of the first text-indent test in the CSS 2.1 Test Suite.

Step to Reproduce:
1.  Visit the above URL.

Actual Results:
  All of the divs line up as expected; except for div "seven" (the third from the bottom on the rendered page), which is indented a little bit more than the others.  See attached screenshot.

Expected Results:
   All of the divs (that is, blue boxes), would be in a "tall solid unbroken column" (from the URL above).

Build Date & Platform:
   8 December 2007, for Mac OS X

Additional Builds and Platforms:
  Also occurs in Safari 3.0.4 (5523.10) for Mac OS X.

  Doesn't occur in Firefox 2.0.0.11 for Mac OS X.

Additional Information:
  Yes, I have the 'Ahem' font installed.
Comment 1 Sam Stigler 2007-12-09 00:45:30 PST
Created attachment 17797 [details]
Bug in WebKit r28565
Comment 2 Sam Stigler 2007-12-09 00:47:25 PST
Created attachment 17798 [details]
Behaves correctly in Firefox 2.0.0.11
Comment 3 Robert Blaut 2008-02-09 13:54:31 PST
There are a couple of CSS 2.1 tests that fails in Webkit because of incorrect treating 1ex as 1em.

http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1601-c547-indent-00-b-a.htm
http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1604-c541-word-sp-00-b-a.htm
http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t040302-c61-rel-len-00-b-ag.htm


Ahem font used in these tests have x-height set to 0.8em according to http://hixie.ch/resources/fonts/:

"The font's alphabetic baseline is 0.2em above its bottom, and 0.8em below its top. The font has an x-height of 0.8em."
Comment 4 Robert Blaut 2008-02-09 13:55:12 PST
*** Bug 16363 has been marked as a duplicate of this bug. ***
Comment 6 Anatoli Papirovski 2008-05-07 22:50:00 PDT
Created attachment 21006 [details]
patch 2

Ok, as discussed I've corrected the xheight calculation not to make ex = em which was done to fix a gmail bug that no longer exists. I've also removed the "ex" from margin declaration on input radio and checkbox.

I didn't include any test cases, because a lot (and I mean A LOT) of test cases need to be regenerated, pretty much anything that uses "ex", "x-height", or radio or checkbox form elements.

Also, those Microsoft test cases are actually wrong... per the CSS spec, if the font does not have an xHeight specified, the rendering engine should try and compute it from an actual x, thus the xHeight for Ahem is 0.8 rather than 0.5.
Comment 7 mitz 2008-05-07 22:57:08 PDT
(In reply to comment #6)
Do you need to patch SimpleFontDataCGWin.cpp in the same way?

> I didn't include any test cases, because a lot (and I mean A LOT) of test cases
> need to be regenerated, pretty much anything that uses "ex", "x-height", or
> radio or checkbox form elements.

Can you use pixel values that match the current 0.5ex value for the default font so that most tests and web pages do not change? What are the new, asymmetric values based on? Should the values be mirrored for RTL?
Comment 8 Anatoli Papirovski 2008-05-07 23:11:31 PDT
Comment on attachment 21006 [details]
patch 2

I took the values from Gecko stylesheet, but now that you mention it... I think they don't make much sense, so I'll just use a value that will match 0.5ex.

I'll check if I need to edit the win file or not.

I cleared the review flag and I'll post an updated patch in the morning.
Comment 9 Anatoli Papirovski 2008-05-08 07:13:25 PDT
Created attachment 21015 [details]
patch (updated)

Ok, I updated the patch: I've fixed the same issue within the windows file and I've modified the margin to be 3px for all sides, which matches the 0.5ex.

I still did not include any test cases in the patch since there's still about 20 that need to be regenerated.
Comment 10 Darin Adler 2008-05-24 23:14:01 PDT
Comment on attachment 21015 [details]
patch (updated)

Thanks for tackling this.

I'd like Hyatt to weigh in with his opinion about this.

As with any other bug fix, we need a regression test case in the patch, and the patch also needs to include updates to the results of any existing test cases affected by the change.

Setting review- because of the lack of a test case for now. Please make a new patch that includes a test case as well as any changes to the expected results of existing test cases.
Comment 11 Anatoli Papirovski 2008-05-25 07:27:50 PDT
The reason I did not include anything was because it made the file too big and Mitz told me that whoever would commit could just regenerate the results on their own. Sure I'd be happy to include the test cases, but do you really want me to upload a patch that's several MB big?
Comment 12 Anatoli Papirovski 2009-01-01 22:28:50 PST
I would like somebody to go over this bug sometime soon. As mentioned in my previous post, based on my discussion with Mitz on IRC, we decided not to attach LayoutTests results, because they affected too many files (3.8mb worth of updates). Whoever commits this, could create the new test results themselves without having to upload the file to bugzilla.

Other than that this is a perfectly valid patch, that could be integrated into WebKit and finally fix the "ex" implementation.
Comment 13 Robert Blaut 2009-01-05 23:41:29 PST
Comment on attachment 21015 [details]
patch (updated)

I assume you ask for review one more time.
Comment 14 Nikolas Zimmermann 2009-02-09 05:04:27 PST
Dave, can you comment on this? it resides in the review queue for a long time now.
Comment 15 Adele Peterson 2009-03-23 11:22:48 PDT
Comment on attachment 21015 [details]
patch (updated)

Even if you don't want to include the test results, you still need to include the actual test case in the patch.

Regardless, since Darin wants Hyatt to take a look, I'm adding him as the requested reviewer here.
Comment 16 Evan 2009-05-12 13:43:54 PDT
what is going on with this?
Comment 17 Maciej Stachowiak 2009-05-21 21:54:01 PDT
Comment on attachment 21015 [details]
patch (updated)

1) The patch removes this comment and the code explained by it:

-        // Use the maximum of either width or height because "x" is nearly square
-        // and web pages that foolishly use this metric for width will be laid out
-        // poorly if we return an accurate height. Classic case is Times 13 point,
-        // which has an "x" that is 7x6 pixels.

Do you believe this is no longer necessary? If so, why not? Did you test what other browsers do with Times 13 point? I think such a specific comment needs to be addressed and rebutted if it no longer applies.

2) The change to the default style of radio buttons and checkboxes seems unrelated. Is there any reason it needs to be included with this patch, rather than making the change separately? Also, what is the reason for the html4.cc change? It doesn't seem to be explained in the patch or in the bug.

r- for now but I will personally review this patch given satisfactory answers to these questions, rather than waiting for Hyatt.
Comment 18 Anatoli Papirovski 2009-05-21 23:27:26 PDT
1) This was incorrect in the first place. It shouldn't be WebKit's business to try and fix people's faulty code. None of the other browser do it: Firefox, Opera, even IE. All the browsers at this point calculate x-height, and therefore 'ex', correctly. WebKit is the only one that doesn't.

2) This was discuss with hyatt and mitz on IRC. In fact, this whole thing was discussed with him (a year ago now). That's why I've been pushing so hard for him specifically to review this. It needs to be included as part of this patch, because the changes to 'ex' would change the margins on checkboxes/radios.

I'm sorry, but this is getting a little silly, if you ask me. This was perfectly fine back a year ago, with some minor problems that could have been easily rectified (missing test-case results, which was discussed on IRC and agreed that whoever commits would create)... it really seems that WebKit has no interest in getting its last few CSS1 and CSS2 bugs fixed. (This isn't the only CSS1/CSS2 bug in this position.)
Comment 19 Simon Fraser (smfr) 2011-03-09 17:52:42 PST
Fixed by http://trac.webkit.org/changeset/80662 ?
Comment 21 Gérard Talbot 2011-07-18 16:24:35 PDT
> > http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t040302-c61-rel-len-00-b-ag.htm
> 
> This c61-rel-len-000 testcase has been removed.

Argh... sorry about that. That testcase is correct and was not removed.

Gérard