Bug 105454 - vcl harfbuzz / common sal layout: unexpected no wrap of text
Summary: vcl harfbuzz / common sal layout: unexpected no wrap of text
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
4.1 all versions
Hardware: All Linux (All)
: medium normal
Assignee: Khaled Hosny
URL:
Whiteboard: target:5.4.0
Keywords: bibisected, bisected, regression
: 66818 102226 (view as bug list)
Depends on:
Blocks: Font-Rendering VCL-OpenGL
  Show dependency treegraph
 
Reported: 2017-01-20 15:49 UTC by Miklos Vajna
Modified: 2017-05-13 14:35 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Rerproducer document. (25.50 KB, application/msword)
2017-01-20 15:49 UTC, Miklos Vajna
Details
MSO screenshot. (39.03 KB, image/png)
2017-01-20 16:03 UTC, Miklos Vajna
Details
LO 4.0 screenshot. (39.86 KB, image/png)
2017-01-20 16:04 UTC, Miklos Vajna
Details
LO master (towards 5.4) screenshot. (51.05 KB, image/png)
2017-01-20 16:04 UTC, Miklos Vajna
Details
Kerning rerproducer document. (8.77 KB, application/vnd.oasis.opendocument.text)
2017-01-26 09:04 UTC, Miklos Vajna
Details
MS-DOC kerning reproducer (21.50 KB, application/msword)
2017-03-03 16:23 UTC, Miklos Vajna
Details
MS-DOC kerning reproducer in LO, before (17.45 KB, image/png)
2017-03-03 16:24 UTC, Miklos Vajna
Details
MS-DOC kerning reproducer in LO, after (17.45 KB, image/png)
2017-03-03 16:24 UTC, Miklos Vajna
Details
MS-DOC kerning reproducer in MSO (12.97 KB, image/png)
2017-03-03 16:26 UTC, Miklos Vajna
Details
MS-DOC kerning reproducer in LO, before, second try. (17.69 KB, image/png)
2017-03-06 12:01 UTC, Miklos Vajna
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Miklos Vajna 2017-01-20 15:49:21 UTC
Created attachment 130581 [details]
Rerproducer document.

Hi,

Steps to reproduce:

1) Open the attached document.

2) Check the layout of the text in the only cell of the table in the document.

Expected: the last word of the text is wrapped to a second line.

Actual: the last word of the text is still in the first line.

As far as I see LO 4.0 rendered this correctly, matching the MSO behavior, but LO 4.1 and newer versions started to render differently.
Comment 1 Miklos Vajna 2017-01-20 15:58:28 UTC
git bisect says:

- 9cb21a33421b8531dd25f76cc757f0d7f0fcc5ee is still good (2 lines)
- b8b0416620feecf3ede3305830a2b145c62a5bf9 is already bad (1 line)

It's not possible to bisect more between these two, as there is either no text at all, or soffice crashes on startup (so there are 8 commits in the good..bad range).

This is how I still bisected manually:

- check out fefd5116256e706ee1ab3d03e56cc0c2ff95305f, the last good commit
- cherry-pick 64fc186bb9cbb6af43d73149e9e593c48fbb0f22 "Remove unused variables": crash
- cherry-pick f0393d7ff69011a16b100541ef18e5090544e4a1 "[harfbuzz] Fix text width calculation, 3rd try": still good (2 lines)
- bff8fa97e16f0f06fddc5545ea36c8bd2b18a580 "Enable HarfBuzz by default": already bad (1 line)

So I think this is a regression from bff8fa97e16f0f06fddc5545ea36c8bd2b18a580, i.e. the commit that changed the icu layout to the harfbuzz layout.

Adding Cc: to Khaled Hosny

Khaled, do you have any idea if the new Common Sal Layout could be fixed to handle this correctly? (Yes, I verified that the font in the document is not missing from the system.)
Comment 2 Miklos Vajna 2017-01-20 16:03:49 UTC
Created attachment 130582 [details]
MSO screenshot.
Comment 3 Miklos Vajna 2017-01-20 16:04:06 UTC
Created attachment 130583 [details]
LO 4.0 screenshot.
Comment 4 Miklos Vajna 2017-01-20 16:04:38 UTC
Created attachment 130584 [details]
LO master (towards 5.4) screenshot.

