Bug 101009

Created attachment 172255 [details] Patch

Kent-san, Could you take a look? This fix is based on HarfBuzz developer's suggestion. https://bugzilla.mozilla.org/show_bug.cgi?id=801410#c12

Note: chromium r165892 is required to pass the new test. http://src.chromium.org/viewvc/chrome?view=rev&revision=165892

Comment on attachment 172255 [details] Patch rubber-stamped

Comment on attachment 172255 [details] Patch Attachment 172255 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14721672 New failing tests: fast/dom/Document/createElementNS-namespace-err.html editing/deleting/skip-virama-001.html http/tests/incremental/frame-focus-before-load.html fast/css/text-rendering.html fast/dom/DOMImplementation/createDocument-namespace-err.html editing/selection/extend-selection-home-end.html editing/text-iterator/thai-cursor-movement.html editing/selection/extend-backward-by-word-over-non-editable.html editing/selection/regional-indicators.html editing/inserting/insert-thai-characters-001.html editing/selection/extend-forward-by-word-over-non-editable.html editing/selection/51344.html fast/css/case-transform.html editing/deleting/delete-ligature-001.html editing/deleting/delete-ligature-003.html editing/style/make-text-writing-direction-inline.html editing/selection/home-end.html editing/selection/css-pseudo-element.html fast/dom/Document/createAttributeNS-namespace-err.html editing/deleting/delete-ligature-002.html editing/selection/extend-to-line-boundary.html editing/selection/thai-word-at-document-end.html editing/text-iterator/rtl-first-letter-text-iterator-crash.html editing/selection/move-by-word-visually-single-space-one-element.html fast/dom/Element/setAttributeNS-namespace-err.html editing/selection/select-line-break-with-opposite-directionality.html fast/dom/Document/createElement-invalid-names.html http/tests/incremental/slow-utf8-text.pl css3/flexbox/inline-flex-crash.html fast/dom/Document/createElement-valid-names.html

Created attachment 172453 [details] Patch for landing

Comment on attachment 172453 [details] Patch for landing Clearing flags on attachment: 172453 Committed r133550: <http://trac.webkit.org/changeset/133550>

All reviewed patches have been landed. Closing bug.

