REOPENED Bug 100050
Font's fast code path doesn't handle partial runs correctly when kerning or ligatures are enabled
https://bugs.webkit.org/show_bug.cgi?id=100050
Summary Font's fast code path doesn't handle partial runs correctly when kerning or l...
mitz
Reported 2012-10-22 16:30:04 PDT
drawSimpleText, drawEmphasisMarksForSimpleText, and selectionRectForSimpleText all rely on advancing a WidthIterator to the “from” offset, then advancing it again to the “to” offset. If from > 0 or to < the run’s length, then WidthIterator doesn’t get a chance to apply font transforms between the characters on either side of the “from” and “to” boundaries, which can lead to incorrect results.
Attachments
Patch (9.40 KB, patch)
2013-04-25 08:51 PDT, Allan Sandfeld Jensen
no flags
Patch (11.14 KB, patch)
2013-04-25 09:45 PDT, Allan Sandfeld Jensen
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (373.02 KB, application/zip)
2013-04-25 10:54 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (430.65 KB, application/zip)
2013-04-25 13:28 PDT, Build Bot
no flags
Patch (12.42 KB, patch)
2013-04-26 06:11 PDT, Allan Sandfeld Jensen
no flags
Patch (12.45 KB, patch)
2013-04-26 07:15 PDT, Allan Sandfeld Jensen
no flags
Patch (12.44 KB, patch)
2013-04-26 07:21 PDT, Allan Sandfeld Jensen
no flags
Patch (12.74 KB, patch)
2013-05-13 05:16 PDT, Allan Sandfeld Jensen
no flags
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (592.09 KB, application/zip)
2013-05-13 06:33 PDT, Build Bot
no flags
Patch (12.78 KB, patch)
2013-05-13 07:24 PDT, Allan Sandfeld Jensen
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (552.96 KB, application/zip)
2013-05-13 17:50 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (621.54 KB, application/zip)
2013-05-13 19:14 PDT, Build Bot
no flags
Patch (13.35 KB, patch)
2013-05-14 02:19 PDT, Allan Sandfeld Jensen
no flags
Patch (15.43 KB, patch)
2013-05-14 07:53 PDT, Allan Sandfeld Jensen
no flags
Patch (39.84 KB, patch)
2013-05-15 02:52 PDT, Allan Sandfeld Jensen
no flags
Patch (41.68 KB, patch)
2013-06-05 05:00 PDT, Allan Sandfeld Jensen
no flags
Patch (6.85 KB, patch)
2013-08-26 04:35 PDT, Allan Sandfeld Jensen
darin: review+
Test that shows failing selection. (133 bytes, text/html)
2014-02-07 10:20 PST, Enrica Casucci
no flags
mitz
Comment 1 2012-10-22 20:18:06 PDT
See also bug 100068.
Allan Sandfeld Jensen
Comment 2 2013-04-25 08:51:31 PDT
mitz
Comment 3 2013-04-25 09:16:09 PDT
Comment on attachment 199667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199667&action=review > Source/WebCore/platform/graphics/WidthIterator.cpp:115 > + // Transforms may also change the advance of the preceding character if there is one. > + int transformStart = lastGlyphCount > 0 ? lastGlyphCount - 1 : 0; I don’t understand this. It is not necessarily true that glyphBuffer->fontDataAt(lastGlyphCount - 1) == fontData, but it makes no sense to pass glyphs from a different font to SimpleFontData::applyTransforms(). I also don’t understand why this consideration applies to the glyph just before lastGlyphCount but not to the glyph before that.
Allan Sandfeld Jensen
Comment 4 2013-04-25 09:36:03 PDT
(In reply to comment #3) > (From update of attachment 199667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199667&action=review > > > Source/WebCore/platform/graphics/WidthIterator.cpp:115 > > + // Transforms may also change the advance of the preceding character if there is one. > > + int transformStart = lastGlyphCount > 0 ? lastGlyphCount - 1 : 0; > > I don’t understand this. It is not necessarily true that glyphBuffer->fontDataAt(lastGlyphCount - 1) == fontData, but it makes no sense to pass glyphs from a different font to SimpleFontData::applyTransforms(). I also don’t understand why this consideration applies to the glyph just before lastGlyphCount but not to the glyph before that. Yeah, there is a number of other problems with that solution. It only handles 2-character kerning and can not deal with ligatures. I am testing another approach that doesn't try to fix the short-comings of WidthIterator, but work around it more generically.
Allan Sandfeld Jensen
Comment 5 2013-04-25 09:45:06 PDT
Created attachment 199674 [details] Patch Second approach, always run the WidthIterator on complete TextRuns
Build Bot
Comment 6 2013-04-25 10:54:12 PDT
Comment on attachment 199674 [details] Patch Attachment 199674 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/220196 New failing tests: editing/selection/anchor-focus1.html editing/selection/select-across-readonly-input-5.html editing/selection/inline-closest-leaf-child.html editing/selection/drag-select-1.html fast/forms/focus-selection-textarea.html editing/selection/move-between-lines-of-different-editabilities.html editing/selection/caret-at-bidi-boundary.html editing/selection/5057506.html editing/pasteboard/4242293-1.html editing/selection/programmatic-selection-on-mac-is-directionless.html fast/css/layerZOrderCrash.html editing/selection/hit-test-anonymous.html fast/forms/focus-selection-input.html editing/selection/hit-test-on-text-with-line-height.html editing/selection/fake-doubleclick.html editing/selection/click-left-of-rtl-wrapping-text.html editing/pasteboard/drop-inputtext-acquires-style.html fast/css/text-overflow-ellipsis-text-align-center.html editing/selection/14971.html editing/selection/select-bidi-run.html fast/css/text-overflow-ellipsis-text-align-right.html editing/selection/fake-drag.html editing/execCommand/insert-list-with-noneditable-content.html editing/selection/select-across-readonly-input-3.html editing/selection/caret-bidi-first-and-last-letters.html editing/execCommand/findString-2.html fast/loader/javascript-url-in-object.html fast/dom/Document/CaretRangeFromPoint/caretRangeFromPoint-in-zoom-and-scroll.html editing/selection/select-across-readonly-input-2.html fast/css/text-overflow-ellipsis-text-align-left.html
Build Bot
Comment 7 2013-04-25 10:54:15 PDT
Created attachment 199686 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Allan Sandfeld Jensen
Comment 8 2013-04-25 12:02:38 PDT
Comment on attachment 199674 [details] Patch Has issue with offset calculation that needs to be investigated.
Build Bot
Comment 9 2013-04-25 13:28:50 PDT
Comment on attachment 199674 [details] Patch Attachment 199674 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/11371 New failing tests: editing/selection/anchor-focus1.html editing/selection/inline-closest-leaf-child.html editing/selection/selection-extend-should-not-move-across-caret-on-mac.html editing/selection/drag-select-1.html editing/selection/move-between-lines-of-different-editabilities.html editing/selection/caret-at-bidi-boundary.html editing/selection/5057506.html editing/pasteboard/4242293-1.html editing/selection/programmatic-selection-on-mac-is-directionless.html editing/pasteboard/copy-image-with-alt-text.html fast/css/layerZOrderCrash.html editing/selection/hit-test-anonymous.html editing/pasteboard/smart-paste-001.html editing/selection/hit-test-on-text-with-line-height.html editing/selection/fake-doubleclick.html editing/selection/click-left-of-rtl-wrapping-text.html fast/css/text-overflow-ellipsis-text-align-center.html editing/selection/14971.html editing/selection/select-bidi-run.html editing/selection/fake-drag.html editing/execCommand/insert-list-with-noneditable-content.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html editing/selection/caret-bidi-first-and-last-letters.html editing/selection/select-from-textfield-outwards.html editing/execCommand/findString-2.html fast/dom/Document/CaretRangeFromPoint/caretRangeFromPoint-in-zoom-and-scroll.html editing/selection/user-select-all-selection.html
Build Bot
Comment 10 2013-04-25 13:28:52 PDT
Created attachment 199744 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Allan Sandfeld Jensen
Comment 11 2013-04-26 06:11:30 PDT
Created attachment 199822 [details] Patch Fixed offsetForPosition and removed the separate iteration to calculate glyphFrom and glyphTo
EFL EWS Bot
Comment 12 2013-04-26 06:23:11 PDT
EFL EWS Bot
Comment 13 2013-04-26 06:32:34 PDT
Build Bot
Comment 14 2013-04-26 06:41:33 PDT
Build Bot
Comment 15 2013-04-26 06:53:09 PDT
Allan Sandfeld Jensen
Comment 16 2013-04-26 07:15:59 PDT
Created attachment 199831 [details] Patch Avoid comparisons and convertions between int and unsigned int
Allan Sandfeld Jensen
Comment 17 2013-04-26 07:21:50 PDT
Antoine Quint
Comment 18 2013-04-30 07:27:50 PDT
Radar WebKit Bug Importer
Comment 19 2013-04-30 08:37:55 PDT
Allan Sandfeld Jensen
Comment 20 2013-05-07 03:43:29 PDT
Any comments or reviews? Also are there anything in the rdar problems I should know about?
Jocelyn Turcotte
Comment 21 2013-05-08 09:33:34 PDT
Comment on attachment 199832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199832&action=review > Source/WebCore/platform/graphics/FontFastPath.cpp:436 > + if (to == run.length()) { > + afterWidth = totalWidth; > + glyphPos = localGlyphBuffer.size(); > + } else { > + for (; glyphPos < localGlyphBuffer.size() && it.m_characterIndex[glyphPos] < to; ++glyphPos) > + afterWidth += localGlyphBuffer.advanceAt(glyphPos).width(); > + } It would be nice if you could avoid the special (to == run.length()) case by going from the end to "to". Something like: afterWidth = totalWidth; glyphPos = localGlyphBuffer.size() - 1; for (glyphPos >= glyphFrom) afterWidth -= advanceAt(glyphPos)... > Source/WebCore/platform/graphics/FontFastPath.cpp:597 > + if (to == run.length()) ditto > Source/WebCore/platform/graphics/FontFastPath.cpp:641 > - offset = it.m_currentCharacter; > - float w; > - if (!it.advanceOneCharacter(w, localGlyphBuffer)) > - break; > + float w = glyphBuffer.advanceAt(glyphPosition++).width(); This makes this methods more complex than it already was. Can you think of any way to use an abstraction similar to WidthIterator::advanceOneCharacter?
Allan Sandfeld Jensen
Comment 22 2013-05-13 05:16:21 PDT
Created attachment 201550 [details] Patch cleaned-up following review comment
Build Bot
Comment 23 2013-05-13 06:33:13 PDT
Comment on attachment 201550 [details] Patch Attachment 201550 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/346794 New failing tests: fast/inline/justify-emphasis-inline-box.html fast/text/justify-ideograph-leading-expansion.html
Build Bot
Comment 24 2013-05-13 06:33:16 PDT
Created attachment 201555 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Allan Sandfeld Jensen
Comment 25 2013-05-13 07:24:06 PDT
Build Bot
Comment 26 2013-05-13 17:50:20 PDT
Comment on attachment 201565 [details] Patch Attachment 201565 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/375941 New failing tests: fast/inline/justify-emphasis-inline-box.html fast/text/justify-ideograph-leading-expansion.html
Build Bot
Comment 27 2013-05-13 17:50:23 PDT
Created attachment 201652 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Build Bot
Comment 28 2013-05-13 19:14:33 PDT
Comment on attachment 201565 [details] Patch Attachment 201565 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/375955 New failing tests: fast/inline/justify-emphasis-inline-box.html fast/text/justify-ideograph-leading-expansion.html
Build Bot
Comment 29 2013-05-13 19:14:36 PDT
Created attachment 201665 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Allan Sandfeld Jensen
Comment 30 2013-05-14 02:10:29 PDT
The assertions are not reproducable in the Qt port.
Allan Sandfeld Jensen
Comment 31 2013-05-14 02:19:58 PDT
Jocelyn Turcotte
Comment 32 2013-05-14 07:12:03 PDT
Comment on attachment 201690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201690&action=review > Source/WebCore/platform/graphics/FontFastPath.cpp:609 > -int Font::offsetForPositionForSimpleText(const TextRun& run, float x, bool includePartialGlyphs) const > +int Font::offsetForPositionForSimpleText(const TextRun& run, const float x, bool includePartialGlyphs) const I think this should match the header. > Source/WebCore/platform/graphics/FontFastPath.cpp:626 > + if (currentX - glyphWidth / 2.0f <= x) currentX has its origin on the right of the screen while x has its origin on the left, does it? To use this variable name it would be nice to match their meaning (i.e. "currentX += glyphWidth;" should be used at the end of both the ltr and rtl loops). > Source/WebCore/platform/graphics/WidthIterator.cpp:269 > + m_characterIndex.append(textIterator.currentCharacter()); This line feels quite out of place and can see this being a potential source of problem with future patches. The relationship between m_characterIndex and glyphBuffer is that there should be an entry added to m_characterIndex for each glyph added to glyphBuffer, and this should be obvious in the code. A thing that could help is to add "m_characterIndex.append(textIterator.currentCharacter() - advanceLength);" immediately after each "glyphBuffer->add(...)" instead. Or keep textIterator.currentCharacter() before calling textIterator.advance and use the variable once we do glyphBuffer->add.
Allan Sandfeld Jensen
Comment 33 2013-05-14 07:28:41 PDT
(In reply to comment #32) > > Source/WebCore/platform/graphics/FontFastPath.cpp:626 > > + if (currentX - glyphWidth / 2.0f <= x) > > currentX has its origin on the right of the screen while x has its origin on the left, does it? > To use this variable name it would be nice to match their meaning (i.e. "currentX += glyphWidth;" should be used at the end of both the ltr and rtl loops). > No currentX is in the same origin as x, but in the RTL case we find x by starting with the full width and moving to the left with each glyph.
Jocelyn Turcotte
Comment 34 2013-05-14 07:40:57 PDT
(In reply to comment #33) Ok yep I see it now. Also to do currentX += glyphWidth; like I wanted we would need to start from the end of the glyphRun, and this wouldn't make things any simpler.
Allan Sandfeld Jensen
Comment 35 2013-05-14 07:53:27 PDT
Jocelyn Turcotte
Comment 36 2013-05-14 08:04:27 PDT
Comment on attachment 201713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201713&action=review The general logic looks good to me, but I think you should definitely get somebody to look at it who knows that code better. > Source/WebCore/platform/graphics/WidthIterator.cpp:211 > + int currentCharacterIndex = textIterator.currentCharacter(); Nit: This could go at the top of the loop.
Allan Sandfeld Jensen
Comment 37 2013-05-14 08:52:32 PDT
(In reply to comment #36) > (From update of attachment 201713 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201713&action=review > > The general logic looks good to me, but I think you should definitely get somebody to look at it who knows that code better. > Dan (mitz), what do think now?
Darin Adler
Comment 38 2013-05-14 10:04:12 PDT
Comment on attachment 201713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201713&action=review > Source/WebCore/ChangeLog:15 > + This fix is necessary for Qt because the complex font-path can not disable > + shaping, leading to the complex path painting slighly different from the > + fast path, which messes up selection painting. > + > + No change in functionality, no new tests. If this fix is necessary for Qt, then there must be tests that now pass in Qt that used to fail. Those should be listed here. > Source/WebCore/WebCore.order:-6413 > -__ZN7WebCore13WidthIterator19advanceOneCharacterERfPNS_11GlyphBufferE Don’t bother updating this file by hand. It is computer-generated from time to time and should not be edited.
Darin Adler
Comment 39 2013-05-14 10:05:04 PDT
Comment on attachment 201713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201713&action=review >> Source/WebCore/ChangeLog:15 >> + No change in functionality, no new tests. > > If this fix is necessary for Qt, then there must be tests that now pass in Qt that used to fail. Those should be listed here. I mean, there should be a change for test expectations to expect that those tests now pass.
Allan Sandfeld Jensen
Comment 40 2013-05-15 00:51:56 PDT
(In reply to comment #39) > (From update of attachment 201713 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201713&action=review > > >> Source/WebCore/ChangeLog:15 > >> + No change in functionality, no new tests. > > > > If this fix is necessary for Qt, then there must be tests that now pass in Qt that used to fail. Those should be listed here. > > I mean, there should be a change for test expectations to expect that those tests now pass. It is a change in font rendering, and none of the layout tests trigger it since it depends on specific fonts with ligatures. While I could add a test it would have to be a pixel test introducing a new font to layout tests.
Allan Sandfeld Jensen
Comment 41 2013-05-15 02:52:00 PDT
Allan Sandfeld Jensen
Comment 42 2013-06-03 06:31:18 PDT
The latest patch was merged into the Qt 5.1 branch in order to make it for Qt 5.1 RC1
Allan Sandfeld Jensen
Comment 43 2013-06-05 05:00:23 PDT
Created attachment 203806 [details] Patch Now with a proper automatic reference test
Allan Sandfeld Jensen
Comment 44 2013-06-07 08:24:24 PDT
darin, mitz: any thoughts on the last patch?
Allan Sandfeld Jensen
Comment 45 2013-06-11 04:24:27 PDT
I discover another related bug this patch solves. Partial text-selection when using web SVG fonts and any font features set. SVG fonts are only supported by the fast code path, but we currently end up forcing them to the complex font path when drawing partial text-selection. This often leads to a completely different font being used for the partially selected lines.
Antti Koivisto
Comment 46 2013-07-02 04:56:03 PDT
Comment on attachment 203806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > Source/WebCore/ChangeLog:9 > + Always let WidthIterator iterate over an entire TextRun to avoid problems > + with pixel rounding or shaping on partial runs. This sounds like something that would regress performance. Please explain why it wouldn't.
Allan Sandfeld Jensen
Comment 47 2013-07-02 05:23:25 PDT
(In reply to comment #46) > (From update of attachment 203806 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > > > Source/WebCore/ChangeLog:9 > > + Always let WidthIterator iterate over an entire TextRun to avoid problems > > + with pixel rounding or shaping on partial runs. > > This sounds like something that would regress performance. Please explain why it wouldn't. In all common cases we already iterate over the entire run. The only case we didn't is the for a line that is only partially selected (and then we only optimized the LTR case). In this case we now also iterate over the entire run, but while that is likely slightly slower than only iterating over part of it, it is only a single line, which averages 60 characters, so at most 30 more characters to iterate over. For RTL lines there is no difference.
Allan Sandfeld Jensen
Comment 48 2013-07-31 09:40:33 PDT
(In reply to comment #47) > (In reply to comment #46) > > (From update of attachment 203806 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > > > > > Source/WebCore/ChangeLog:9 > > > + Always let WidthIterator iterate over an entire TextRun to avoid problems > > > + with pixel rounding or shaping on partial runs. > > > > This sounds like something that would regress performance. Please explain why it wouldn't. > > In all common cases we already iterate over the entire run. The only case we didn't is the for a line that is only partially selected (and then we only optimized the LTR case). In this case we now also iterate over the entire run, but while that is likely slightly slower than only iterating over part of it, it is only a single line, which averages 60 characters, so at most 30 more characters to iterate over. For RTL lines there is no difference. Did that adequately explain it? Are there any other issue with the patch?
Antti Koivisto
Comment 49 2013-08-21 05:39:36 PDT
Comment on attachment 203806 [details] Patch Ok, looks like a good simplification then. r=me
Antti Koivisto
Comment 50 2013-08-21 05:46:53 PDT
Comment on attachment 203806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > Source/WebCore/platform/graphics/Font.cpp:-289 > - CodePath codePathToUse = codePath(run); > - // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050 > - if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length())) > - codePathToUse = Complex; I think you are underselling this patch. This might actually improve selection performance with kerning & ligatures enabled.
Allan Sandfeld Jensen
Comment 51 2013-08-21 06:42:00 PDT
Darin Adler
Comment 52 2013-08-21 09:55:40 PDT
What’s the typical size of m_characterIndex? Should it have some inline capacity to avoid memory allocation?
Allan Sandfeld Jensen
Comment 53 2013-08-21 11:05:10 PDT
(In reply to comment #52) > What’s the typical size of m_characterIndex? Should it have some inline capacity to avoid memory allocation? That would probably be a good idea. I think the average size will be between 10 and 50, so having an inline capacity of 64 would probably speed it up.
Darin Adler
Comment 54 2013-08-21 11:15:07 PDT
I like to make a histogram dumper to find out what the numbers are on actual web pages in a case like this.
Allan Sandfeld Jensen
Comment 55 2013-08-23 09:18:14 PDT
(In reply to comment #54) > I like to make a histogram dumper to find out what the numbers are on actual web pages in a case like this. I dumped a few number of sites(In reply to comment #54) > I like to make a histogram dumper to find out what the numbers are on actual web pages in a case like this. A few websites, but nothing fancy (facebook.com, gmail.com, apple.com, google.com): 1 |1722 (30.94%) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 |496 (8.91%) ++++++++++++++++++ 5 |489 (8.79%) ++++++++++++++++++ 6 |387 (6.95%) ++++++++++++++ 7 |334 (6.00%) ++++++++++++ 8 |278 (4.99%) ++++++++++ 9 |242 (4.35%) +++++++++ 3 |223 (4.01%) ++++++++ 10 |171 (3.07%) +++++++ 2 |144 (2.59%) ++++++ 18 |101 (1.81%) ++++ 12 |94 (1.69%) ++++ 16 |90 (1.62%) ++++ 13 |75 (1.35%) +++ 15 |73 (1.31%) +++ I would suggesting having an inline capacity of 8, and reserving the length or m_run in the constructor. That should avoid resizes and keep all the common small text-runs inline.
Ryosuke Niwa
Comment 56 2013-08-23 09:59:28 PDT
Comment on attachment 203806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > Source/WebCore/platform/graphics/WidthIterator.h:85 > + Vector<int> m_characterIndex; This variable name looks too generic. characterIndex for what?
Allan Sandfeld Jensen
Comment 57 2013-08-23 10:21:41 PDT
(In reply to comment #56) > (From update of attachment 203806 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > > > Source/WebCore/platform/graphics/WidthIterator.h:85 > > + Vector<int> m_characterIndex; > > This variable name looks too generic. characterIndex for what? Of glyph. It maps glyphs back to their position in the original text.
Ryosuke Niwa
Comment 58 2013-08-23 10:31:00 PDT
(In reply to comment #57) > (In reply to comment #56) > > (From update of attachment 203806 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=203806&action=review > > > > > Source/WebCore/platform/graphics/WidthIterator.h:85 > > > + Vector<int> m_characterIndex; > > > > This variable name looks too generic. characterIndex for what? > > Of glyph. It maps glyphs back to their position in the original text. m_characterIndexOfGlyph or m_glyphCharacterIndexMap would be a better name then.
Allan Sandfeld Jensen
Comment 59 2013-08-26 04:25:30 PDT
Reopening for improving naming and some adding some micro-optimization to allocation.
Allan Sandfeld Jensen
Comment 60 2013-08-26 04:35:53 PDT
Darin Adler
Comment 61 2013-08-26 10:17:07 PDT
Comment on attachment 209638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209638&action=review > Source/WebCore/platform/graphics/WidthIterator.cpp:71 > + m_characterIndexOfGlyph.reserveCapacity(m_run.length()); Should call reserveInitialCapacity, which is more efficient than reserveCapacity but can only be called on a newly constructed vector. > Source/WebCore/platform/graphics/WidthIterator.h:85 > + Vector<int, 10> m_characterIndexOfGlyph; Should have a comment somewhere indicating where the 10 came from.
Allan Sandfeld Jensen
Comment 62 2013-08-27 03:21:22 PDT
Enrica Casucci
Comment 63 2014-02-06 16:33:23 PST
This change has broken selection and caret movement in text that contains ligatured characters. The fast path does not handle correctly computing the offset in a line box in a partial run.
Allan Sandfeld Jensen
Comment 64 2014-02-07 00:36:12 PST
(In reply to comment #63) > This change has broken selection and caret movement in text that contains ligatured characters. > The fast path does not handle correctly computing the offset in a line box in a partial run. Do you have a test-case? There should be nothing stopping the fast path from supporting this, but there might be bugs in some cases.
Enrica Casucci
Comment 65 2014-02-07 10:19:52 PST
(In reply to comment #64) > (In reply to comment #63) > > This change has broken selection and caret movement in text that contains ligatured characters. > > The fast path does not handle correctly computing the offset in a line box in a partial run. > > Do you have a test-case? There should be nothing stopping the fast path from supporting this, but there might be bugs in some cases. I'm attaching a test case. Try to place the selection between the two 'f' in the word 'off'. You can't. If you place the selection after the word 'off' and move with the arrow key to the left, the caret shows up between 'o' and 'f'. What is really bad is that the selection in the DOM is correct (that is why your test did not catch it), but the caret is painted in the wrong position. The code you wrote to compute the selected text rect will never work when there is a ligature, because one of the advances is 0.
Enrica Casucci
Comment 66 2014-02-07 10:20:29 PST
Created attachment 223466 [details] Test that shows failing selection.
Enrica Casucci
Comment 67 2014-02-07 10:29:06 PST
One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. I'd like to roll this out, since it creates serious correctness issues. In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough.
Allan Sandfeld Jensen
Comment 68 2014-02-07 10:45:33 PST
(In reply to comment #67) > One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. > I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. > I'd like to roll this out, since it creates serious correctness issues. > In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough. That was not possible at the time. Qt did not enable ligatures in the fast-font path, and Mac only used the fast-font path for kerning and ligatures during printing, so no caret. I assume the Mac setting has been changed in the mean time causing the issue.
Allan Sandfeld Jensen
Comment 69 2014-02-07 10:50:19 PST
(In reply to comment #67) > One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. > I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. > I'd like to roll this out, since it creates serious correctness issues. > In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough. That was not possible at the time. Qt did not enable ligatures in the fast-font path, and Mac only used the fast-font path for kerning and ligatures during printing, so no caret. I assume the Mac setting has been changed in the mean time causing the issue. (In reply to comment #68) > (In reply to comment #67) > > One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. > > I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. > > I'd like to roll this out, since it creates serious correctness issues. > > In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough. > > That was not possible at the time. Qt did not enable ligatures in the fast-font path, and Mac only used the fast-font path for kerning and ligatures during printing, so no caret. > > I assume the Mac setting has been changed in the mean time causing the issue. Btw, how exactly to you expect to be able to separate ligatures in general? The code only supports selecting glyphs at a time. How do you select half of æ, œ, &, ß or W?
mitz
Comment 70 2014-02-07 10:53:14 PST
(In reply to comment #68) > (In reply to comment #67) > > One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. > > I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. > > I'd like to roll this out, since it creates serious correctness issues. > > In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough. > > That was not possible at the time. Qt did not enable ligatures in the fast-font path, and Mac only used the fast-font path for kerning and ligatures during printing, so no caret. That is completely wrong. > > I assume the Mac setting has been changed in the mean time causing the issue. That is an incorrect assumption. This bug’s title states that the fast code path doesn’t handle partial runs correctly when kerning *or ligatures* are enabled. You seem to have fixed a the kerning part, and proceeded to proclaim it fixed. That was your mistake.
mitz
Comment 71 2014-02-07 10:54:48 PST
(In reply to comment #69) > (In reply to comment #67) > Btw, how exactly to you expect to be able to separate ligatures in general? The code only supports selecting glyphs at a time. That is incorrect. The Core Text-based implementation of the complex path supports selecting partial glyphs.
Ryosuke Niwa
Comment 72 2014-02-07 16:33:21 PST
Allan Sandfeld Jensen
Comment 73 2014-02-09 03:29:55 PST
(In reply to comment #70) > > I assume the Mac setting has been changed in the mean time causing the issue. > > That is an incorrect assumption. > > This bug’s title states that the fast code path doesn’t handle partial runs correctly when kerning *or ligatures* are enabled. You seem to have fixed a the kerning part, and proceeded to proclaim it fixed. That was your mistake. Then change it so it is enabled when kerning is enabled. Btw, a revert will break SVG fonts. Partial selection of ligatures are NOT generally solvable. CoreText might work for simple ligatures, fi fl and friends, but it is not generally solvable for obvious reasons.
Allan Sandfeld Jensen
Comment 74 2014-02-09 03:38:42 PST
(In reply to comment #70) > (In reply to comment #68) > > (In reply to comment #67) > > > One more thing: I don't really understand how you implementation of Font::selectionRectForSimpleText is any different from the original one and how it is supposed to deal with ligatures correctly. > > > I'm really sorry I did not see this patch when you posted it because I would have given you the feedback immediately. > > > I'd like to roll this out, since it creates serious correctness issues. > > > In the future, if you make changes to code that affects how the caret position is computed, I suggest you write tests that look at the caret rect. Testing the correctness of the DOM selection is not enough. > > > > That was not possible at the time. Qt did not enable ligatures in the fast-font path, and Mac only used the fast-font path for kerning and ligatures during printing, so no caret. > > That is completely wrong. > Whether the fast-font path is can be used or not for specific typesetting features is controlled by WidthIterator::supportsTypesettingFeatures(). The code still disables it for all but printerFonts. Does that mean printerFonts does not relate to printing? The term is a bit vague and not documented, so I might have misinterpreted it.
Alexey Proskuryakov
Comment 75 2014-02-10 14:56:26 PST
See also: bug 119747.
Myles C. Maxfield
Comment 76 2014-11-03 21:10:45 PST
Is there a reason this never got landed?
Myles C. Maxfield
Comment 77 2014-11-03 21:11:43 PST
Whoops, disregard my last comment
Myles C. Maxfield
Comment 78 2022-06-23 12:20:25 PDT
The test committed in the original patch (https://github.com/WebKit/WebKit/commit/7979e0725249f604122d6e4ab361b409717665f5) passes now on ToT, and the "Test that shows failing selection." also passes on ToT, and the last comment bug is 9 years old, so I think we should just close this one.
Tim Horton
Comment 79 2023-03-17 17:19:56 PDT
Not actually fixed; the bug is resolved because we fall back to the complex path, but the bug itself was about the fast path not handling partial runs, and that remains true (and links to this bug!)
Note You need to log in before you can comment on or make changes to this bug.