Bug 41535

Created attachment 60519 [details] Example of effect of line-height on vertical alignment The line-height property is currently set to 1. Removing this will change the behavior of middle. This example shows how the vertical alignment is affected by line height settings. It is possible that we don't want to mess with line-height at all. This may be an artifact of earlier alignment issues that are now solved with the use of baseline alignments.

I agree that in the MathML implementation we don't want to use vertical-align:middle internally to avoid effects that you are talking about. But, for instance, we need to use xHeight() to align the fraction bar. The test case, was intended to show a case affected by the wrong behaviour of xHeight(), even if vertical-align:middle indeed uses xHeight() AND lineHeight() to compute the vertical shift.

Created attachment 66398 [details] Correct test My previous test was erroneous. This one shows that vertical-align: middle; fails on a MathML element when rendered with Apple Symbol font. The behaviour is correct with STIX fonts. It is due to the fact that internally vertical-align: middle; calls xHeight() that returns 0 with Apple Symbol font as this font doesn't contain an x glyph (see https://bugs.webkit.org/show_bug.cgi?id=38377#c2).

Created attachment 66642 [details] Patch Patch making a guess of xHeight() when the font doesn't contain any "x" glyph.

Comment on attachment 66642 [details] Patch > + if (static_cast<int>(m_xHeight) == 0 && m_ascent > 0) Why the cast to int? WebKit style is to not write “== 0” and “!= 0”. > + m_xHeight = 0.66 * m_ascent; Why 0.66? If you used 3 / 2 (or 66 / 100, I don’t know if the difference is meaningful) you could avoid float arithmetic here. Should this really be based the on the already-rounded m_ascent, and always rounded down, or would it be better to use fAscent and round to the nearest integer? Do other ports besides Mac need a similar changed?

I'll correct the style. The cast is here, because xHeight is a float and Core Graphics seems to return a value just above zero but not zero for Apple Symbols fonts that have no "x" glyph. Thus the test if (!m_xHeight) is always false. I thought that m_ascent was a float. It is indeed an int. I don't know which possibility is the best: - 0.66 * fAscent; - static_cast<float>(2 * m_ascent / 3); The first one is probably better from a precision point of view, but does it matter in practice? The second one should be faster. After testing with the test case, the precision changes nothing. So the second possibility is probably better. The change is only needed for the mac port, as it is here to fix the Apple Symbols font problem. I don't know if it happens with other fonts on other platforms, but the code without my patch is supposed to deal correctly with fonts without an "x" glyph. It seems that it is Core Graphics that doesn't properly deals with Apple Symbols font. An equivalent fix should ideally be inside CGFontGetXHeight().

Created attachment 66662 [details] Revised patch Revised patch with above changes. However, the patch perhaps changes some MathML GTK tests results, but I personally have no ways to test or to rebaseline. Alex Milowski could do it (I put him in CC) or a committer could rebaseline needed tests if the tree is on fire.