Seems this patch breaks EFL debug layout test. Would you please take a look at this? 20:36:46.556 1146 worker/7 css3/flexbox/inline-flex-crash.html crashed, (stderr lines): 20:36:46.556 1146 ASSERTION FAILED: i < size() 20:36:46.556 1146 /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WTF/wtf/Vector.h(550) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = short unsigned int, long unsigned int inlineCapacity = 256ul, size_t = long unsigned int] 20:36:46.556 1146 1 0x7ffa1eba5392 WTF::Vector<unsigned short, 256ul>::at(unsigned long) 20:36:46.556 1146 2 0x7ffa1eba49cd WTF::Vector<unsigned short, 256ul>::operator[](unsigned long) 20:36:46.556 1146 3 0x7ffa1eba492d WebCore::HarfBuzzShaper::HarfBuzzRun::glyphToCharacterIndexes() 20:36:46.556 1146 4 0x7ffa1eba38c9 WebCore::HarfBuzzShaper::setGlyphPositionsForHarfBuzzRun(WebCore::HarfBuzzShaper::HarfBuzzRun*, hb_buffer_t*) 20:36:46.556 1146 5 0x7ffa1eba377e WebCore::HarfBuzzShaper::shapeHarfBuzzRuns() 20:36:46.556 1146 6 0x7ffa1eba2d1e WebCore::HarfBuzzShaper::shape(WebCore::GlyphBuffer*) 20:36:46.556 1146 7 0x7ffa1eb99aff WebCore::Font::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const 20:36:46.556 1146 8 0x7ffa1e1b1643 WebCore::Font::width(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const 20:36:46.556 1146 9 0x7ffa1e4323ad WebCore::RenderText::widthFromCache(WebCore::Font const&, int, int, float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const 20:36:46.556 1146 10 0x7ffa1e42e7bc WebCore::RenderText::computePreferredLogicalWidths(float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&) 20:36:46.556 1146 11 0x7ffa1e42dafd WebCore::RenderText::computePreferredLogicalWidths(float) 20:36:46.556 1146 12 0x7ffa1e42d53e WebCore::RenderText::trimmedPrefWidths(float, float&, bool&, float&, bool&, bool&, bool&, float&, float&, float&, float&, bool&) 20:36:46.556 1146 13 0x7ffa1e2caae4 WebCore::RenderBlock::computeInlinePreferredLogicalWidths() 20:36:46.556 1146 14 0x7ffa1e2c926d WebCore::RenderBlock::computePreferredLogicalWidths() 20:36:46.556 1146 15 0x7ffa1e314e03 WebCore::RenderBox::minPreferredLogicalWidth() const 20:36:46.556 1146 16 0x7ffa1e36111c WebCore::RenderFlexibleBox::computePreferredLogicalWidths() 20:36:46.556 1146 17 0x7ffa1e314e59 WebCore::RenderBox::maxPreferredLogicalWidth() const 20:36:46.557 1146 18 0x7ffa1e31b3d9 WebCore::RenderBox::computeLogicalWidthInRegionUsing(WebCore::SizeType, WebCore::FractionalLayoutUnit, WebCore::RenderBlock const*, WebCore::RenderRegion*, WebCore::FractionalLayoutUnit) const 20:36:46.557 1146 19 0x7ffa1e31a905 WebCore::RenderBox::computeLogicalWidthInRegion(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderRegion*, WebCore::FractionalLayoutUnit) const 20:36:46.557 1146 20 0x7ffa1e31a0c7 WebCore::RenderBox::updateLogicalWidth() 20:36:46.557 1146 21 0x7ffa1e362119 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::FractionalLayoutUnit) 20:36:46.557 1146 22 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() 20:36:46.557 1146 23 0x7ffa1e282e77 WebCore::RenderObject::layoutIfNeeded() 20:36:46.557 1146 24 0x7ffa1e2fe567 WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) 20:36:46.557 1146 25 0x7ffa1e2af043 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) 20:36:46.557 1146 26 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() 20:36:46.557 1146 27 0x7ffa1e2b4279 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) 20:36:46.557 1146 28 0x7ffa1e2b3dd4 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) 20:36:46.557 1146 29 0x7ffa1e2af064 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) 20:36:46.557 1146 30 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() 20:36:46.557 1146 31 0x7ffa1e2b4279 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&)

