Bug 4355

Summary:

CSS1: use small-caps variant font if it's available

Product:

WebKit

Reporter:

Nicholas Shanks <nickshanks>

Component:

CSS

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

ap, ddkilzer, emacemac7, eric, ian, justin.garcia, mitz, mmaxfield, robert

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
small-caps patch 1.0	darin: review-
Sample output before & after	none
small-caps patch 2.0	none
small-caps patch 2.0.1	none
small-caps patch 2.0.2	none
small-caps patch 2.1	none
layout-tests/fast/css/009.html	none
layout-tests/fast/css/009-expected.txt	none
ChangeLog entry (edit as appropriate)	none
small-caps patch 2.1.1	darin: review-
layout-tests/fast/css/009-expected.txt	none

Nicholas Shanks

Reported 2005-08-09 10:04:52 PDT

A simple patch which selects a small-caps variant of a parent font if there's one available, and uses 70% reduction otherwise. There are a couple of relavent issues which all match existing behaviour, and have not been addressed by this patch. • The small-caps font is assumed to have all of the lower-case letters that are present in the parent font. • This patch only uses the small-caps font for rendering lower-case letters of bicameral scripts. Glyphs for characters such as parentheses, numbers, currency symbols etc. are still taken from the parent font and may be oversized/misaligned as a result. I intend to address these issues in a future patch. The patch presented here is in the simplest form I could provide, as the more thorough patch I have been working on has had problems with mismatching glyph repertoires and fallback fonts.

Attachments
small-caps patch 1.0 (3.08 KB, patch) 2005-08-09 10:05 PDT, Nicholas Shanks	darin: review-	Details Formatted Diff Diff
Sample output before & after (20.22 KB, image/png) 2005-08-09 10:38 PDT, Nicholas Shanks	no flags	Details
small-caps patch 2.0 (12.94 KB, patch) 2005-08-09 17:43 PDT, Nicholas Shanks	no flags	Details Formatted Diff Diff
small-caps patch 2.0.1 (12.80 KB, patch) 2005-08-09 17:48 PDT, Nicholas Shanks	no flags	Details Formatted Diff Diff
small-caps patch 2.0.2 (12.74 KB, patch) 2005-08-10 05:42 PDT, Nicholas Shanks	no flags	Details Formatted Diff Diff
small-caps patch 2.1 (12.89 KB, patch) 2005-08-10 11:52 PDT, Nicholas Shanks	no flags	Details Formatted Diff Diff
layout-tests/fast/css/009.html (1.41 KB, text/html) 2005-08-10 11:56 PDT, Nicholas Shanks	no flags	Details
layout-tests/fast/css/009-expected.txt (3.68 KB, text/plain) 2005-08-10 12:06 PDT, Nicholas Shanks	no flags	Details
ChangeLog entry (edit as appropriate) (562 bytes, text/plain) 2005-08-10 12:08 PDT, Nicholas Shanks	no flags	Details
small-caps patch 2.1.1 (12.85 KB, patch) 2005-08-10 12:20 PDT, Nicholas Shanks	darin: review-	Details Formatted Diff Diff
layout-tests/fast/css/009-expected.txt (3.68 KB, text/plain) 2005-08-10 13:30 PDT, Nicholas Shanks	no flags	Details
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Nicholas Shanks

Comment 1 2005-08-09 10:05:57 PDT

Created attachment 3287 [details] small-caps patch 1.0

Nicholas Shanks

Comment 2 2005-08-09 10:38:22 PDT

Created attachment 3288 [details] Sample output before & after This is a screenshot of the results of applying this patch, before (top three) and after (bottom three). The HTML being rendered is reproduced below. You will see that in the before set, rows 2 and 3 are identical, whereas in the second set, the lower-case letters on row 2 now match the lower-case letters from row one, whilst the rest of row 2 remains the same. To demonstrate fallback, the pound sign (included in Prospero, but not in ProsperoSmallCaps) and euro sign (not included in either) are shown. <html> <style type="text/css"> html { font: 18pt Prospero; } .true { font-family: ProsperoSmallCaps; } .css { font-variant: small-caps; } .simulate > span { text-transform: uppercase; font-size: 70%; } </style> <div class="true">This is a sample of Small Caps £€</div> <div class="css">This is a sample of Small Caps £€</div> <div class="simulate">T<span>his</span> <span>is</span> <span>a</span> <span>sample</span> <span>of</span> S<span>mall</span> C<span>aps</span> £€</div>

