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.
See also bug 100068.
Created attachment 199667 [details] Patch
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.
(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.
Created attachment 199674 [details] Patch Second approach, always run the WidthIterator on complete TextRuns
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
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
Comment on attachment 199674 [details] Patch Has issue with offset calculation that needs to be investigated.
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
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
Created attachment 199822 [details] Patch Fixed offsetForPosition and removed the separate iteration to calculate glyphFrom and glyphTo
Comment on attachment 199822 [details] Patch Attachment 199822 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/38682
Comment on attachment 199822 [details] Patch Attachment 199822 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/43839
Comment on attachment 199822 [details] Patch Attachment 199822 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/148527
Comment on attachment 199822 [details] Patch Attachment 199822 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/35841
Created attachment 199831 [details] Patch Avoid comparisons and convertions between int and unsigned int
Created attachment 199832 [details] Patch
<rdar://problem/13458289>
<rdar://problem/13772881>
Any comments or reviews? Also are there anything in the rdar problems I should know about?
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?
Created attachment 201550 [details] Patch cleaned-up following review comment
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
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
Created attachment 201565 [details] Patch
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
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
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
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
The assertions are not reproducable in the Qt port.
Created attachment 201690 [details] Patch
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.
(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.
(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.
Created attachment 201713 [details] Patch
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.
(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?
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.
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.
(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.
Created attachment 201810 [details] Patch
The latest patch was merged into the Qt 5.1 branch in order to make it for Qt 5.1 RC1
Created attachment 203806 [details] Patch Now with a proper automatic reference test
darin, mitz: any thoughts on the last patch?
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.
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.
(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.
(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?
Comment on attachment 203806 [details] Patch Ok, looks like a good simplification then. r=me
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.
Committed r154384: <http://trac.webkit.org/changeset/154384>
What’s the typical size of m_characterIndex? Should it have some inline capacity to avoid memory allocation?
(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.
I like to make a histogram dumper to find out what the numbers are on actual web pages in a case like this.
(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.
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?
(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.
(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.
Reopening for improving naming and some adding some micro-optimization to allocation.
Created attachment 209638 [details] Patch
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.
Committed r154674: <http://trac.webkit.org/changeset/154674>
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.
(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.
(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.
Created attachment 223466 [details] Test that shows failing selection.
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.
(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 #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?
(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.
(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.
Reverted the patches in http://trac.webkit.org/changeset/163655.
(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.
(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.
See also: bug 119747.
Is there a reason this never got landed?
Whoops, disregard my last comment
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.
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!)