Summary: | Small-caps non-BMP characters are garbled in the complex text codepath | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||||
Component: | WebCore Misc. | Assignee: | Michael Saboff <msaboff> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, mmaxfield, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 154842 | ||||||||||
Attachments: |
|
Description
Michael Saboff
2016-03-01 13:49:19 PST
Created attachment 272601 [details]
Patch
Committed r197423: <http://trac.webkit.org/changeset/197423> Comment on attachment 272601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272601&action=review > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301 > - ASSERT(uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF); > + ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter))); This change is incorrect. The assertion correctly reflects what the code that calls this function can currently handle. The code that calls capitalized does not handle a replaced character that is non-BMP correctly. This function should have been returning Optional<UChar> given what the callers expect. Look for code below that puts the capitalized character into m_smallCapsBuffer. It does not turn the non-BMP character into a surrogate pair, and so depends on what was asserted here before. We need test cases that cover this. Comment on attachment 272601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272601&action=review >> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301 >> + ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter))); > > This change is incorrect. The assertion correctly reflects what the code that calls this function can currently handle. The code that calls capitalized does not handle a replaced character that is non-BMP correctly. This function should have been returning Optional<UChar> given what the callers expect. > > Look for code below that puts the capitalized character into m_smallCapsBuffer. It does not turn the non-BMP character into a surrogate pair, and so depends on what was asserted here before. We need test cases that cover this. Yes, you are correct. Michael and I are writing a patch write now to fix this up. Reopening to attach new patch. Created attachment 272620 [details]
Patch
Comment on attachment 272620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272620&action=review r=me. Remove the duplicate change in capitalized(). > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:301 > - ASSERT(uppercaseCharacter == baseCharacter || uppercaseCharacter <= 0xFFFF); > + ASSERT(uppercaseCharacter == baseCharacter || (U_IS_BMP(baseCharacter) == U_IS_BMP(uppercaseCharacter))); This is not needed since it is the prior patch. Created attachment 272622 [details]
Patch
Committed r197435: <http://trac.webkit.org/changeset/197435> Comment on attachment 272622 [details]
Patch
Clearing review flag, as the bug is in RESOLVED state.
|