Darin Adler

Comment 3 2005-08-09 11:48:42 PDT

Looks to me like the test cases will work as layout tests.

Darin Adler

Comment 4 2005-08-09 11:49:19 PDT

Comment on attachment 3287 [details] small-caps patch 1.0 Please fix formatting. You need a space between the "if" and the "(" on lines like: + if([renderer _capitalizeSmallCaps]) Also, seems better to just make the capitalizeSmallCaps field @public rather than calling a method on every character to check that boolean. Otherwise, looks good, and we can do review+ if we also have a layout test demonstrating the correct behavior -- they metrics in the layout test will be the telltale about whether the correct font and glyph was chosen.

Nicholas Shanks

Comment 5 2005-08-09 13:45:33 PDT

Actually, that patch fails miserably for AAT fonts with kLetterCaseType: kSmallCapsSelector available (Big Caslon, Didot, Hoefler Text, etc). The returned font is the same, and the regular lowercase glyphs get chosen from the unicode glyph map. I have fixed this for the ATSU branch (in _createATSUTextLayoutForRun, modifying the _ATSUSstyle object), but am not sure how to go about fixing it via CG yet. Since CG is by default used for all characters below U+0300 I had to force the ATSU branch to test this.

Nicholas Shanks

Comment 6 2005-08-09 17:43:52 PDT

Created attachment 3303 [details] small-caps patch 2.0 This adds support for AAT/OpenType small-caps features via the ATSUI codepath, variant typefaces and 70% simulations still go down the CG path. Maciej suggested I try setting the style in fillStyleWithAttributes too, I will look at that tomorrow. I am wondering what fonts to use in the regression test. There are AAT small-caps fonts which shipped with the OS (Hoefler Text is striking), and fonts without small-caps, for the 70% mode, but there doesn't appear to be any common font whose small-caps are available via an alternative typeface. The example I used above, Prospero, is the only one I have. My current test HTML looks like this: <html> <style type="text/css"> .hoefler { font: 36pt "Hoefler Text"; } .prospero { font: 36pt "Prospero"; } .prospero > .true { font-family: "ProsperoSmallCaps"; } .lucidagrande { font: 36pt "Lucida Grande"; } .css { font-variant: small-caps; } .simulate > span { text-transform: uppercase; font-size: 70%; } </style> <div class="hoefler"> <div class="css">This is a sample of Small Caps £€</div> <div class="simulate">T<span>his</span> <span>is</span> <span>a</span> <span>sample</span> <span>of</span> S<span>mall</span> C<span>aps</span> £€</div> </div> <div class="prospero"> <div class="true">This is a sample of Small Caps £€</div> <div class="css">This is a sample of Small Caps £€</div> <div class="simulate">T<span>his</span> <span>is</span> <span>a</span> <span>sample</span> <span>of</span> S<span>mall</span> C<span>aps</span> £€</div> </div> <div class="lucidagrande css">This is a sample of Small Caps £€</div>

Nicholas Shanks

Comment 7 2005-08-09 17:48:43 PDT

Created attachment 3304 [details] small-caps patch 2.0.1 forgot to take out an NSLog() call

Nicholas Shanks

Comment 8 2005-08-10 05:42:28 PDT

Created attachment 3310 [details] small-caps patch 2.0.2 typecast pointer to void* in free() call and other little fixes

Nicholas Shanks

Comment 9 2005-08-10 05:55:11 PDT