Though this layout is basically unchanged since 4.1.
Comment 5 Miklos Vajna 2017-01-20 16:06:34 UTC
I only reproduced the bug on Linux x86_64 with gtk3 vclplug, but probably it doesn't matter, that's the whole point of the CSL, as far as I understand. :-)
Comment 6 Khaled Hosny 2017-01-20 17:17:22 UTC
I don’t know off hand, my first thought would be kerning since we always enable kerning but MSO does not (except in limited cases AFAIK), but this would have shown up even pre-CSL as we had kerning on by default since 4.1 on Linux IIRC, but  you can test this by appending “:-kern” to the font name (i.e. “Callibri:-kern”) and see if it makes any difference (we have an old way to disable kerning, but it currently ignored since not matter what I did it always came as off and no way made it on).

If this does not make a difference, then it might be the use of unhinted glyph advances; prior to CSL we used glyph advances from FreeType which would depend on the hinting mode being set, but now we use unhinted glyph advances. It does not seem right for the document layout to be dependent on the hinting mode being used.
Comment 7 Miklos Vajna 2017-01-23 08:53:00 UTC
Just selecting the text in the table and changing the font name to "Calibri:-kern" does not fix the problem here, the text is still 1 line.

> but this would have shown up even pre-CSL as we had kerning on by default since 4.1 on Linux IIRC

As I mentioned above, this bug doesn't seem to be introduced by CommonSalLayout, it actually started when the ICU -> HarfBuzz layout change happened between LO 4.0 and 4.1.

Thanks!
Comment 8 Khaled Hosny 2017-01-23 10:06:35 UTC
(In reply to Miklos Vajna from comment #7)
> Just selecting the text in the table and changing the font name to
> "Calibri:-kern" does not fix the problem here, the text is still 1 line.
> 
> > but this would have shown up even pre-CSL as we had kerning on by default since 4.1 on Linux IIRC
> 
> As I mentioned above, this bug doesn't seem to be introduced by
> CommonSalLayout, it actually started when the ICU -> HarfBuzz layout change
> happened between LO 4.0 and 4.1.

Oh, I missed that part. Now I think ligatures play a rule as well, Callibri has a “tt” ligature that would be active for the first word, but disabling ligatures does not give two lines. However, if I disable both kerning and ligatures (which would roughly match MSO and pre-HarfBuzz LO), I get the old line break:

Calibri:-liga&-kern

So I think the way forward is to make the kerning and ligature settings that we already have actually work, default to them on for new documents and turn them off for Western (and may be CJK) fonts in old/MSO documents. But this needs someone who knows the code outside VCL (i.e. not me ☺).
Comment 9 Miklos Vajna 2017-01-24 09:25:05 UTC
Great! :-)

Kerning is a character (i.e. not paragraph) property outside VCL, in Writer you can turn it on/off at Format -> Character -> Position -> Pair kerning.

It's interesting that unticking that checkbox + using the "Calibri:-liga" is still one line, I would expect that would be 2 lines.

FWIW, the document has that checkbox (properly) unticked, while for new documents it's ticked in. So nominally it looks good, except this unexpected font name vs UI checkbox difference.

Regarding ligatures, do you think we have UI anywhere to turn that on/off? If not, then perhaps we would need a second checkbox below the kerning one to control this (+ all the infrastructure behind such a checkbox: document model, storing this boolean in ODF, etc).

Thanks.
Comment 10 Khaled Hosny 2017-01-24 22:49:01 UTC
That kerning option is currently completely ignored by the layout engine. It should be eventually setting/unsetting SalLayoutFlags::KerningPairs on ImplLayoutArgs.mnFlags, but (IIRC) when I checked for it in CommonSalLayout::LayoutText() it was always unset.

We also have SalLayoutFlags::EnableLigatures, but I don’t know what part of the code sets it and (also IIRC) it was always unset.
Comment 11 Miklos Vajna 2017-01-25 10:26:26 UTC
OK, sounds like there are two issues here, then:

1) The kerning character property is not respected by CSL.

2) There is no character property that could tell CSL to or not to enable ligatures.

Would it be OK to focus on kerning in this bugreport, and create a follow-up bug for the ligature problem? If so and it helps, I can create a trivial test doc that has kerning enabled and disabled for testing purposes.

Thanks.
Comment 12 Khaled Hosny 2017-01-25 13:27:46 UTC
(In reply to Miklos Vajna from comment #11)
> OK, sounds like there are two issues here, then:
> 
> 1) The kerning character property is not respected by CSL.
> 
> 2) There is no character property that could tell CSL to or not to enable
> ligatures.
> 
> Would it be OK to focus on kerning in this bugreport, and create a
> follow-up bug for the ligature problem? If so and it helps, I can
> create a trivial test doc that has kerning enabled and disabled
> for testing purposes.