(In reply to comment #6) > I'll correct the style. > The cast is here, because xHeight is a float and Core Graphics seems to return a value just above zero but not zero for Apple Symbols fonts that have no "x" glyph. Thus the test if (!m_xHeight) is always false. I am slightly confused here. CGFontGetXHeight() returns an integer. Is the integer returned for Apple Symbols not zero? Or are you saying that the result of static_cast<float>(0) / m_unitsPerEm is not zero? That is odd. By casting to int and comparing to zero you are treating all values smaller than 1 as zero. > I thought that m_ascent was a float. It is indeed an int. > I don't know which possibility is the best: > - 0.66 * fAscent; > - static_cast<float>(2 * m_ascent / 3); > > The first one is probably better from a precision point of view, but does it matter in practice? > The second one should be faster. > > After testing with the test case, the precision changes nothing. So the second possibility is probably better. I still don’t understand the choice of either constant here. This is what <http://www.w3.org/TR/2009/CR-CSS2-20090908/syndata.html#length-units> says: In the cases where it is impossible or impractical to determine the x-height, a value of 0.5em should be used. > The change is only needed for the mac port, as it is here to fix the Apple Symbols font problem. The change is not font-specific, and there may be other fonts, which may be used on other platforms, that have a zero x-height. How do other platforms deal with it?

(In reply to comment #8) > (In reply to comment #6) > I am slightly confused here. CGFontGetXHeight() returns an integer. Is the integer returned for Apple Symbols not zero? Or are you saying that the result of static_cast<float>(0) / m_unitsPerEm is not zero? That is odd. By casting to int and comparing to zero you are treating all values smaller than 1 as zero. Indeed the result of the cast to a float followed by a division gives a non-zero results (CPU rounding problem). It is zero + epsilon where epsilon is the smallest float possible. I'm aware that the cast to an int is not perfect. A solution should be to store the result of GFontGetXHeight() and to test it. But in that case, Tiger returns a float for xHeight through NSFont... > > I still don’t understand the choice of either constant here. This is what <http://www.w3.org/TR/2009/CR-CSS2-20090908/syndata.html#length-units> says: > > In the cases where it is impossible or impractical to determine the x-height, a value of 0.5em should be used. I agree that 0.5 em is the most natural, but I wanted to have the same result between Apple Symbols and other fonts. The green box in the test case is not correctly aligned if I use 0.5 em. If I use 2/3, it renders like others fonts. Strange... > > > The change is only needed for the mac port, as it is here to fix the Apple Symbols font problem. > > The change is not font-specific, and there may be other fonts, which may be used on other platforms, that have a zero x-height. How do other platforms deal with it? On Cairo and Pango, they seem deal internally with such problems. On Haiku, Win, WinCE, and CairoWin they use: m_xHeight = m_ascent * 0.56f; when no "x" glyph exists or as default (depending on the implementations). On Core Graphics on Windows, it is like on the mac. On Linux and ChromiumWin they use 0.56, if no "x" glyph exists. However, 0.56 is erroneous as can been seen when zooming to have big fonts. Try with the test case and switch from apple symbol to arial or STIX fonts. Should I correct other implementations?

About the cast to an int, from a practical point of view, it is not a problem as fonts with a non-erreoneous xHeight of 1 will have roughly a 2 or 3 points height at max. The error of our guess won't be detectable for such small fonts.

Comment on attachment 66662 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=66662&action=review Seems OK but needs at least a comment and perhaps some tweaks to the code. > WebCore/platform/graphics/mac/SimpleFontDataMac.mm:281 > + if (!static_cast<int>(m_xHeight) && m_ascent) > + m_xHeight = static_cast<float>(2 * m_ascent / 3); This is a bit confusing. The code needs a comment explaining what it is doing. The change log explains that this works around a particular broken font, so the comment needs to at least say that. That’s the reason why the code exists at all. The technique of casting m_xHeight to int before checking it against zero is unusual and needs to be explained as well. Why is that a good way to check? What’s the point of checking m_ascent against zero? Is there a reason we need the computed m_xHeight to be an integer? If so, why? The comment should address the three why questions.

Created attachment 70843 [details] Revised patch I added comments in the code. About the int cast, it is a workaround for the "almost" zero value returned by CGFontGetXHeight(). I changed the guess to use fAscent (float) instead of m_ascent (int) to avoid the float cast. The check for fAscent is to avoid an unnecessary computation. As some tests are modified, it will break the GTK build. Could someone add the GTK test results? Or rebaseline just after landing?

Created attachment 71012 [details] Updated test results Rebaseline of some tests due to bug 47771

Created attachment 71538 [details] Updated patch with a test rebaseline tests rebaseline due to bug 43462

Comment on attachment 71538 [details] Updated patch with a test rebaseline I think this is OK, though sad that we need it

Created attachment 71542 [details] Correct tests The commit queue rejected the patch due to missing rebaselined tests in my last patch. This one fix it.

Comment on attachment 71538 [details] Updated patch with a test rebaseline Rejecting patch 71538 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: ts successful. Files=14, Tests=304, 1 wallclock secs ( 0.64 cusr + 0.13 csys = 0.77 CPU) Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/abarth/git/webkit-queue/LayoutTests Testing 21623 test cases. mathml/presentation/fenced.xhtml -> failed Exiting early after 1 failures. 17697 tests run. 265.62s total testing time 17696 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4654020

Comment on attachment 71542 [details] Correct tests Clearing flags on attachment: 71542 Committed r70304: <http://trac.webkit.org/changeset/70304>

All reviewed patches have been landed. Closing bug.