I have looked into supporting AAT fonts with small-caps via the CG codepath, but the ATSUStyle object that is created for the CG path in fillStyleWithAttributes() is then used to create an 'ATSStyleGroup' via WKGetATSStyleGroup(), which is passed to WKConvertCharToGlyphs(). One of both of these Apple-internal functions from lbWebKitSystemInterface.a ignores AAT/OpenType font features, and thus I have no way to get the CG path to return the small-caps glyphs from a font without doing a lot of redundant and probably slow legwork myself (i.e. scanning the font for the correct glyphs, something which would require far more knowledge of Apple's AAT and OpenType implementations that I have).

Nicholas Shanks

Comment 10 2005-08-10 11:52:39 PDT

Created attachment 3314 [details] small-caps patch 2.1 Swapped algorithm around so that it checks for AAT small-caps first, which removes an ambiguity when comparing screen and printer fonts for equality.

Nicholas Shanks

Comment 11 2005-08-10 11:56:43 PDT

Created attachment 3315 [details] layout-tests/fast/css/009.html

Nicholas Shanks

Comment 12 2005-08-10 12:06:05 PDT

Created attachment 3318 [details] layout-tests/fast/css/009-expected.txt

Nicholas Shanks

Comment 13 2005-08-10 12:08:24 PDT

Created attachment 3319 [details] ChangeLog entry (edit as appropriate)

Nicholas Shanks

Comment 14 2005-08-10 12:20:30 PDT

Created attachment 3322 [details] small-caps patch 2.1.1 more tinkering: removed redundant parameter

Darin Adler

Comment 15 2005-08-10 13:21:00 PDT

Comment on attachment 3322 [details] small-caps patch 2.1.1 Looking good! Here are the outstanding issues I see: 1) Still a few cases of "if(" instead of "if (" in this patch. 2) Using the ATSU code path is going to have a number of undesirable effects, including incorrect handling of "\n" characters -- so we might not want to do this until we fix those. I think Justin Garcia from WebKit team was just looking at that recently. 3) I still don't see this handling cases where the small caps variant doesn't cover all the characters needed. Can this result in a mix of two different fonts on a page? Other browsers don't do this, so this might cause us to look broken on pages they look fine on. Is there a reason I should not be worried about this? 4) This: + // will choke on surrogate pairs? +// if(!u_isUUppercase(run->characters[i])) is not a practical issue. I believe the function does the right thing with half of a surrogate pair (nothing). Also, why land the u_isUUppercase check commented out? Lets just omit it. I don't like adding commented-out code. 6) I don't understand why this patch both triggers the ATSU code path for all "true small caps" cases, but also still adds true small caps support to the CG path. We don't need to do both. 7) A simpler way to do this: + if(style->smallCaps && simulatedSmallCaps) + free((void *)capsRun.characters); is to initialize capsRun.characters to NULL and then just leave out the if. 8) Misspelled separate in a comment "variant as seperate font". 9) Please omit the uneeded cast to (void *) here: + free((void *)selectors); 10) Why does small caps also set old-style numbers? Need a rationale in the code at least so later contributors will know why we should keep this around. And I'd like to see the test case include a test of that unless it's impossible.

Nicholas Shanks

Comment 16 2005-08-10 13:30:49 PDT

Created attachment 3325 [details] layout-tests/fast/css/009-expected.txt Forgot to set DYLD_FRAMEWORK_PATH when running DumpRenderTree to generate this file.

Dave Hyatt

Comment 17 2005-08-10 15:06:18 PDT

Are we regressing small-caps performance though? The ATSUI code path is (quite literally) more than 4x slower than the CG code path. If we are forced down the ATSUI code path where we weren't before, then performance could suffer considerably. That may be acceptable given how rarely small-caps is used though.

Nicholas Shanks

Comment 18 2005-08-10 15:29:12 PDT

