Summary: | Parenthesis in RTL fonts are not mirrored with SVG fonts | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, webkit.review.bot, zimmermann | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 77648 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Philip Rogers
2012-01-25 18:40:35 PST
Created attachment 124053 [details]
First-pass at fixing mirroring with SVG fonts
Comment on attachment 124053 [details] First-pass at fixing mirroring with SVG fonts View in context: https://bugs.webkit.org/attachment.cgi?id=124053&action=review Nice first stab, needs more tests and some speedups: > Source/WebCore/svg/SVGFontData.cpp:142 > + if (mirror) > + remainingTextInRun = replaceMirroredCharacters(remainingTextInRun.characters(), remainingTextInRun.length()); > if (!currentCharacter && arabicForms.isEmpty()) > arabicForms = charactersWithArabicForm(remainingTextInRun, mirror); Danger Will Robinson :-) This can potentially break the arabic-form support - which already handles mirroring. Please add another testcase (you can grep for arabic-form and duplicate an existing) to see how it plays together with your testcase text data. It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot. You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters. Created attachment 124235 [details]
Update for fixing mirroring with SVG fonts
Added a test to show Arabic forms are preserved and cleaned up the glyph-selection-bidi-mirror.svg test.
Attachment 124235 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1
Source/WebCore/svg/SVGFontData.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
Source/WebCore/svg/SVGFontData.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > (From update of attachment 124053 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124053&action=review > > Nice first stab, needs more tests and some speedups: > > > Source/WebCore/svg/SVGFontData.cpp:142 > > + if (mirror) > > + remainingTextInRun = replaceMirroredCharacters(remainingTextInRun.characters(), remainingTextInRun.length()); > > if (!currentCharacter && arabicForms.isEmpty()) > > arabicForms = charactersWithArabicForm(remainingTextInRun, mirror); > > Danger Will Robinson :-) This can potentially break the arabic-form support - which already handles mirroring. Please add another testcase (you can grep for arabic-form and duplicate an existing) to see how it plays together with your testcase text data. Good eye :) I added the glyph-selection-arabic-forms.svg test and I think everything still works as it did (except now with correct mirroring!) I'm new to Arabic forms but if I understand them correctly, the original code did not handle mirroring for Arabic forms. A mirror parameter is passed to charactersWithArabicForm but only for determining whether the text should be processed ltr or rtl. I think it would be best to change that bool parameter from "mirror" to "rtl" in charactersWithArabicForm to prevent confusion--do you agree? > > It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot. > You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters. We definitely agree here but (per our IRC chat) I think it's best to leave this slightly slow function because the characters are immutable. If this ends up being a problem, a future optimization would be to do both mirroring and space normalization in a single pass. Note: This patch will fail tests on most platforms at the moment. I need to add/update platform-specific test expectations when I get in the office tomorrow morning. Created attachment 124241 [details]
Update for fixing mirroring with SVG fonts
(Fix import ordering and run check-webkit-style before uploading this time)
Comment on attachment 124241 [details] Update for fixing mirroring with SVG fonts Attachment 124241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11265580 New failing tests: svg/custom/glyph-selection-bidi-mirror.svg svg/custom/glyph-selection-arabic-forms.svg Comment on attachment 124241 [details] Update for fixing mirroring with SVG fonts View in context: https://bugs.webkit.org/attachment.cgi?id=124241&action=review r- for the lacking ChangeLog, and as there's more to do I think: > Source/WebCore/ChangeLog:8 > + > + Tests: svg/custom/glyph-selection-arabic-forms.svg Needs a real ChangeLog :-) > Source/WebCore/svg/SVGFontData.cpp:277 > + mirroredCharacters.append((UChar) mirroredChar(characters[i])); You're casting from UChar32 to UChar here - doesn't this break any non-bmp glyphs? What you're supposed to do is using: UChar32 character = 0; unsigned clusterLength = 0; SurrogatePairAwareTextIterator textIterator(characters, 0, length, length); while (textIterator.consume(character, clusterLength)) { the SurrogatePairAwareTextIterator.. - see eg. SVGGlyphMap.h collectGlyphsForString(), where the 'remainingTextInRun' is being passed to. The mirroring needs to happen on UChar32 basis - you wouldn't want to mirror parts of a surrogate pair, but instead the full UTF-16 code point, if I get this right. Then to create a String again, you need to decompose those potentially mirrored UTF-16 UChar32, back into two utf-8 UChar pairs. Now if mirroring the two parts of a surrogate pair, is the same as mirroring the full UChar32, then my comment is wrong, but I think this isn't the case. Please double check. (In reply to comment #7) > (From update of attachment 124241 [details]) > Attachment 124241 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11265580 > > New failing tests: > svg/custom/glyph-selection-bidi-mirror.svg > svg/custom/glyph-selection-arabic-forms.svg You didn't update the cr expectations. (In reply to comment #5) > Good eye :) I added the glyph-selection-arabic-forms.svg test and I think everything still works as it did (except now with correct mirroring!) I'm new to Arabic forms but if I understand them correctly, the original code did not handle mirroring for Arabic forms. A mirror parameter is passed to charactersWithArabicForm but only for determining whether the text should be processed ltr or rtl. I think it would be best to change that bool parameter from "mirror" to "rtl" in charactersWithArabicForm to prevent confusion--do you agree? Hah, good spot as well, yeah stupid that I named it mirror :-) Please rename to avoid the confusion. > > It's also a PITA that we're allocating a remainingTextInRun String object several times :( This needs to be optimized, applySVGGlyphSelection is hot. > > You shoulnd't need to build a whole new string, but instead iterate over the remainingTextInRun by looping over its const UChar* buffer, and call mirroredChar on the individual characters. Meep, forgot that a String is immutable, ignore that comment.. > We definitely agree here but (per our IRC chat) I think it's best to leave this slightly slow function because the characters are immutable. If this ends up being a problem, a future optimization would be to do both mirroring and space normalization in a single pass. ... fuly agreed. It needs someone to create a perf testcase for this, and Instruments-it. If we ever see it in profiling, we can revisit this. > Note: This patch will fail tests on most platforms at the moment. I need to add/update platform-specific test expectations when I get in the office tomorrow morning. Ah okay, great. Created attachment 124330 [details]
Update per reviewer comments
(In reply to comment #8) > (From update of attachment 124241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124241&action=review > > r- for the lacking ChangeLog, and as there's more to do I think: > > > Source/WebCore/ChangeLog:8 > > + > > + Tests: svg/custom/glyph-selection-arabic-forms.svg > > Needs a real ChangeLog :-) Added! > > > Source/WebCore/svg/SVGFontData.cpp:277 > > + mirroredCharacters.append((UChar) mirroredChar(characters[i])); > > You're casting from UChar32 to UChar here - doesn't this break any non-bmp glyphs? > What you're supposed to do is using: > > UChar32 character = 0; > unsigned clusterLength = 0; > SurrogatePairAwareTextIterator textIterator(characters, 0, length, length); > while (textIterator.consume(character, clusterLength)) { > > the SurrogatePairAwareTextIterator.. - see eg. SVGGlyphMap.h collectGlyphsForString(), where the 'remainingTextInRun' is being passed to. > The mirroring needs to happen on UChar32 basis - you wouldn't want to mirror parts of a surrogate pair, but instead the full UTF-16 code point, if I get this right. > > Then to create a String again, you need to decompose those potentially mirrored UTF-16 UChar32, back into two utf-8 UChar pairs. > Now if mirroring the two parts of a surrogate pair, is the same as mirroring the full UChar32, then my comment is wrong, but I think this isn't the case. > Please double check. This was a completely valid thing to check (and the new test glyph-selection-non-bmp.svg now proves it) but it turns out that the cast isn't problematic because WTF:UChar is 16 bits (unlike the 8bit version we're both used to). This cast was actually already occurring in Font::normalizeSpaces, which works similarly to replaceMirroredCharacters. Comment on attachment 124330 [details] Update per reviewer comments Attachment 124330 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11349786 New failing tests: svg/custom/glyph-selection-arabic-forms.svg Comment on attachment 124330 [details] Update per reviewer comments View in context: https://bugs.webkit.org/attachment.cgi?id=124330&action=review > Source/WebCore/svg/SVGFontData.cpp:279 > + StringBuilder mirroredCharacters; > + mirroredCharacters.reserveCapacity(length); > + > + for (unsigned i = 0; i < length; ++i) > + mirroredCharacters.append((UChar) mirroredChar(characters[i])); > + > + return mirroredCharacters.toString(); This loop is incorrect. It walks through UTF-16 character codes rather than walking through Unicode characters so won’t correctly mirror non-BMP characters. The need to cast mirroredChar’s result to (UChar) emphasizes the mistake. It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL. > Source/WebCore/svg/SVGFontData.h:59 > + String replaceMirroredCharacters(const UChar* characters, unsigned length) const; Naming this function that returns a newly created string “replace” is not so great. I would name it something like createStringWithMirroredCharacters. Created attachment 124346 [details]
Update per reviewer comments
(In reply to comment #13) > It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL. I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think? (In reply to comment #15) > (In reply to comment #13) > > > It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL. > I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think? Do you think it will have an effect? The mirrored characters are listed at http://www.unicode.org/Public/UNIDATA/BidiMirroring.txt and I don't think we even mirror the ones over 0xFFFF (they are listed as optional to mirror). Comment on attachment 124346 [details] Update per reviewer comments Attachment 124346 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11191845 New failing tests: svg/custom/glyph-selection-arabic-forms.svg Comment on attachment 124346 [details]
Update per reviewer comments
I'm not sure why EWS is complaining about this; there's no information in the log and it passes locally on Linux.
> I'm not sure why EWS is complaining about this; there's no information in the log and it passes locally on Linux.
The EWS might not have all its fonts set up exactly right.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > > > It’s straightforward to iterate the characters instead and handle surrogates correctly. There are examples of the use of U16_NEXT to read a character at a time in functions such as SegmentedFontData::containsCharacters and we can correctly write the characters in UTF-16 by using U16_LENGTH, U16_LEAD, and U16_TRAIL. > > I suggested before your review, that he should use SurrogatePairAwareTextiterator - which encapsulates this while handling hiragana/katagana correctly. What do you think? > > Do you think it will have an effect? The mirrored characters are listed at http://www.unicode.org/Public/UNIDATA/BidiMirroring.txt and I don't think we even mirror the ones over 0xFFFF (they are listed as optional to mirror). I wanted to expand on this a bit to make sure I understand correctly. The SurrogatePairTextIterator handles combining some Japanese semi-voiced characters. For example: べ - U+3079 - "HIRAGANA LETTER BE" へ - U+3078 - "HIRAGANA LETTER HE" ゙ - U+3099 - "COMBINING KATAKANA-HIRAGANA VOICED SOUND MARK" When iterating over the string "abcべdef" the iterator will combine the HE letter and the voice mark to form the Be character, so the iterator values will be 'a', 'b', 'c', 'Be', 'd', 'e', 'f'. The reason this doesn't matter for mirroring is that a relatively small set of characters are mirrored (such as parenthesis, brackets, etc) and they don't fall in the range of these Japanese voiced characters. Re: the EWS failure: I've installed some font packages in the process of tracking down this bug so it may be that my test expectations are the ones with the wrong font setup. I'll rebaseline on a vanilla linux box and put this patch back up tomorrow. Created attachment 124563 [details]
Update glyph-selection-arabic-forms.svg to only use Khan character in the hopes that EWS Linux will pass
Comment on attachment 124563 [details] Update glyph-selection-arabic-forms.svg to only use Khan character in the hopes that EWS Linux will pass View in context: https://bugs.webkit.org/attachment.cgi?id=124563&action=review Patch looks great, r=me with a few comments: > Source/WebCore/svg/SVGFontData.cpp:277 > + for (unsigned i = 0; i < length; ) { I would have used a while loop for this. No blocker. > LayoutTests/ChangeLog:11 > + which handles mirroring the characters when rendering selecting glyphs > + for SVG fonts. I also made a small cosmetic change in the function s/rendering// ? I'm not sure what you mean otherwise. The ChangeLog should be fixed at least before landing. Created attachment 124972 [details]
Update per reviewer comments
Comment on attachment 124972 [details]
Update per reviewer comments
Neglected to switch to the while loop. Updated patch forthcoming
Created attachment 124974 [details]
Update per reviewer comments
Updated patch--now with more while!
Comment on attachment 124974 [details] Update per reviewer comments Clearing flags on attachment: 124974 Committed r106551: <http://trac.webkit.org/changeset/106551> All reviewed patches have been landed. Closing bug. Created attachment 125187 [details]
Update for test failures
This change slightly modifies the tests to not require specific fonts on the system. Before I had an example of the test output using non-SVG fonts as a comparison, but that ended up being problematic due to cross-platform high-unicode fonts. This change also requires rebaselining on Windows and GTK due to font aliasing issues.
Attachment 125187 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Traceback (most recent call last):
File "Tools/Scripts/check-webkit-style", line 48, in <module>
sys.exit(CheckWebKitStyle().main())
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
patch_checker.check(patch)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
self._processor.process(lines, file_path, **kwargs)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
checker.check(lines)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 117, in check
overrides=overrides)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
is_lint_mode=True, overrides=overrides)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 721, in __init__
self._add_skipped_tests(port.skipped_tests(tests))
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
return self.skipped_layout_tests(test_list)
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 125187 [details]
Update for test failures
Philip says this is safe, and applies locally, just the bots have problems. I'll trust him :-)
(In reply to comment #30) > (From update of attachment 125187 [details]) > Philip says this is safe, and applies locally, just the bots have problems. I'll trust him :-) Filed a bug for the patch issue and Ojan jumped on it and landed a fix! https://bugs.webkit.org/show_bug.cgi?id=77744 This patch appears to be stuck somewhere in cqland. I'll re-upload bright and early Monday morning so we can land this. Created attachment 125637 [details]
Reupload now that check-webkit-style is fixed.
(In reply to comment #31) > This patch appears to be stuck somewhere in cqland. I'll re-upload bright and early Monday morning so we can land this. 10AM = bright and early in engineer terms :) Attachment 125637 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Last 3072 characters of output: xt:111: Path does not exist. tables/mozilla_expected_failures/marvin/tables_caption_align_right.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:112: Path does not exist. tables/mozilla_expected_failures/marvin/x_caption_align_left.xml [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:113: Path does not exist. tables/mozilla_expected_failures/marvin/x_caption_align_right.xml [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:114: Path does not exist. tables/mozilla_expected_failures/other/test4.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:115: Path does not exist. tables/mozilla/bugs/bug29157.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:116: Path does not exist. tables/mozilla/other/wa_table_thtd_rowspan.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:117: Path does not exist. tables/mozilla/other/wa_table_tr_align.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:118: Path does not exist. tables/mozilla_expected_failures/marvin/backgr_layers-show.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:119: Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_lhs.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:120: Path does not exist. tables/mozilla_expected_failures/marvin/table_frame_rhs.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:121: Path does not exist. fast/css/bidi-override-in-anonymous-block.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:122: Path does not exist. fast/dom/HTMLTableElement/colSpan.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:123: Path does not exist. fast/dom/HTMLTableElement/createCaption.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:124: Path does not exist. fast/repaint/table-section-repaint.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:127: Path does not exist. http/tests/media/video-buffering-repaints-controls.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:130: Path does not exist. fast/css/text-overflow-input.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:133: Path does not exist. fast/table/027.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:134: Path does not exist. fast/table/027-vertical.html [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:137: Path does not exist. svg/custom/glyph-selection-arabic-forms.svg [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:138: Path does not exist. svg/custom/glyph-selection-bidi-mirror.svg [test/expectations] [5] LayoutTests/platform/win/test_expectations.txt:139: Path does not exist. svg/custom/glyph-selection-non-bmp.svg [test/expectations] [5] Total errors found: 116 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style. (In reply to comment #34) > Attachment 125637 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > If any of these errors are false positives, please file a bug against check-webkit-style. I think this is a flake as well, as making a space change in platform/win/test_expectations.txt with a fresh, clean checkout will result in a similar error. I filed another bug against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=77873 Comment on attachment 125637 [details]
Reupload now that check-webkit-style is fixed.
Philip, can you find anyone manually landing this for you? Otherwise I'll try to do it today. Patch still looks good, I'll try cq+, let's see if it still applies.
Comment on attachment 125637 [details] Reupload now that check-webkit-style is fixed. Landed patch manually for Philip in r107211. |