Which version of harfbuzz-ng does EFL port use? To fix the issue, we need harfbuzz-ng version: http://cgit.freedesktop.org/harfbuzz/commit/?id=da70111ab234e8b740ce6fb1789a1809fbec0c44 FYI, here is a chromium bug report. http://code.google.com/p/chromium/issues/detail?id=158978#c1 (In reply to comment #9) > Seems this patch breaks EFL debug layout test. > Would you please take a look at this? > > > 20:36:46.556 1146 worker/7 css3/flexbox/inline-flex-crash.html crashed, (stderr lines): > 20:36:46.556 1146 ASSERTION FAILED: i < size() > 20:36:46.556 1146 /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WTF/wtf/Vector.h(550) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = short unsigned int, long unsigned int inlineCapacity = 256ul, size_t = long unsigned int] > 20:36:46.556 1146 1 0x7ffa1eba5392 WTF::Vector<unsigned short, 256ul>::at(unsigned long) > 20:36:46.556 1146 2 0x7ffa1eba49cd WTF::Vector<unsigned short, 256ul>::operator[](unsigned long) > 20:36:46.556 1146 3 0x7ffa1eba492d WebCore::HarfBuzzShaper::HarfBuzzRun::glyphToCharacterIndexes() > 20:36:46.556 1146 4 0x7ffa1eba38c9 WebCore::HarfBuzzShaper::setGlyphPositionsForHarfBuzzRun(WebCore::HarfBuzzShaper::HarfBuzzRun*, hb_buffer_t*) > 20:36:46.556 1146 5 0x7ffa1eba377e WebCore::HarfBuzzShaper::shapeHarfBuzzRuns() > 20:36:46.556 1146 6 0x7ffa1eba2d1e WebCore::HarfBuzzShaper::shape(WebCore::GlyphBuffer*) > 20:36:46.556 1146 7 0x7ffa1eb99aff WebCore::Font::floatWidthForComplexText(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const > 20:36:46.556 1146 8 0x7ffa1e1b1643 WebCore::Font::width(WebCore::TextRun const&, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const > 20:36:46.556 1146 9 0x7ffa1e4323ad WebCore::RenderText::widthFromCache(WebCore::Font const&, int, int, float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >*, WebCore::GlyphOverflow*) const > 20:36:46.556 1146 10 0x7ffa1e42e7bc WebCore::RenderText::computePreferredLogicalWidths(float, WTF::HashSet<WebCore::SimpleFontData const*, WTF::PtrHash<WebCore::SimpleFontData const*>, WTF::HashTraits<WebCore::SimpleFontData const*> >&, WebCore::GlyphOverflow&) > 20:36:46.556 1146 11 0x7ffa1e42dafd WebCore::RenderText::computePreferredLogicalWidths(float) > 20:36:46.556 1146 12 0x7ffa1e42d53e WebCore::RenderText::trimmedPrefWidths(float, float&, bool&, float&, bool&, bool&, bool&, float&, float&, float&, float&, bool&) > 20:36:46.556 1146 13 0x7ffa1e2caae4 WebCore::RenderBlock::computeInlinePreferredLogicalWidths() > 20:36:46.556 1146 14 0x7ffa1e2c926d WebCore::RenderBlock::computePreferredLogicalWidths() > 20:36:46.556 1146 15 0x7ffa1e314e03 WebCore::RenderBox::minPreferredLogicalWidth() const > 20:36:46.556 1146 16 0x7ffa1e36111c WebCore::RenderFlexibleBox::computePreferredLogicalWidths() > 20:36:46.556 1146 17 0x7ffa1e314e59 WebCore::RenderBox::maxPreferredLogicalWidth() const > 20:36:46.557 1146 18 0x7ffa1e31b3d9 WebCore::RenderBox::computeLogicalWidthInRegionUsing(WebCore::SizeType, WebCore::FractionalLayoutUnit, WebCore::RenderBlock const*, WebCore::RenderRegion*, WebCore::FractionalLayoutUnit) const > 20:36:46.557 1146 19 0x7ffa1e31a905 WebCore::RenderBox::computeLogicalWidthInRegion(WebCore::RenderBox::LogicalExtentComputedValues&, WebCore::RenderRegion*, WebCore::FractionalLayoutUnit) const > 20:36:46.557 1146 20 0x7ffa1e31a0c7 WebCore::RenderBox::updateLogicalWidth() > 20:36:46.557 1146 21 0x7ffa1e362119 WebCore::RenderFlexibleBox::layoutBlock(bool, WebCore::FractionalLayoutUnit) > 20:36:46.557 1146 22 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() > 20:36:46.557 1146 23 0x7ffa1e282e77 WebCore::RenderObject::layoutIfNeeded() > 20:36:46.557 1146 24 0x7ffa1e2fe567 WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) > 20:36:46.557 1146 25 0x7ffa1e2af043 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) > 20:36:46.557 1146 26 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() > 20:36:46.557 1146 27 0x7ffa1e2b4279 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) > 20:36:46.557 1146 28 0x7ffa1e2b3dd4 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) > 20:36:46.557 1146 29 0x7ffa1e2af064 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) > 20:36:46.557 1146 30 0x7ffa1e2ae5a0 WebCore::RenderBlock::layout() > 20:36:46.557 1146 31 0x7ffa1e2b4279 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&)

