New issue
Advanced search Search tips

Issue 796943 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 6122



Sign in to add a comment

[LayoutNG] Shape across element boundaries do not work for ligatures

Project Member Reported by kojii@chromium.org, Dec 21 2017

Issue description

In LayoutNG, it works for Arabic shaping, but not for ligatures.

drott@, any ideas where to look at?
 
shape-across-elements.png
5.1 KB View Download

Comment 1 by kojii@chromium.org, Dec 21 2017

Status: Available (was: Untriaged)
and kerning too. This is causing layout diff in several tests when there's element boundaries, as we apply kerning better (for spaces) in NG.

Comment 2 by kojii@chromium.org, Dec 21 2017

ex.
fast/text/word-space.html

<div>A B</div>
<div><span>A</span> B</div>

shape to different widths.

Comment 3 by kojii@chromium.org, Dec 22 2017

Cc: behdad@chromium.org
Gecko shows both shaping and ligatures correctly, so we're doing something wrong. Learning how we call hb...

Comment 4 by kojii@chromium.org, Dec 22 2017

Gecko:
shape-across-elements-gecko.png
5.8 KB View Download

Comment 5 by kojii@chromium.org, Dec 22 2017

Found some docs for pre-context in hb_buffer_add_utf, pointing to:
https://bugzilla.mozilla.org/show_bug.cgi?id=801410#c13

So we probably need to do something to use pre-context properly. Haven't figured out about post-context yet, nor if it's needed.

Comment 6 by kojii@chromium.org, Dec 22 2017

Updated test and screenshot, Blink, LayoutNG, and Gecko from left.

Some interesting observations:
* Kerning across elements doesn't work (compare the width of two "AVA").
* Ligature needs some new way in NGInlineItem because two items need to share a glyph.
* Arabic joining somehow works.

While this still looks improvements for Arabic and no changes for kerning/ligatures, the total width of NG with kerning are slightly worse then legacy in some tests. Need to look a bit more if the change is acceptable.
shape-across-elements.png
33.3 KB View Download

Comment 7 by kojii@chromium.org, Dec 27 2017

Blockedon: 6122
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7acc1db96a365e47925f46b5811451ec67e17ad8

commit 7acc1db96a365e47925f46b5811451ec67e17ad8
Author: Koji Ishii <kojii@chromium.org>
Date: Mon Jan 08 00:40:54 2018

Fix ShapeResult::PositionForOffset when the first glyph is missing

This patch fixes reading out-of-bounds of Vector in
ShapeResult::PositionForOffset when the first glyph is missing
in RTL ShapeResult.

The test relies on SubRange/CopyRange does not copy glyphs when
offset is mid of a ligature, which maybe changed in future, but
the out-of-bounds could happen when missing glyph occurs.

Bug: 796943
Change-Id: I951f838dd60986a031e9c989ba3eb1a23a7eeba4
Reviewed-on: https://chromium-review.googlesource.com/853592
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527550}
[modify] https://crrev.com/7acc1db96a365e47925f46b5811451ec67e17ad8/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
[modify] https://crrev.com/7acc1db96a365e47925f46b5811451ec67e17ad8/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5489f09ec47182268fe7267450318f7bce5858cd

commit 5489f09ec47182268fe7267450318f7bce5858cd
Author: Koji Ishii <kojii@chromium.org>
Date: Mon Jan 08 14:00:30 2018

Disable kerning for tests that rely on undefined kerning behavior

This patch disables kerning for tests that assumes how kerning
work across element or node boundaries.

These tests fail when Blink tried to change the kerning logic in
LayoutNG.

Bug: 796943
Change-Id: I285b7e3b895af2ad5abf518c74e65d24b38ecb88
Reviewed-on: https://chromium-review.googlesource.com/853552
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527628}
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-before-after-001.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-before-after-002.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-before-after-003.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-button.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-fieldset.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-pass-ref.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-td-001.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/external/wpt/css/css-display/display-contents-unusual-html-elements-none.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/fast/block/block-image-becomes-float-expected.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/fast/block/block-image-becomes-float.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/fast/inline/do-not-detach-whitespace-with-out-of-flow-siblings-inside-inline-parent-expected.html
[modify] https://crrev.com/5489f09ec47182268fe7267450318f7bce5858cd/third_party/WebKit/LayoutTests/fast/inline/do-not-detach-whitespace-with-out-of-flow-siblings-inside-inline-parent.html

Comment 10 by behdad@google.com, Jan 8 2018

The pre-context is irrelevant. We already do that fine.  The difference is that Firefox ignores color and some other changes and shapes the text as one hb_shape() call, and then colors some parts differently.  That's why you see the cap of the second 'f' in the 'ffi' ligature also colored with the 'i'.  We don't do that. We break the text when color changes.
Thank you for the comment, understood. I had an impression that giving pre-context will give us the same result as one hb_shape() call, but thinking more, it's not possible at least for ligatures.

I'm getting close to switching to one hb_shape() call in LayoutNG.
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c

commit b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c
Author: Koji Ishii <kojii@chromium.org>
Date: Wed Jan 10 07:08:38 2018

[LayoutNG] Shape across element boundaries

This patch shapes multiple NGInlineItem together when possible,
and split the ShapeResult to corresponding items to enable
features such as kerning and ligature across element boundaries.

LayoutNG already applies kerning across spaces. Applying across
element boundaries improves typography in more cases.

For ligatures, Gecko distributes one glyph to multiple elements
by using the number of code units or code points in each element,
while Edge puts the glyph to the element its first code unit
belongs to. This patch takes Edge's approach. Both have pros and
cons, but Edge's approach is much simpler to start with.

TESTS:
- fast/text/international/shape-across-elements-simple.html
  is a new test that tests shaping across elements without testing
  which element a ligature should be placed.
- Several rebaselines are made, for improved typography.

Bug: 796943, 636993
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Iab6881da5bbc1ddb1e8fa39a53b72c36d405f4a5
Reviewed-on: https://chromium-review.googlesource.com/852015
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528256}
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/invalid/residual-style-expected.html
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/invalid/residual-style.html
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/text/international/shape-across-elements-simple-expected.html
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/text/international/shape-across-elements-simple.html
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/text/unmatched-surrogate-pair-expected.html
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/fast/text/unmatched-surrogate-pair.html
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css1/text_properties/text_transform-expected.png
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t1602-c546-txt-align-00-b-expected.png
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t1602-c546-txt-align-00-b-expected.txt
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/inline/drawStyledEmptyInlines-expected.png
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/atomic-inline-before-ellipsis-expected.png
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/font-features/caps-native-synthesis-expected.png
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/tables/mozilla/bugs/bug8411-expected.png
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/platform/linux/virtual/layout_ng/fast/block/float/016-expected.png
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/platform/mac/virtual/layout_ng/fast/block/float/016-expected.png
[add] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/LayoutTests/platform/win/virtual/layout_ng/fast/block/float/016-expected.png
[modify] https://crrev.com/b82e6262b2c80a8c42f6a91adc9947f6a9f8d79c/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_node.cc

Comment 13 by behdad@google.com, Jan 13 2018

> For ligatures, Gecko distributes one glyph to multiple elements
by using the number of code units or code points in each element

It's not number of codepoints. It's number of graphemes.  We have similar code for other things, like cursor positions.
Components: Blink>Layout

Comment 15 by e...@chromium.org, Mar 5 2018

Components: -Blink>Layout Blink>Fonts

Sign in to add a comment