Bug 16362 – Incorrect implementation of 'ex' unit

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Sam Stigler]

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.

[Sam Stigler]

Created attachment 17797 [details]
Bug in WebKit r28565

[Sam Stigler]

Created attachment 17798 [details]
Behaves correctly in Firefox 2.0.0.11

[Robert Blaut]

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."

[Robert Blaut]

*** Bug 16363 has been marked as a duplicate of this bug. ***

[Alex Coles]

Several IE CSS Test Cases, dependent on x-length, Fail in WebKit nightly r30853:

http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Height/height-035.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Max-height/max-height-035.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Max-width/max-width-035.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Min-height/min-height-035.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Min-width/min-width-035.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_10/Properties/Width/width-035.htm

[Anatoli Papirovski]

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.

(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?

[Anatoli Papirovski]

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.

[Anatoli Papirovski]

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.

[Darin Adler]

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.

[Anatoli Papirovski]

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?

[Anatoli Papirovski]

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.

[Robert Blaut]

Comment on attachment 21015 [details]
patch (updated)

I assume you ask for review one more time.

[Nikolas Zimmermann]

Dave, can you comment on this? it resides in the review queue for a long time now.

[Adele Peterson]

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.

[Evan]

what is going on with this?

[Maciej Stachowiak]

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.

[Anatoli Papirovski]

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.)

[Simon Fraser (smfr)]

Fixed by http://trac.webkit.org/changeset/80662 ?

[Gérard Talbot (no longer involved)]

> http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1601-c547-indent-00-b-a.htm

is now at

http://test.csswg.org/suites/css2.1/latest/html4/c547-indent-000.htm

and is invalid (because of rounding issues; needs code tuning) as explained in

http://lists.w3.org/Archives/Public/public-css-testsuite/2011Feb/0038.html




> http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1604-c541-word-sp-00-b-a.htm

is now at 

http://test.csswg.org/suites/css2.1/latest/html4/c541-word-sp-000.htm

and is invalid (because of rounding issues; needs code tuning) as explained in

http://lists.w3.org/Archives/Public/public-css-testsuite/2011Feb/0035.html



> 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.

regards, Gérard

[Gérard Talbot (no longer involved)]

> > 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

[Ahmad Saleem]

Created attachment 460505 [details]
Safari 15.5 matches other browsers

On the test case mentioned in Comment 21 (which is not invalid), Safari 15.5 on macOS 12.4 matches all other browsers as shown in the attached screenshot. Is there anything further required in this bug or this can be marked as "RESOLVED CONFIGURATION CHANGED"? Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/96011878>