(In reply to comment #10) > Which version of harfbuzz-ng does EFL port use? EFL port is using 0.9.2 version. > To fix the issue, we need harfbuzz-ng version: > http://cgit.freedesktop.org/harfbuzz/commit/?id=da70111ab234e8b740ce6fb1789a1809fbec0c44 > Whoa, even latest tag doesn't cover this commit. :) > FYI, here is a chromium bug report. > http://code.google.com/p/chromium/issues/detail?id=158978#c1 > I see. I will discuss with EFL WebKittens for bumping harfbuzz and be back here. ;)

(In reply to comment #11) > Whoa, even latest tag doesn't cover this commit. :) Adding behdad@. > I will discuss with EFL WebKittens for bumping harfbuzz and be back here. ;) Thanks. Should I add #if PLATFORM(CHROMIUM) at this moment?

(In reply to comment #12) > Thanks. Should I add #if PLATFORM(CHROMIUM) at this moment? Sounds reasonable. Feel free to change so without review.

(In reply to comment #13) > (In reply to comment #12) > > Thanks. Should I add #if PLATFORM(CHROMIUM) at this moment? > > Sounds reasonable. Feel free to change so without review. Thanks!

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > Thanks. Should I add #if PLATFORM(CHROMIUM) at this moment? > > > > Sounds reasonable. Feel free to change so without review. > > Thanks! Done. http://trac.webkit.org/changeset/133563

(In reply to comment #15) > Done. > http://trac.webkit.org/changeset/133563 Thanks! EFL bots are fine now. :)

Bumping handled in bug 101323.

Shouldn't the new test force the complex text path using text-rendering:optimizelegibility? I wouldn't expect reference1 to automatically go to the complex path, so can we really assume that the widths would be the same?

(In reply to comment #18) > Shouldn't the new test force the complex text path using text-rendering:optimizelegibility? I wouldn't expect reference1 to automatically go to the complex path, so can we really assume that the widths would be the same? For this text, I think the widths should be the same regardless WebKit uses fast path or complex path.

I disagree. In the simple text path, -webkit-font-kerning:auto typically means not to kern, and in the complex text path it typically means to kern. Arial has a kerning pair for U+043A U+0430 that increases the width of the second line by 1 pixel. Instead of forcing the complex text path for the first line, I'd be ok with putting -webkit-font-kerning:none on both.

(In reply to comment #20) > I disagree. In the simple text path, -webkit-font-kerning:auto typically means not to kern, and in the complex text path it typically means to kern. > > Arial has a kerning pair for U+043A U+0430 that increases the width of the second line by 1 pixel. > > Instead of forcing the complex text path for the first line, I'd be ok with putting -webkit-font-kerning:none on both. Ok, preparing the fix.

Reopening to attach new patch.

Created attachment 174558 [details] Fix layout test

(In reply to comment #23) > Created an attachment (id=174558) [details] > Fix layout test Kent-san, could you rubber-stamp?

Comment on attachment 174558 [details] Fix layout test rubber stamped

Comment on attachment 174558 [details] Fix layout test Thanks!

Comment on attachment 174558 [details] Fix layout test Clearing flags on attachment: 174558 Committed r134870: <http://trac.webkit.org/changeset/134870>

All reviewed patches have been landed. Closing bug.

If your port renders the same characters with the same style differently depending on whether it takes the fast path or the complex path, then your port has a bug in the complex path.

(In reply to comment #29) > If your port renders the same characters with the same style differently depending on whether it takes the fast path or the complex path, then your port has a bug in the complex path. It looks to me like -webkit-font-kerning:auto doesn't force complex text and doesn't do any kerning in the simple text path on any port. Even something as simple as: <div>AV</div> <div style="text-rendering:optimizelegibility">AV</div> will show a width difference on any port that supports kerning (including Safari). Do you think this behaviour is wrong?

(In reply to comment #30) > (In reply to comment #29) > > If your port renders the same characters with the same style differently depending on whether it takes the fast path or the complex path, then your port has a bug in the complex path. > > It looks to me like -webkit-font-kerning:auto doesn't force complex text and doesn't do any kerning in the simple text path on any port. > > Even something as simple as: > <div>AV</div> > <div style="text-rendering:optimizelegibility">AV</div> > > will show a width difference on any port that supports kerning (including Safari). Do you think this behaviour is wrong? I said “If your port renders the same characters *with the same style* differently depending on whether it takes the fast path or the complex path, then your port has a bug in the complex path.” In your above example, the style is different, so it is irrelevant to my statement. Kerning affecting width is expected. Choice of code path (an implementation detail) affecting appearance is a bug. If a port doesn’t render the AV these two divs the same, then it has a bug: <div>AV e</div> <div>AV e&#x0300;</div>

> In your above example, the style is different, so it is irrelevant to my statement. Kerning affecting width is expected. Choice of code path (an implementation detail) affecting appearance is a bug. If a port doesn’t render the AV these two divs the same, then it has a bug: > <div>AV e</div> > <div>AV e&#x0300;</div> Thanks mitz, this is really helpful. The BlackBerry port was broken here, as is both Chrome and Safari on Windows and Chrome on Linux.

Ok, my apologies to Kenichi Ishibashi and Kent Tamura, that change shouldn't have been necessary (although the test still tests the desired thing, I think). I've opened https://bugs.webkit.org/show_bug.cgi?id=102603