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.
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.)
Created attachment 130582 [details] MSO screenshot.
Created attachment 130583 [details] LO 4.0 screenshot.
Created attachment 130584 [details] LO master (towards 5.4) screenshot. Though this layout is basically unchanged since 4.1.
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. :-)
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.
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!
(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 ☺).
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.
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.
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.
(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.
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.
*** Bug 102226 has been marked as a duplicate of this bug. ***
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.
Created attachment 131607 [details] MS-DOC kerning reproducer in LO, before
Created attachment 131608 [details] MS-DOC kerning reproducer in LO, after
Created attachment 131609 [details] MS-DOC kerning reproducer in MSO
(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.)
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.
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.
*** Bug 66818 has been marked as a duplicate of this bug. ***
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.
Marking as resolved, the kerning / Linux regression is fixed; I'll file a follow-up bug for the ligature thing.
The fix for this bug is causing text in tables to be cut off on Windows. See Bug 107055 for details.