In reply to comment #15: 1) Fixed. 2) Fair enough. 3) In the AAT case, the only way to tell is by comparing the glyph IDs that would be drawn before and after turning on the feature. And then you can't even tell if they are small caps, just that a different glyph is being drawn. For example Hoefler Text includes Latin small-caps, but not Cyrillic small-caps, even though it does have Cyrillic miniscules. The selected (non-substitute) small-caps font has no unicode glyph map created for it, it is simply assumed to have all characters present in the Regular font. I realise this is a bug and I tried to resolve it by instantiating the small-caps renderer with the small-caps font in it's 'font' variable, and doing away with the smallCapsFont variable altogether. This however was causing segfaults and I couldn't figure out why, so I aborted that idea. I think if I tried again I might be able to get this idea to work, and the Regular font would be added to the fallback list at the top. The unicode mapping is only used on the CG path, so this bug only affects typefaces where the Regular and Small Caps versions are in separate fonts, and miniscules present in the Regular but absent in the Small Caps (e.g. Cyrilic) are used. In this instance a box is drawn. For other missing characters behaviour is to fall back to the Regular font at 100% without capitalisation. 4) I just copied and pasted applyMirroringToRun(), which had that comment, so I left it in as I didn't know the status of it. I wasn't sure if checking if the letter is not uppercase was worth it or not, I presume it'll be just as quick to attempt to uppercase it than to check in the first place. 5) There is no step 5! 6) The only method/function this touches which is specifically in the CG path is widthForNextCharacter (), which is to obtain a small-caps version of the substitute font if available. You're right, I didn't see this as a way that it could jump from simulated/variant (CG-compatible) to AAT small-caps. I will change that back to always using a 70% scale for the time being. This removes the need for a separate - smallCapsFontForFont: method, since we'd no longer need to pass _fontUsed to it. I could leave that change in for future use on the CG path when that is possible, or I could return it to the way it was for the time being (and avoid a call to objc_msgsend() ). 7) Done. 8) You have keen eyes! Fixed. 9) Done. What about "free((void *)capsRun.characters)" leave that as is, matching syntax elsewhere? 10) Because I thought it would look nice :-) I had thought there were no system-shipped fonts that had both small-caps and old-style numbers as separate AAT properties, and where the old-style numbers also caused a change to the metrics of the string as would be required for testing. However after hunting a bit harder, I have found that Apple Chancery does, lining numbers are a pixel or two wider at 48pt than the old-style numbers are, and a dollar sign which is significantly wider with lining numbers. It's small-caps also have none of the swashes of the regular caps, so are very visually distinct. As to whether the old-style numbers feature should be activated at all, well, I guess it's 50/50. I like them, and think they go well together. Is that enough rationale? I will hold off from doing further changes until you've responded to some of the open points that remain. In response to comment #17: The ATSUI path is only taken when the font being used has an AAT/OpenType Small Caps feature as determined by ATSUGetFontFeatures(), otherwise we stay on CG. There is also a very small performance hit when first obtaining the font for the small-caps renderer, or (presently but about to be removed according to point 6 Darin made above) when attempting to obtain a small-caps version of a substitute font for any particular character on the CG path. I've just realised that no attempt is being made to generate a small-caps variant for substitute fonts on the ATSU path! I will look into doing that too.

Nicholas Shanks

Comment 19 2005-08-10 16:36:27 PDT

Bad news regarding Apple Chancery: it defaults to old-style figures, meaning turning them on makes no change, hence no difference in metrics. Hoefler Text Black, Italic and Black Italic (but not Regular) also have both Small Caps and Old-Style Figures but these three, sadly, also have them on by default. I conclude that there are no fonts shipping with thee OS suitable for demonstrating whether or not activating Old-Style Figures is working, but given that this is set in the same system call as small caps itself, I can't see how anything can break one and not the other. For fonts that don't support Old-Style Figures, the setting is simply ignored. Fonts that support Old-Style Figures, but not Small-Caps are not even considered for using in a small-caps run.

Nicholas Shanks

Comment 20 2006-03-26 05:19:17 PST

Pending since August 2005: response from darin to comment #15 and comment #18. e.g. Has Justin fixed the /n handling in ATSU? Suggested course of action for fonts with an incomplete small-caps repertoire?

Nicholas Shanks

Comment 21 2006-06-04 07:50:38 PDT

Justin, could you respond to Darin's comments on this. It's been in limbo since August :-/

Dave Hyatt

Comment 22 2006-06-04 11:31:55 PDT

Text has changed drastically since this patch was last worked on, so it would need to be recoded anyway. :(

Nicholas Shanks

Comment 23 2011-10-08 00:21:46 PDT

I'll have another crack at this in the next month. Reassigning to me since hyatt hasn't touched it in five years.

Myles C. Maxfield

Comment 24 2014-11-17 10:04:12 PST

Assigning to me because it hasn't been touched in years

Myles C. Maxfield

Comment 25 2015-12-10 00:47:16 PST

*** This bug has been marked as a duplicate of bug 149774 ***

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