Bug 100050 - Font's fast code path doesn't handle partial runs correctly when kerning or ligatures are enabled
Summary: Font's fast code path doesn't handle partial runs correctly when kerning or l...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords: InRadar
Depends on:
Blocks: 110211 117540
  Show dependency treegraph
 
Reported: 2012-10-22 16:30 PDT by mitz
Modified: 2017-09-28 05:42 PDT (History)
18 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2013-04-25 08:51 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (11.14 KB, patch)
2013-04-25 09:45 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
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 Details
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 Details
Patch (12.42 KB, patch)
2013-04-26 06:11 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (12.45 KB, patch)
2013-04-26 07:15 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2013-04-26 07:21 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (12.74 KB, patch)
2013-05-13 05:16 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
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 Details
Patch (12.78 KB, patch)
2013-05-13 07:24 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
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 Details
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 Details
Patch (13.35 KB, patch)
2013-05-14 02:19 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (15.43 KB, patch)
2013-05-14 07:53 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (39.84 KB, patch)
2013-05-15 02:52 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (41.68 KB, patch)
2013-06-05 05:00 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.85 KB, patch)
2013-08-26 04:35 PDT, Allan Sandfeld Jensen
darin: review+
Details | Formatted Diff | Diff
Test that shows failing selection. (133 bytes, text/html)
2014-02-07 10:20 PST, Enrica Casucci
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 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.
Comment 1 mitz 2012-10-22 20:18:06 PDT
See also bug 100068.
Comment 2 Allan Sandfeld Jensen 2013-04-25 08:51:31 PDT
Created attachment 199667 [details]
Patch
Comment 3 mitz 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.
Comment 4 Allan Sandfeld Jensen 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.
Comment 5 Allan Sandfeld Jensen 2013-04-25 09:45:06 PDT
Created attachment 199674 [details]
Patch

Second approach, always run the WidthIterator on complete TextRuns
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Allan Sandfeld Jensen 2013-04-25 12:02:38 PDT
Comment on attachment 199674 [details]
Patch

Has issue with offset calculation that needs to be investigated.
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Allan Sandfeld Jensen 2013-04-26 06:11:30 PDT
Created attachment 199822 [details]
Patch

Fixed offsetForPosition and removed the separate iteration to calculate glyphFrom and glyphTo
Comment 12 EFL EWS Bot 2013-04-26 06:23:11 PDT
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 13 EFL EWS Bot 2013-04-26 06:32:34 PDT
Comment on attachment 199822 [details]
Patch

Attachment 199822 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/43839
Comment 14 Build Bot 2013-04-26 06:41:33 PDT
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 15 Build Bot 2013-04-26 06:53:09 PDT
Comment on attachment 199822 [details]
Patch

Attachment 199822 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/35841
Comment 16 Allan Sandfeld Jensen 2013-04-26 07:15:59 PDT
Created attachment 199831 [details]
Patch

Avoid comparisons and convertions between int and unsigned int
Comment 17 Allan Sandfeld Jensen 2013-04-26 07:21:50 PDT
Created attachment 199832 [details]
Patch
Comment 18 Antoine Quint 2013-04-30 07:27:50 PDT
<rdar://problem/13458289>
Comment 19 Radar WebKit Bug Importer 2013-04-30 08:37:55 PDT
<rdar://problem/13772881>
Comment 20 Allan Sandfeld Jensen 2013-05-07 03:43:29 PDT
Any comments or reviews? 

Also are there anything in the rdar problems I should know about?
Comment 21 Jocelyn Turcotte 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?
Comment 22 Allan Sandfeld Jensen 2013-05-13 05:16:21 PDT
Created attachment 201550 [details]
Patch