Sounds fine. Disabling the kerning in CSL is trivial, something like:

diff --git a/vcl/source/gdi/CommonSalLayout.cxx b/vcl/source/gdi/CommonSalLayout.cxx
index 34514788d949..94afb7dc4107 100644
--- a/vcl/source/gdi/CommonSalLayout.cxx
+++ b/vcl/source/gdi/CommonSalLayout.cxx
@@ -446,6 +446,12 @@ bool CommonSalLayout::LayoutText(ImplLayoutArgs& rArgs)
     hb_buffer_set_unicode_funcs(pHbBuffer, pHbUnicodeFuncs);
 #endif
 
+    if (!(rArgs.mnFlags & SalLayoutFlags::KerningPairs))
+    {
+        SAL_DEBUG("Disabling kerning");
+        maFeatures.push_back({ HB_TAG('k','e','r','n'), 0, 0, static_cast<unsigned int>(-1) });
+    }
+
     ParseFeatures(mrFontSelData.maTargetName);
 
     double nXScale = 0;


Which seems to work actually, but we seem to not set the flag on UI text, I guess that what I saw back in the day (or things changed since 2013 when I last checked that). So I guess we can just do the code above and make sure the flag is set for UI text as there is no point disabling it there.
Comment 13 Miklos Vajna 2017-01-26 09:04:33 UTC
Created attachment 130690 [details]
Kerning rerproducer document.

The second "VA" line is supposed to be longer than the previous, but in current LO master they are rendered the same.
Comment 14 Khaled Hosny 2017-02-28 15:00:21 UTC
*** Bug 102226 has been marked as a duplicate of this bug. ***
Comment 15 Miklos Vajna 2017-03-03 16:23:06 UTC
Created attachment 131606 [details]
MS-DOC kerning reproducer

I'm attaching an [MS-DOC] file that has kerning enabled in some places, just to show the MSO behavior, the old LO one and the LO one after https://gerrit.libreoffice.org/#/c/34169/ is in. It looks like the old one doesn't match the MSO one, while the new one does, so it's promising.
Comment 16 Miklos Vajna 2017-03-03 16:24:05 UTC
Created attachment 131607 [details]
MS-DOC kerning reproducer in LO, before
Comment 17 Miklos Vajna 2017-03-03 16:24:48 UTC
Created attachment 131608 [details]
MS-DOC kerning reproducer in LO, after
Comment 18 Miklos Vajna 2017-03-03 16:26:03 UTC
Created attachment 131609 [details]
MS-DOC kerning reproducer in MSO
Comment 19 Johnny_M 2017-03-03 17:37:13 UTC
(In reply to Miklos Vajna from comment #16)
> Created attachment 131607 [details]
> MS-DOC kerning reproducer in LO, before

There seems to be a mistake - it's the same file as the one "after": kerning-lo-after.png


Just on a side note for people checking this bug report in future: With introduction of HarfBuzz on all platforms (bug 89870) in the LO version 5.3, this issue affects all platforms, not only Linux. (On the latter it was introduced earlier - in the version 4.1: https://wiki.documentfoundation.org/ReleaseNotes/4.1#Core , which is the reason for this issue to be referring to the version 4.1.)
Comment 20 Miklos Vajna 2017-03-06 12:01:01 UTC
Created attachment 131667 [details]
MS-DOC kerning reproducer in LO, before, second try.

I've attached the wrong screenshot for the "kerning in LO, before" case.
Comment 21 Commit Notification 2017-03-14 14:52:20 UTC
Khaled Hosny committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ded07624096183ed310187f29d4692bb39b7d24a

tdf#105454: Stop ignoring font kerning setting

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Khaled Hosny 2017-03-28 18:49:38 UTC
*** Bug 66818 has been marked as a duplicate of this bug. ***
Comment 23 Commit Notification 2017-03-30 10:20:13 UTC
Miklos Vajna committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=38b0c24fa5cbb4246e03d77ac022dfdc9fdede03

Related: tdf#105454 DOCX import: fix unwanted enabled-by-default kerning

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Miklos Vajna 2017-03-31 08:54:59 UTC
Marking as resolved, the kerning / Linux regression is fixed; I'll file a follow-up bug for the ligature thing.
Comment 25 Luke 2017-04-10 03:14:30 UTC
The fix for this bug is causing text in tables to be cut off on Windows. See Bug 107055 for details.