cleaned-up following review comment
Comment 23 Build Bot 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
Comment 24 Build Bot 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
Comment 25 Allan Sandfeld Jensen 2013-05-13 07:24:06 PDT
Created attachment 201565 [details]
Patch
Comment 26 Build Bot 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
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Build Bot 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
Comment 30 Allan Sandfeld Jensen 2013-05-14 02:10:29 PDT
The assertions are not reproducable in the Qt port.
Comment 31 Allan Sandfeld Jensen 2013-05-14 02:19:58 PDT
Created attachment 201690 [details]
Patch
Comment 32 Jocelyn Turcotte 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.
Comment 33 Allan Sandfeld Jensen 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.
Comment 34 Jocelyn Turcotte 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.
Comment 35 Allan Sandfeld Jensen 2013-05-14 07:53:27 PDT
Created attachment 201713 [details]
Patch
Comment 36 Jocelyn Turcotte 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.
Comment 37 Allan Sandfeld Jensen 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?
Comment 38 Darin Adler 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.
Comment 39 Darin Adler 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.
Comment 40 Allan Sandfeld Jensen 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.
Comment 41 Allan Sandfeld Jensen 2013-05-15 02:52:00 PDT
Created attachment 201810 [details]
Patch
Comment 42 Allan Sandfeld Jensen 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
Comment 43 Allan Sandfeld Jensen 2013-06-05 05:00:23 PDT
Created attachment 203806 [details]
Patch

Now with a proper automatic reference test
Comment 44 Allan Sandfeld Jensen 2013-06-07 08:24:24 PDT
darin, mitz: any thoughts on the last patch?
Comment 45 Allan Sandfeld Jensen 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.
Comment 46 Antti Koivisto 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.
Comment 47 Allan Sandfeld Jensen 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.
Comment 48 Allan Sandfeld Jensen 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?
Comment 49 Antti Koivisto 2013-08-21 05:39:36 PDT
Comment on attachment 203806 [details]
Patch

Ok, looks like a good simplification then. r=me
Comment 50 Antti Koivisto 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.
Comment 51 Allan Sandfeld Jensen 2013-08-21 06:42:00 PDT
Committed r154384: <http://trac.webkit.org/changeset/154384>
Comment 52 Darin Adler 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?
Comment 53 Allan Sandfeld Jensen 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.
Comment 54 Darin Adler 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.
Comment 55 Allan Sandfeld Jensen 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.
Comment 56 Ryosuke Niwa 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?
Comment 57 Allan Sandfeld Jensen 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.
Comment 58 Ryosuke Niwa 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.
Comment 59 Allan Sandfeld Jensen 2013-08-26 04:25:30 PDT
Reopening for improving naming and some adding some micro-optimization to allocation.
Comment 60 Allan Sandfeld Jensen 2013-08-26 04:35:53 PDT
Created attachment 209638 [details]
Patch
Comment 61 Darin Adler 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.
Comment 62 Allan Sandfeld Jensen 2013-08-27 03:21:22 PDT
Committed r154674: <http://trac.webkit.org/changeset/154674>
Comment 63 Enrica Casucci 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.
Comment 64 Allan Sandfeld Jensen 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.
Comment 65 Enrica Casucci 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.
Comment 66 Enrica Casucci 2014-02-07 10:20:29 PST
Created attachment 223466 [details]
Test that shows failing selection.
Comment 67 Enrica Casucci 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.
Comment 68 Allan Sandfeld Jensen 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.
Comment 69 Allan Sandfeld Jensen 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?
Comment 70 mitz 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.
Comment 71 mitz 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.
Comment 72 Ryosuke Niwa 2014-02-07 16:33:21 PST
Reverted the patches in http://trac.webkit.org/changeset/163655.
Comment 73 Allan Sandfeld Jensen 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.
Comment 74 Allan Sandfeld Jensen 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.
Comment 75 Alexey Proskuryakov 2014-02-10 14:56:26 PST
See also: bug 119747.
Comment 76 Myles C. Maxfield 2014-11-03 21:10:45 PST
Is there a reason this never got landed?
Comment 77 Myles C. Maxfield 2014-11-03 21:11:43 PST
Whoops, disregard my last comment